Fwd: [llvm] r182233 - Partially revert change in r181200 that tried to simplify JIT unit test #ifdefs.

[Ulrich Weigand]

Bob Wilson <bob.wilson at apple.com> wrote on 20.05.2013 08:17:04:

> Sorry for taking so long to report this issue, but it is breaking
> one of our ARM buildbots.  Hopefully the commit log is sufficient to
> explain the problem.  Let me know if I can do anything to help find
> a better solution.

> The export list for this test requires the following symbols to be
available:
>  JITTest_AvailableExternallyFunction
>  JITTest_AvailableExternallyGlobal
> The change in r181200 commented them out, which caused the test to fail
to
> link, at least on Darwin. I have only reverted the change for arm, since
I
> can't test the other targets and since it sounds like that change was
fixing
> real problems for those other targets. It should be possible to rearrange
the
> code to keep those definitions outside the #ifdefs, but that should be
done by
> someone who can reproduce the problems that r181200 was trying to fix.

Hi Bob,

sorry for the breakage, I didn't notice that those functions were
mentioned in the .def file.  If I understand correctly, this would
cause problems whenever LLVM was built on a Windows host platform,
not only when targeting ARM.  So the fix really ought to handle
PowerPC and SystemZ too.

My change originally intended to address two issues:

(1) When building the unit tests using Clang as host compiler
    (in particular, when bootstrapping), we would get spurious
    "unused function" warning messages for some helper routines.

(2) The #ifdef condition was duplicated many times, making it
    awkward to add a new platform that also wants to disable
    old-style JIT unit tests.

Now, those "AvailableExternally" symbols clearly need to be defined
unconditionally, given that they are referenced from the .def file.
As you mention, a better solution to that effect would be to rearrange
the code to keep those definitions outside the #ifdefs.  This will
still address issues (1) and (2) above, since *those* definitions
will *not* trigger any warnings (since they are marked as external).

Also, since those two definitions need to be outside the anonymous
namespace covering the rest of the file, it makes sense to pull them
up to the top of the file anyway.

The attached patch reverts your 182233 revision and instead pulls the
two definitions up.  This still allows me to bootstrap with no
warnings on SystemZ.  Can you confirm that this still solves your
problem?

Thanks,
Ulrich

(See attached file: diff-jit-unittest)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-jit-unittest
Type: application/octet-stream
Size: 5965 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130520/8b49f7a5/attachment.obj>