Page MenuHomePhabricator

[X86MCTargetDesc.h] Speculative fix for macro collision with sys/param.h
AbandonedPublic

Authored by vsk on Feb 24 2020, 4:27 PM.

Details

Summary

This is a speculative fix for the following modules build failure:

$ ... -c /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/Target/X86/X86AvoidTrailingCall.cpp
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/Target/X86/X86AvoidTrailingCall.cpp:17:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/Target/X86/X86InstrInfo.h:16:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:19:
In file included from /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h:139:
lib/Target/X86/X86GenInstrInfo.inc:1029:5: error: expected identifier
    FSCALE      = 1014,
    ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/sys/param.h:251:16: note: expanded from macro 'FSCALE'
#define FSCALE  (1<<FSHIFT)
                ^
1 error generated.

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/10254

I'm really not sure what's going on here or whether the fix is appropriate,
any guidance very much appreciated!

rdar://59740397

Diff Detail

Event Timeline

vsk created this revision.Feb 24 2020, 4:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 4:27 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript
aprantl added inline comments.Feb 24 2020, 4:38 PM
llvm/include/llvm/module.modulemap
43 ↗(On Diff #246339)

Would it be even safer to use textual here, so it doesn't get absorbed into another module? (Not sure about the exact semantics of exclude either).

vsk marked an inline comment as done.Feb 24 2020, 4:48 PM

Hm, I can't reproduce the build failure on a fresh llvm checkout on the bot (without this patch, that is). If I'm able to reproduce, I'll try out the 'textual header' and 'exclude header' approaches.

Are you using the same SDK version as the build machine? This seems to me like (yet another) no-submodule-local-visibility issue. X86GenInstrInfo.inc has no includes and should be fully isolated, but it seems one of the previous headers in LLVM_Backend_Target is pulling in the module for param.h and so we get that macro in here. Maybe whatever SDK you are using is not making the param.h submodule visible in the LLVM_Backend_Target implementation so that's why we survive the build on your machine.

aprantl accepted this revision.Feb 25 2020, 9:09 AM

Anyway, please try landing this with "textual" first and change it to exclude if it doesn't work.

This revision is now accepted and ready to land.Feb 25 2020, 9:09 AM
vsk added a comment.Feb 25 2020, 10:25 AM

Are you using the same SDK version as the build machine?

No, I can't reproduce this locally. But I also set up a fresh llvm checkout on the builder, fired off a modules build, and could not reproduce there either. I've since cleared the build directory for the lldb-cmake job, but the issue still reproduces for that job as of this morning.

vsk added a comment.Feb 25 2020, 10:42 AM

I disabled the bot and ran ninja in the lldb-cmake workspace directly: I could reproduce the problem this way. Neither exclude header "Target/X86/X86GenInstrInfo.inc" nor exclude textual "Target/X86/X86GenInstrInfo.inc" addressed the issue. I'm really not sure how to fix this now, other than maybe adding #undef FSCALE to X86MCTargetDesc.h.

vsk updated this revision to Diff 246513.Feb 25 2020, 10:48 AM
vsk retitled this revision from [modulemap] Speculative fix for macro collision with sys/param.h to [X86MCTargetDesc.h] Speculative fix for macro collision with sys/param.h.

I tested out doing #undef FSCALE, and this resolved the build breakage on our bot.

vsk added a comment.Feb 25 2020, 10:53 AM

I'll land this as a stopgap despite not really understanding what caused this, as I don't have a better solution on hand. I'd be happy to see this reverted in favor of something more principled.

vsk added a comment.Feb 25 2020, 1:30 PM

I thought this was enough to unblock the bot, but we're pulling in macros in other places as well:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp:177:29: error: expected identifier
enum Format { Default, GNU, BSD, DARWIN, Unknown };
                            ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/sys/param.h:72:13: note: expanded from macro 'BSD'
#define BSD     199506          /* System version (year & month). */
                ^

I don't think it's acceptable to #undef our way out of this. Are the system/SDL headers incorrect, or is llvm's modulemap incorrect? Something else?

Can you check if adding -Xclang -fmodules-local-submodule-visibility allows you to get a passing clang/LLVM build? (LLDB isn't fully building with that mode yet but I have a fix for that)

See also reviews D74891 (the LLDB fix) and D74892 (enables local submodule visibility by default).

vsk added a comment.Feb 25 2020, 1:40 PM

Can you check if adding -Xclang -fmodules-local-submodule-visibility allows you to get a passing clang/LLVM build? (LLDB isn't fully building with that mode yet but I have a fix for that)

On it, I'll pause the bot now and check.

vsk added a comment.Feb 25 2020, 1:52 PM

@teemperor the build fails in the same place with D74891 and D74892 applied, you can see here that '-Xclang -fmodules-local-submodule-visibility' gets picked up:

FAILED: tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.o 
/Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/llvm-ar -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/tools/llvm-ar -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/libxml2 -Iinclude -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include -Wdocumentation -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -fmodules -fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache -fcxx-modules -Xclang -fmodules-local-submodule-visibility -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O3  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.o -MF tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.o.d -o tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.cpp.o -c /Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/tools/llvm-ar/llvm-ar.cpp:177:29: error: expected identifier
enum Format { Default, GNU, BSD, DARWIN, Unknown };
                            ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/sys/param.h:72:13: note: expanded from macro 'BSD'
#define BSD     199506          /* System version (year & month). */

That's really not great... Puh.

What about just disabling LLVM_ENABLE_MODULES until we figure this out? That way we're at least not flying blind without any LLDB tests running on macOS. If you can keep the build directory around that would be great (otherwise I'll see if I can reproduce on the machine tomorrow with my own setup).

vsk added a comment.Feb 25 2020, 2:34 PM

Sounds good, I'll turn the bot back on without LLVM_ENABLE_MODULES. @Bigcheese and I had a look: we were able to complete the llvm-ar build using the system's clang compiler, so we suspect a recent clang regression we captured a modules crash reproducer (attached to 59740397).

vsk abandoned this revision.Feb 25 2020, 2:39 PM

Modules build disabled in llvm-zorg, I'll file a bug to re-enable it.

e2ed346e (HEAD -> master, origin/master, origin/HEAD) lldb-cmake job: Temporarily disable LLVM_ENABLE_MODULES

I just saw the commit 621388468b5125f05d649262681676bed2faefe6 and that seems a lot like the problem we have here. So I guess libc++ could include that header and then leak it into this TU. I'll check it out once lldb-incremental is back from the dead