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

[vsk]

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

[aprantl]

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]

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.

[teemperor]

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]

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

[vsk]

In D75091#1891060, @teemperor wrote:

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]

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]

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

[vsk]

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]

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?

[teemperor]

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)

[teemperor]

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

[vsk]

In D75091#1892170, @teemperor wrote:

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]

@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). */

[teemperor]

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]

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]

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

[teemperor]

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

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h

 MCRegister getX86SubSuperRegisterOrZero(MCRegister, unsigned,
                                         bool High = false);
 
 } // End llvm namespace
 
+// Some Unix systems define an FSCALE macro in sys/param.h. This macro somehow
+// gets pulled into this header when -fmodules is enabled, preventing us from
+// textually including X86GenInstrInfo.inc.
+#undef FSCALE
 
 // Defines symbolic names for X86 registers.  This defines a mapping from
 // register name to register number.
 //
 #define GET_REGINFO_ENUM