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

[Bob Wilson]

On May 20, 2013, at 4:33 AM, Ulrich Weigand <Ulrich.Weigand at de.ibm.com> wrote:

> 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?

Yes, I think that ought to work.  That is exactly what I was suggesting about rearranging the code.  I haven't actually tried to reproduce the exact test that our buildbot is running, but let's go ahead with your patch and see what happens.

BTW, the JITTests.def file is converted by the Makefile in that directory to a JITTests.exports file that gets used on other platforms besides Windows.  We very recently added support for -export-dynamic to Apple's linker, but without that, an LTO build of LLVM requires those exports for the unit tests.  When newer versions of the linker are more widespread, I will try to remove the exports file for non-Windows platforms, since the GNU linker has supported -export-dynamic for a long time.