Page MenuHomePhabricator

[Shave]: allow Clang to run the target linker.
AbandonedPublic

Authored by dougk on Jun 30 2015, 9:16 AM.

Details

Summary

Implement the ShaveToolChain::buildLinker() method instead of calling llvm_unreachable.
Lightly tested, but works ok on a few of the sample applications.

Diff Detail

Event Timeline

dougk updated this revision to Diff 28793.Jun 30 2015, 9:16 AM
dougk retitled this revision from to [Shave]: allow Clang to run the target linker..
dougk updated this object.
dougk edited the test plan for this revision. (Show Details)
dougk added reviewers: chandlerc, jyknight.
dougk added a subscriber: Unknown Object (MLST).
jyknight added inline comments.Jul 1 2015, 8:22 AM
lib/Driver/Tools.cpp
9110

Perhaps this should be a default plus a command-line argument, instead of an environment variable?

9144

Doesn't seem right to hardcode this particular version. Using a default for ToolsRoot if MV_TOOLS_DIR isn't set sounds okay, but presumably should be done up above before the loop.

9160

"-O9"? That seems weird, and is not actually a thing.

9198

These (oneleon/bothleons) don't seem to do anything different than "rtems", so I'd remove them

9221

Why are these building up explicit paths, instead of adding the proper -L search paths, and then -lc -lssp etc?

dougk abandoned this revision.Sep 18 2015, 5:00 AM
dougk added inline comments.
lib/Driver/Tools.cpp
9110

How about if '--sysdir' provides the value, falling back to the environment var, and then a fixed default?
There is ample precedent for use of getenv() in some of the other toolchains.

9144

I could see a couple ways around hardcoding a version - if neither --sysroot nor MV_TOOLS_DIR is used, then the path is arbitrary anyway, so doesn't need a versioned subdir. And arguably is sysroot is given, then it could be an already-versioned pathname. Otoh, detecting the version works fine in that case.

For reference, the example sub-Makefile has hardcoded:
MV_TOOLS_VERSION ?= 00.50.62.5
MV_TOOLS_DIR ?= $(HOME)/WORK/Tools/Releases/General

9160

This was cargo-cultism. I've no objection to taking it out, but who's to say that their linker doesn't use it for something, as why else would they bother to put it in the examples?