Page MenuHomePhabricator

[runtimes] Remove support for GCC-style 32 bit multilib builds
ClosedPublic

Authored by ldionne on Nov 23 2021, 1:39 PM.

Details

Reviewers
Mordante
danalbert
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGfa1c077b41ae: [runtimes] Remove support for GCC-style 32 bit multilib builds
Summary

This patch removes the ability to build the runtimes in the 32 bit
multilib configuration, i.e. using -m32. Instead of doing this, one
should cross-compile the runtimes for the appropriate target triple,
like we do for all other triples.

As it stands, -m32 has several issues, which all seem to be related to
the fact that it's not well supported by the operating systems that
libc++ support. The simplest path towards fixing this is to remove
support for the configuration, which is also the best course of action
if there is little interest for keeping that configuration. If there
is a desire to keep this configuration around, we'll need to do some
work to figure out the underlying issues and fix them.

Diff Detail

Event Timeline

ldionne created this revision.Nov 23 2021, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 1:39 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Nov 23 2021, 1:39 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 23 2021, 1:39 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

This patch was motivated by D114385, where I did some work to fix libunwind when running on sanitizers. One thing leading to the next, I ended up hitting issues with the 32 bit configuration, and decided I should ask around whether anybody actually wants to keep it around.

Pinging some vendors (or folks who work on other platforms) to hear whether they care about that configuration.

@dim @phosek @mstorsjo @thakis @tstellar @danalbert

No objection from me; in my setups, everything is essentially cross compiled anyway, and building for same-arch-32-bit on a 64 bit host would be just as much cross as any other target.

libcxx/docs/ReleaseNotes.rst
155

Would it be good to be a bit more helpful and suggest how to build for such a configuration, if that's needed? I.e. -DLLVM_ENABLE_RUNTIMES=libcxx and the corresponding -DLLVM_RUNTIME_TARGETS=your-32-bit-triple, or pointing cmake at llvm-project/runtimes and just passing -m32 in CMAKE_CXXFLAGS?

MaskRay added a comment.EditedNov 23 2021, 2:17 PM

[runtimes] Remove support for building in 32 bits mode on a 64 bit target

I think this needs to be clarified. It is about the GCC multilib style -m32 (inherited to Clang), not about the general cross-compilation feature.

I think there are still users with 32-bit cross compilation need on 64-bit host. Do you have a supported build instruction for them?

AIUI, some compiler-rt sanitizer tests (e.g. %clangxx_asan) need -m32 for i686-pc-linux-gnu testing on x86_64-pc-linux-gnu. If -m32 is unsupported, they need a fallback.

I wasn't involved in development of this feature, but I assume it was intended for the 32-bit multilib build? If so, I'm not sure how useful this is because you cannot build both 32-bit and 64-bit library in a single build, and the build system doesn't use the multilib layout, so you still need to do two builds and then some manual copying. Passing the right triple to the build should be equivalent in terms of effort required on the user side so I'm supportive of removing this feature.

ldionne marked an inline comment as done.Nov 25 2021, 12:17 PM

[runtimes] Remove support for building in 32 bits mode on a 64 bit target

I think this needs to be clarified. It is about the GCC multilib style -m32 (inherited to Clang), not about the general cross-compilation feature.

Yes, indeed, I'll clarify the commit message. In fact, I wan't 100% aware of the backstory before you folks commented -- I'm glad I pinged several people now.

I think there are still users with 32-bit cross compilation need on 64-bit host. Do you have a supported build instruction for them?

AIUI, some compiler-rt sanitizer tests (e.g. %clangxx_asan) need -m32 for i686-pc-linux-gnu testing on x86_64-pc-linux-gnu. If -m32 is unsupported, they need a fallback.

I assume it should be possible to build the runtimes using the target they actually need, i.e. i686-pc-linux-gnu. They should be able to add -DLLVM_RUNTIME_TARGETS=i686-pc-linux-gnu when configuring their build of the runtimes to achieve that.

libcxx/docs/ReleaseNotes.rst
155

Good suggestion, I improved the release note. Please LMK if that's still not enough after I update.

ldionne updated this revision to Diff 389862.Nov 25 2021, 12:23 PM
ldionne marked an inline comment as done.
ldionne retitled this revision from [runtimes] Remove support for building in 32 bits mode on a 64 bit target to [runtimes] Remove support for GCC-style 32 bit multilib builds.
ldionne edited the summary of this revision. (Show Details)

Update commit message and improve the release note.

I just tested a bootstrapping build where I built the runtimes for i386-pc-linux-gnu
and ran it on the Docker image we use for CI, and everything worked.

ldionne updated this revision to Diff 390425.Nov 29 2021, 11:47 AM

Rebase onto main.

Is everybody happy with the way this is worded now? I'll ship this by the end of the week unless someone objects.

@manojgupta @vitalybuka it would be great if you folks could try this out with the sanitizers setup to make sure this doesn't break you.

Thanks for adding me. This is going to break Chrome OS 32-bit builds so I'll need to check if I can remove uses of LIBCXX_BUILD_32_BITS with something more suitable.

Mordante accepted this revision as: Mordante.Nov 29 2021, 12:02 PM
Mordante added a subscriber: Mordante.

LGTM from the code changes PoV. Should we, in a separate patch, add a 32-bit CI job to keep testing 32-bit Linux?

Will it also fix the bug https://bugs.llvm.org/show_bug.cgi?id=49294 related to m32?

Yes, moving to a proper triple would fix that issue!

danalbert accepted this revision.Nov 30 2021, 2:21 PM
danalbert added a subscriber: pirama.

Doesn't seem like we use this for Android, but @pirama just in case.

Doesn't seem like we use this for Android, but @pirama just in case.

Thanks for looping me in. We don't use this in Android.

I have removed the usage of this flag from Chrome OS so fine for us as well.

ldionne accepted this revision as: Restricted Project, Restricted Project, Restricted Project.Dec 1 2021, 9:56 AM

Thanks all! I'm going to check this in. I'll actually tone done from FATAL_ERROR to WARNING in this patch, but I will commit another patch immediately after that moves it to FATAL_ERROR -- that way we can revert the FATAL_ERROR one instead of this whole patch if things break unexpectedly.

This revision is now accepted and ready to land.Dec 1 2021, 9:56 AM
This revision was landed with ongoing or failed builds.Dec 1 2021, 9:57 AM
This revision was automatically updated to reflect the committed changes.

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

Ah, yeah, sorry for missing that. Any reason why you can't use target triple approach mentioned above?

SeanP added a comment.Dec 6 2021, 1:31 PM

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

Ah, yeah, sorry for missing that. Any reason why you can't use target triple approach mentioned above?

I think we will be able to. I was noticing all of the "32bit-on-64bits" tags removed from the tests. That seems more involved than just using the triple. I'll let you know if we need more that just using the target triple.

Hi Louis, you missed us on z/OS and AIX (@daltenty) . We provide 32-bit builds. We'll need to recover from this and may need to add some things back.

Ah, yeah, sorry for missing that. Any reason why you can't use target triple approach mentioned above?

I think we will be able to. I was noticing all of the "32bit-on-64bits" tags removed from the tests. That seems more involved than just using the triple. I'll let you know if we need more that just using the target triple.

Those tags had been added by me to work around an issue we couldn't reproduce in normal multilibs build -- it was only showing up on the CI nodes in Google Compute Engine (not even on a Docker container running locally). I don't think the removal of those tags will have any effect on other people including you.