Page MenuHomePhabricator

[Solaris] gcc toolchain handling revamp
ClosedPublic

Authored by fedor.sergeev on Jul 21 2017, 6:58 PM.

Details

Summary

General idea is to utilize generic (mostly Generic_GCC) code
and get rid of Solaris-specific handling as much as possible.

In particular:

  • scanLibDirForGCCTripleSolaris was removed, relying on generic CollectLibDirsAndTriples
  • findBiarchMultilibs is now properly utilized to switch between m32 and m64 include & lib paths on Solaris
  • C system include handling copied from Linux (bar multilib hacks)

Fixes PR24606.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Jul 21 2017, 6:58 PM
aaron.ballman added inline comments.Aug 2 2017, 10:33 AM
lib/Driver/ToolChains/Gnu.cpp
1511 ↗(On Diff #107769)

Add a period at the end of the comment.

1732 ↗(On Diff #107769)

s/Now/Next, look for
Add a period at the end of the comment.

1734 ↗(On Diff #107769)

s/typically/Typically
Add a period at the end of the comment.

1807 ↗(On Diff #107769)

I'd drop the llvm:: here.

1813–1814 ↗(On Diff #107769)

This comment can probably be re-flowed, and needs a period at the end of it.

1837 ↗(On Diff #107769)

s/non/Non
Needs a period at the end of the comment.

1840 ↗(On Diff #107769)

This is a good question to get an answer to before committing. :-) I don't know the answer myself, unfortunately.

lib/Driver/ToolChains/Gnu.h
253 ↗(On Diff #107769)

Might as well drop the llvm:: since the namespace isn't used for SmallVectorImpl.

309 ↗(On Diff #107769)

s/but CrossWindows/but the CrossWindows
s/cant/can't
Add a period at the end of the comment.

lib/Driver/ToolChains/Solaris.cpp
151 ↗(On Diff #107769)

s/both architecture/both an architecture

152 ↗(On Diff #107769)

s/as more/as a more

Add a period at the end of the comment.

160 ↗(On Diff #107769)

s/if/If

208 ↗(On Diff #107769)

Shouldn't use auto here because the type is not spelled out in the initialization.

Corrected comments as suggested by Aaron.
Will be replying on other suggestions inline.

fedor.sergeev marked 11 inline comments as done.
fedor.sergeev added inline comments.
lib/Driver/ToolChains/Gnu.cpp
1840 ↗(On Diff #107769)

So we need somebody that could answer it! :)
Last person who touched these paths was Tom Stellard.
Adding as a reviewer.

lib/Driver/ToolChains/Solaris.cpp
208 ↗(On Diff #107769)

I believe this is the case described in LLVM coding standard as "when the type would have been abstracted away anyways". It would be MultilibSet::IncludeDirsFunc, which IMO does not add anything to readability.

Aside from a coding style nit and the unanswered question that hopefully @tstellar can help answer, this LGTM. I'll wait to accept until we figure out the answer for Linux, however.

lib/Driver/ToolChains/Solaris.cpp
208 ↗(On Diff #107769)

I've always taken that to refer to things like range-based for loop variables and functions that return iterators; not functions that return concrete types. I prefer to see the explicit type in this case so that I am able to better understand the arguments to be passed to the function. When it's abstracted behind auto, I have to work harder to find the information I need compared to when the type is explicitly spelled out.

auto changed to MultilibSet::IncludeDirsFunc.

fedor.sergeev marked 3 inline comments as done.Aug 8 2017, 7:08 AM

ugh... reverting back to llvm::Triple, since plain Triple conflicts with clang::driver::Toolchain::Triple data member.
Built/tested on Solaris11 x86/SPARC, Linux x86.

fedor.sergeev marked an inline comment as not done.Aug 8 2017, 7:43 AM
fedor.sergeev added inline comments.
lib/Driver/ToolChains/Gnu.h
253 ↗(On Diff #107769)

suggested change caused a build failure since Triple resolves into Toolchain::Triple data member, not to llvm::Triple.

aaron.ballman added inline comments.Aug 8 2017, 7:45 AM
lib/Driver/ToolChains/Gnu.h
253 ↗(On Diff #107769)

That's really good to know, thanks (and sorry for the churn)!

ro added a comment.Nov 20 2017, 1:36 AM

What's the status here? This patch is required for my WIP sanitizers-on-Solaris work.

tstellar added inline comments.Nov 20 2017, 5:27 AM
lib/Driver/ToolChains/Gnu.cpp
1840 ↗(On Diff #107769)

Yes, this can be linux only.

fedor.sergeev marked an inline comment as not done.Nov 20 2017, 2:21 PM
In D35755#930030, @ro wrote:

What's the status here? This patch is required for my WIP sanitizers-on-Solaris work.

since @tstellar just resolved the only remaining question I will go change the questionable part to linux-only and then I consider this ready to go.
Then we just have to find somebody who is willing to press the green button...
Thanks for keeping this topic alive, I kinda stopped pinging it.

In D35755#930030, @ro wrote:

What's the status here? This patch is required for my WIP sanitizers-on-Solaris work.

since @tstellar just resolved the only remaining question I will go change the questionable part to linux-only and then I consider this ready to go.
Then we just have to find somebody who is willing to press the green button...
Thanks for keeping this topic alive, I kinda stopped pinging it.

Do we still target Oracle Solaris or SmartOS? Just wondering and noted the domain change in e-mail.

ro added a comment.Nov 21 2017, 2:43 AM

Do we still target Oracle Solaris or SmartOS? Just wondering and noted the domain change in e-mail.

Both, I'd say ;-) They still have much common heritage and the vast majority of linker/toolchain work on Illumos derivatives is done by
a single guy, so the various distributions will be the same in this area. This is very much like asking wether to target OS X 10.11 or 10.13.

There will of course be some divergence, but that can be handled when the need arrises.

jyknight accepted this revision.Dec 19 2017, 2:57 PM

This looks reasonable on the face of it.

I'm assuming you know the layout for Solaris, and it doesn't seem to change the behavior of non-Solaris, so LGTM.

This revision is now accepted and ready to land.Dec 19 2017, 2:57 PM
This revision was automatically updated to reflect the committed changes.