Page MenuHomePhabricator

Fix lld macho standalone build by including llvm/Config/llvm-config.h instead of llvm/Config/config.h
ClosedPublic

Authored by mceier on May 7 2021, 10:41 AM.

Details

Summary

lld/MachO/Driver.cpp and lld/MachO/SyntheticSections.cpp include llvm/Config/config.h which doesn't exist when building standalone lld.

This patch replaces llvm/Config/config.h include with llvm/Config/llvm-config.h just like it is in lld/ELF/Driver.cpp and HAVE_LIBXAR with LLVM_HAVE_LIXAR and moves LLVM_HAVE_LIBXAR from config.h to llvm-config.h

Also it adds LLVM_HAVE_LIBXAR to LLVMConfig.cmake and links liblldMachO2.so with XAR_LIB if LLVM_HAVE_LIBXAR is set.

Diff Detail

Event Timeline

mceier created this revision.May 7 2021, 10:41 AM
mceier requested review of this revision.May 7 2021, 10:41 AM
int3 accepted this revision.May 7 2021, 10:53 AM

Thanks!

This revision is now accepted and ready to land.May 7 2021, 10:53 AM
thakis requested changes to this revision.May 7 2021, 10:57 AM
thakis added a subscriber: thakis.

Wait, this isn't right, is it? We need this for #ifndef HAVE_LIBXAR and HAVE_LIBXAR is in config.h but not in llvm-config.h. This will silently break HAVE_LIBXAR checks.

This revision now requires changes to proceed.May 7 2021, 10:57 AM

(ie you'll have to move HAVE_LIBXAR from config.h to llvm-config.h too)

Wait, this isn't right, is it? We need this for #ifndef HAVE_LIBXAR and HAVE_LIBXAR is in config.h but not in llvm-config.h. This will silently break HAVE_LIBXAR checks.

I think you're right, though I have no idea how to move HAVE_LIBXAR correctly - I doubt that just moving it from include/llvm/Config/config.h.cmake to include/llvm/Config/llvm-config.h.cmake is a good idea.

llvm-config.h seems to create defines with LLVM_ prefix only.

mceier updated this revision to Diff 343729.May 7 2021, 11:45 AM

Moved HAVE_LIBXAR from config.h to llvm-config.h

Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 11:45 AM
thakis accepted this revision.May 7 2021, 3:59 PM

Lgtm assuming you checked that the XAR ifdefs in the two files you're changing still work (with a temporary local #error in the right place or similar)

llvm/include/llvm/Config/llvm-config.h.cmake
97

Maybe fix the missing word in the comment while here :)

This revision is now accepted and ready to land.May 7 2021, 3:59 PM
mceier updated this revision to Diff 343816.May 8 2021, 12:01 AM
mceier edited the summary of this revision. (Show Details)

Added missing word in a comment for HAVE_LIBXAR define

mceier updated this revision to Diff 343819.May 8 2021, 12:53 AM
mceier marked an inline comment as done.
mceier edited the summary of this revision. (Show Details)

Verified that standalone lld builds with HAVE_LIBXAR set and unset.
Added HAVE_LIBXAR to LLVMConfig.cmake and linking liblldMachO2.so with XAR_LIB if HAVE_LIBXAR is set.

Looks great. Do you need me to land this for you, or can you commit yourself?

mceier requested review of this revision.May 10 2021, 8:30 AM

Looks great. Do you need me to land this for you, or can you commit yourself?

I don't have commit rights, so I need you to land this for me, if everything looks good to you ;)

Sorry about the delay here.

llvm/cmake/modules/LLVMConfig.cmake.in
114

While patching this in locally to land, I noticed that this doesn't have the trailing @ that all other lines in this file have. Are you sure this does the right thing as-is?

Also, everything in this file and almost everything in llvm-config.h starts with LLVM_ which makes sense if the purpose of this file is to include it in external codebases. Maybe we should add an LLVM_ prefix to this and the other symbol in this change? (But not sure about this part.)

Sorry about the delay here.

No problem ;)

llvm/cmake/modules/LLVMConfig.cmake.in
114

While patching this in locally to land, I noticed that this doesn't have the trailing @ that all other lines in this file have. Are you sure this does the right thing as-is?

Hmm; it probably is not right - if(HAVE_LIBXAR) will always be true in this case and always link with xar, so I missed one case when xar is not available on the system when testing.

Maybe we should add an LLVM_ prefix to this and the other symbol in this change?

Ok, I can add the prefix.

Will send patches soon. Thanks for the review ;)

mceier updated this revision to Diff 345560.May 14 2021, 2:12 PM
mceier edited the summary of this revision. (Show Details)

Added LLVM_ prefix to all occurrences of HAVE_LIBXAR; fixed one missing char.

mceier marked an inline comment as done.May 14 2021, 2:13 PM
thakis accepted this revision.May 19 2021, 8:06 AM
This revision is now accepted and ready to land.May 19 2021, 8:06 AM