Page MenuHomePhabricator

Implement semantic action for SEARCH_DIR linker script command
ClosedPublic

Authored by davide on Jan 27 2015, 6:52 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 18802.Jan 27 2015, 6:52 PM
davide retitled this revision from to Implement semantic action for SEARCH_DIR linker script command.
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: ruiu, rafaelauler.
davide added a project: lld.
davide added subscribers: emaste, Unknown Object (MLST).
emaste added inline comments.Jan 28 2015, 6:47 AM
lib/Driver/GnuLdDriver.cpp
293–295 ↗(On Diff #18802)

To me this comment implies we recognize a subset of GROUP() and SEARCH_DIR().

Maybe Currently we only recognize this subset of linker script commands:

ruiu edited edge metadata.Jan 28 2015, 10:41 AM

I just committed r227341 to test the existing linker script functionality. Please add a test in the same way as my commit for this patch too.

lib/Driver/GnuLdDriver.cpp
276 ↗(On Diff #18802)

This function will never fail and it's too simple to be a separate function -- it's just one line. I'd write it in-line.

davide updated this revision to Diff 18937.Jan 28 2015, 7:31 PM
davide edited edge metadata.

Attempt to write an unit test and address Ed's and Rui's comments.

emaste added inline comments.Jan 28 2015, 7:35 PM
lib/Driver/GnuLdDriver.cpp
290 ↗(On Diff #18937)

phabricator often has trouble rendering whitespace properly, but please double-check tabs vs spaces here

ruiu accepted this revision.Jan 29 2015, 12:20 PM
ruiu edited edge metadata.

LGTM.

include/lld/ReaderWriter/ELFLinkingContext.h
272 ↗(On Diff #18937)

Don't make a function virtual if not needed.

unittests/DriverTests/GnuLdDriverTest.cpp
184 ↗(On Diff #18937)

I think there's code in the GNU driver to add /usr/lib to the default search path, so this test may pass even if the linker script parser doesn't work. I'd make it something unrealistic, like "/foo/bar", to avoid possible conflict.

This revision is now accepted and ready to land.Jan 29 2015, 12:20 PM
This revision was automatically updated to reflect the committed changes.