This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Fix compiler-rt lookup for x32
ClosedPublic

Authored by hvdijk on Apr 8 2021, 3:46 PM.

Details

Summary

x86_64-linux-gnu and x86_64-linux-gnux32 use different ABIs and objects built for one cannot be used for the other. In order to build and use compiler-rt for x32, we need to treat x32 as a new arch there. This updates the driver to search using the new arch name.

Diff Detail

Event Timeline

hvdijk created this revision.Apr 8 2021, 3:46 PM
hvdijk requested review of this revision.Apr 8 2021, 3:46 PM

Thanks, this finally fixes the build for me. I wasn't aware that there was a getArchNameForCompilerRTLib() function in clang.

glaubitz added inline comments.Apr 9 2021, 3:35 AM
clang/test/Driver/sanitizer-ld.c
309

Do we need want to run the test for i386 anymore?

glaubitz added inline comments.Apr 9 2021, 3:36 AM
clang/test/Driver/sanitizer-ld.c
309

Do we _not_, not _need_. Sorry.

hvdijk added inline comments.Apr 9 2021, 3:38 AM
clang/test/Driver/sanitizer-ld.c
309

This test just somewhat arbitrarily distributes the different sanitizer modules over i386 and x86_64 for testing rather than testing each module for each arch, so I figured there was no harm in changing one of them to x86_64-linux-gnux32. i386 is still getting tested elsewhere.

glaubitz accepted this revision.Apr 9 2021, 3:41 AM
glaubitz added inline comments.
clang/test/Driver/sanitizer-ld.c
309

OK, thanks. Then I will mark this as approved from my side.

This revision is now accepted and ready to land.Apr 9 2021, 3:41 AM

So, we only need D99988 and this one (D100148) now and the LLVM package will finally build on Debian without any additional tweaks.

Hi! Can we get this patch merged as-is or do we need anything else?

Hi! Can we get this patch merged as-is or do we need anything else?

Sorry, miscommunication. I was thinking you could use this to update D99988 to actually build the relevant compiler-rt bits for x32, and then once both that and this are ready, both can be submitted at the same time. The reason for that is that this is not useful by itself, and if it turns out that D99988 actually requires some other clang change as well that we are not seeing yet, it will be easier to update this if it has not been pushed yet. Do you think it would be better to push this first?

Hi! Can we get this patch merged as-is or do we need anything else?

Sorry, miscommunication. I was thinking you could use this to update D99988 to actually build the relevant compiler-rt bits for x32, and then once both that and this are ready, both can be submitted at the same time. The reason for that is that this is not useful by itself, and if it turns out that D99988 actually requires some other clang change as well that we are not seeing yet, it will be easier to update this if it has not been pushed yet. Do you think it would be better to push this first?

Understood. However, I'm not really sure what else we need. Do we just take the architecture definition from here to pass the proper flags to the compiler or do we also need to add
some x32-specific code?

Understood. However, I'm not really sure what else we need. Do we just take the architecture definition from here to pass the proper flags to the compiler or do we also need to add
some x32-specific code?

My personal opinion: I would say to enable the compiler-rt bits that are already correct for x32, which should only require passing the proper flags. Beyond the libsanitizer bits, I do not know which other bits, if any, might already be correct for x32 though.

@hvdijk I just noticed that the sanitizer defines SANITIZER_X32 for x32, so I assume your patch itself is already correct.

hvdijk updated this revision to Diff 359016.Jul 15 2021, 9:36 AM
hvdijk edited the summary of this revision. (Show Details)
hvdijk added a reviewer: MaskRay.

Updated to use Triple.isX32().

Should we perhaps just merge this as is, ahead of the update to compiler-rt to create x32 objects? For non-x32, this change is NFC, for x32, the change results in a better error message about compiler-rt objects not existing rather than being for the wrong architecture.

Updated to use Triple.isX32().

Should we perhaps just merge this as is, ahead of the update to compiler-rt to create x32 objects? For non-x32, this change is NFC, for x32, the change results in a better error message about compiler-rt objects not existing rather than being for the wrong architecture.

Yes, I fully agree on this stance. I've been waiting for this patch for Debian, plus the remaining changes to get Rust to build on x32 ;-).

This revision was landed with ongoing or failed builds.Jul 15 2021, 12:52 PM
This revision was automatically updated to reflect the committed changes.

Since @glaubitz is here:

I want to set LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to on by default so that the libraries for x86-64 will be in lib/clang/13.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.profile.a instead of lib/clang/13.0.0/lib/linux/libclang_rt.profile-x86_64.a.
Clang driver supports both paths.

Since @glaubitz is here:

I want to set LLVM_ENABLE_PER_TARGET_RUNTIME_DIR to on by default so that the libraries for x86-64 will be in lib/clang/13.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.profile.a instead of lib/clang/13.0.0/lib/linux/libclang_rt.profile-x86_64.a.
Clang driver supports both paths.

OK, I don't think this will have any ramifications for Debian. But if you want me to test a patch, please let me know or pull me into the review.