Page MenuHomePhabricator

[Gnu toolchain] Move GCC multilib/multiarch paths support from Linux to Gnu
ClosedPublic

Authored by sthibaul on Feb 2 2020, 1:54 AM.

Diff Detail

Event Timeline

sthibaul created this revision.Feb 2 2020, 1:54 AM

There is no behavior change, I checked for no regression on GNU/Linux with ninja check-all.

  1. Please clang-format your changes
  2. Hurd stuff is not a NFC refactoring, but adding some new feature - tests missing, i think?
sthibaul updated this revision to Diff 243359.Feb 8 2020, 4:28 AM

Right, here is an updated patch

I feel like in this specific case it may be worth splitting this into two patches:

  • Factoring certain stuff out of Linux.cpp into Gnu.(cpp|h).
  • Adding that machinery to Hurd.cpp with related tests.

Also forgive me if I'm wrong but isn't Hurd only limited to one GCC target triple (i386-gnu) for the time being? A lot of the factored out bits seem to be still only applicable to Linux at a glance, in which case I'm not entirely sure of the rationale behind most of the refactoring. Since most people (including me) are unlikely to be intimately familiar with the directory/library layout on Hurd, is it possible to provide some more background on the Hurd-specific side of the patch?

clang/lib/Driver/ToolChains/Gnu.h
324

Maybe rename AddGCCClangSystemIncludeArgs to AddMultilibIncludeArgs for brevity/clarity?

I feel like in this specific case it may be worth splitting this into two patches:

Alright, doing so.

isn't Hurd only limited to one GCC target triple (i386-gnu) for the time being?

Right now, yes, but we'll want to bootstrap a 64bit port sooner or later. Better have that part ready in llvm to lessen the work of the bootstrap.

A lot of the factored out bits seem to be still only applicable to Linux at a glance,

? Everything I have seen in here looked to me only gcc-ish way of handling multilib and multiarch. It is used the same on other GNU systems using gcc: not only GNU/Hurd, but also GNU/kFreeBSD. I do not see anything specific to Linux there, only gcc-specific, which is the point of Gnu.cpp :)

most people (including me) are unlikely to be intimately familiar with the directory/library layout on Hurd, is it possible to provide some more background on the Hurd-specific side of the patch?

It is just a matter of applying the same gcc-ish file layout which is currently missing from the existing Hurd.cpp, thus making multilib/multiarch fail to work. I don't really know what to explain beyond that: it's just the GNU toolchain way, there is no difference between GNU/Linux and GNU/Hurd in that regard. Do you perhaps have more precise questions?

sthibaul updated this revision to Diff 243378.Feb 8 2020, 9:37 AM
sthibaul retitled this revision from [Gnu toolchain] Look at standard GCC multilib/multiarch paths by default to [Gnu toolchain] Move GCC multilib/multiarch paths support from Linux to Gnu.
sthibaul marked an inline comment as done.Feb 8 2020, 9:39 AM

> I feel like in this specific case it may be worth splitting this into two patches:

Alright, doing so.

It is now on https://reviews.llvm.org/D74282

I however didn't find how to specify in phabricator a dependency between the two diffs, is that supported in phabricator?

I however didn't find how to specify in phabricator a dependency between the two diffs, is that supported in phabricator?

Ah, it can be set as child revision after creating the diff, done so.

Hello, any news on this?

lebedev.ri resigned from this revision.Apr 8 2020, 11:49 PM
lebedev.ri added reviewers: MaskRay, arsenm, phosek.

Moving functions from Linux.cpp to Gnu.cpp is definitely appropriate. Have you made any adjustment?

phosek added inline comments.Apr 9 2020, 12:16 AM
clang/lib/Driver/ToolChains/Gnu.cpp
2698

The class already has GCC in the name, so there's no need to repeat GCC in the name of the method, i.e. this can be just Generic_GCC::AddMultilibPaths. The same for other new methods.

sthibaul marked 2 inline comments as done.Apr 9 2020, 3:51 PM

Moving functions from Linux.cpp to Gnu.cpp is definitely appropriate. Have you made any adjustment?

No, it's really pure code move (and the requested codestyle fixes)

clang/lib/Driver/ToolChains/Gnu.cpp
2698

Alright, fixing so

sthibaul updated this revision to Diff 256436.Apr 9 2020, 3:52 PM
sthibaul marked an inline comment as done.
MaskRay accepted this revision.Apr 9 2020, 4:29 PM

Looks great! You may need to wait for @phosek to confirm.

This revision is now accepted and ready to land.Apr 9 2020, 4:29 PM
phosek accepted this revision.Apr 23 2020, 11:51 AM

LGTM

Thanks! I don't have commit access, could somebody commit this for me?

Thanks! I don't have commit access, could somebody commit this for me?

Can you provide your name <email@address>? I need this information to run git c --amend --author=

@MaskRay : Samuel Thibault <samuel.thibault@ens-lyon.org>

This revision was automatically updated to reflect the committed changes.