The current code for GNU/Linux is actually completely generic, and can be moved to Gnu, so it can benefit GNU/Hurd and GNU/kFreeBSD
Details
- Reviewers
kristina sammccall MaskRay arsenm phosek lebedev.ri - Commits
- rG68e89c5b9603: [Gnu toolchain] Move GCC multilib/multiarch paths support from Linux to Gnu Add…
rGc298e5a02292: [Gnu toolchain] Move GCC multilib/multiarch paths support from Linux to Gnu
rG63959803702c: [Driver] Move GCC multilib/multiarch paths support from Linux.cpp to Gnu.cpp
Diff Detail
Event Timeline
There is no behavior change, I checked for no regression on GNU/Linux with ninja check-all.
- Please clang-format your changes
- Hurd stuff is not a NFC refactoring, but adding some new feature - tests missing, i think?
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?
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.
Moving functions from Linux.cpp to Gnu.cpp is definitely appropriate. Have you made any adjustment?
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2694 | 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. |
No, it's really pure code move (and the requested codestyle fixes)
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2694 | Alright, fixing so |
Can you provide your name <email@address>? I need this information to run git c --amend --author=
Maybe rename AddGCCClangSystemIncludeArgs to AddMultilibIncludeArgs for brevity/clarity?