Page MenuHomePhabricator

[Driver] Fix compiler-rt lookup for x32
AcceptedPublic

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

Details

Reviewers
glaubitz
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.

This is a prerequisite for @glaubitz's D99988 and is not useful on its own. It should be submitted together with that when that is ready.

The arch name used here is "x32" to match D99988. If that should turn out to not be specific enough, it could be changed to something like "x86_64_x32".

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.