Page MenuHomePhabricator

[Driver] Update handling of c++ and runtime directories
ClosedPublic

Authored by phosek on May 24 2019, 8:02 PM.

Details

Summary

This is a follow up to r361432 and r361504 which addresses issues
introduced by those changes. Specifically, it avoids duplicating
file and runtime paths in case when the effective triple is the
same as the cannonical one. Furthermore, it fixes the broken multilib
setup in the Fuchsia driver and deduplicates some of the code.

Diff Detail

Repository
rC Clang

Event Timeline

phosek created this revision.May 24 2019, 8:02 PM
mcgrathr accepted this revision.May 24 2019, 11:02 PM

lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.

clang/lib/Driver/Driver.cpp
4428 ↗(On Diff #201383)

I see the renaming Library->Runtime throughout (not clear why, but OK by me). This also swaps the order of getFilePaths vs getRuntimePaths, which seems significant. There's a woeful lack of comments in this function about its essential logic, which is the order in which it consults all the available sources of paths.
So changing the ordering a bit without any comments is concerning.

I'd like to see more comments overall, but as this is a preexisting problem in this code, it's enough for this change that you affirm this ordering change was intentional and add some commentary somewhere about the material change in behavior (here or in the log message).

clang/lib/Driver/ToolChain.cpp
389 ↗(On Diff #201383)

Can you explain the change to use the first entry rather than the first existing entry?
If the first one in the list is presumed to exist, then why is there a list instead of a single directory stored?

410 ↗(On Diff #201383)

Comment about the distinction between D.getTargetTriple() and Triple.str() and why this order between them.

424 ↗(On Diff #201383)

Same here. Can these use a common subroutine/template for trying something based on these two, so the logic about what D.getTargetTriple() and Triple.str() mean and both the code and justifying comments for their ordering logic is de-duplicated?

clang/test/Driver/fuchsia.c
96 ↗(On Diff #201383)

To clarify, these are dropped because their sole intent was always to serve -lc++ and that's properly not enabled for this C-only link?

This revision is now accepted and ready to land.May 24 2019, 11:02 PM
phosek updated this revision to Diff 201432.May 25 2019, 8:30 PM
phosek marked 8 inline comments as done.

lgtm contingent on verifying intended behavior changes and adding comments. The factoring suggestion for the triple,triple searching is preferred but at your discretion, though I'd really like to see at least comments.

I have undone that factoring as I have realized I could simplify the logic further, but as that's going to require more cleanup I'll do it in a follow up change.

clang/lib/Driver/ToolChain.cpp
389 ↗(On Diff #201383)

That was unintentional, I've reverted that change.

424 ↗(On Diff #201383)

I plan on cleaning up the runtime handling in a follow up change.

clang/test/Driver/fuchsia.c
96 ↗(On Diff #201383)

Correct.

This revision was automatically updated to reflect the committed changes.