This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support for parsing OUTPUT command in LinkerScript
ClosedPublic

Authored by davide on Feb 1 2015, 6:58 PM.

Diff Detail

Event Timeline

davide updated this revision to Diff 19123.Feb 1 2015, 6:58 PM
davide retitled this revision from to [ELF] Support for parsing OUTPUT command in LinkerScript.
davide updated this object.
davide edited the test plan for this revision. (Show Details)
davide added reviewers: ruiu, shankarke, rafaelauler.
davide set the repository for this revision to rL LLVM.
davide added a project: lld.
davide added a subscriber: Unknown Object (MLST).
davide added a subscriber: emaste.
davide updated this revision to Diff 19124.Feb 1 2015, 7:04 PM
davide removed rL LLVM as the repository for this revision.

Updated comment.

rafaelauler accepted this revision.Feb 1 2015, 7:10 PM
rafaelauler edited edge metadata.

LGTM.

Next time, could you please upload your diffs with "git diff -U999999 <other-branch>"? This way, we get more context here on Phabricator. Thanks for the patch.

This revision is now accepted and ready to land.Feb 1 2015, 7:10 PM
shankarke added inline comments.Feb 1 2015, 7:21 PM
include/lld/ReaderWriter/LinkerScript.h
190

I have one main comment generally with the linker script parser as to we store all the data from linker scripts as StringRef's. We dont need to keep the linker script file open IMO, so using std::string would be better.

What do you think ?

It's an option, but why dup every string that is already in a memory buffer?

shankarke accepted this revision.Feb 1 2015, 7:37 PM
shankarke edited edge metadata.

IMO, We could close the linker script after reading.

This discussion is unrelated to the patch, LGTM.

Well, I think no file is left open. After MemoryBuffer::getFileOrSTDIN(), the memory buffer is simply filled in with the file contents, and the file is closed right before this call finishes and the lexer starts its work. So, my original point was that, since we already copied the entire file to a separate memory buffer, we would be duplicating work by creating new strings and duplicating the contents of this memory buffer in several small string buffers.

ruiu edited edge metadata.Feb 1 2015, 8:35 PM

It basically looks good, but we already have a large piece of code as a part of the linker script parser which don't have any consumer. This patch adds a parser for OUTPUT() but you didn't make any change to use or interpret that directive. So, the new code doesn't actually provide value to the user by itself. It's also a little bit concerning -- there might be a design flaw that's only noticeable when we try to use the parser.

Could you update the GNU driver to consume OUTPUT directive?

davide added a comment.Feb 1 2015, 8:44 PM
In D7326#116598, @ruiu wrote:

It basically looks good, but we already have a large piece of code as a part of the linker script parser which don't have any consumer. This patch adds a parser for OUTPUT() but you didn't make any change to use or interpret that directive. So, the new code doesn't actually provide value to the user by itself. It's also a little bit concerning -- there might be a design flaw that's only noticeable when we try to use the parser.

Could you update the GNU driver to consume OUTPUT directive?

My plan was to split parsing and action, i.e. to implement the latter in a subsequent commit (was planning to do this tonight or tomorrow). If you have any concern I can incorporate the driver update in this change but I don't see a strong reason why they can't be committed as standalone changes. Let me know what you think about it.

ruiu added a comment.Feb 1 2015, 8:48 PM

Could you upload your patch to the driver to Phab, so that we can review producer side and consumer side at once? I prefer small patches too, so it's totally fine to split them up, my concern was just that I couldn't see the entire picture.

davide added a comment.Feb 1 2015, 9:54 PM

D7328 contains the other bits. Sorry for the lack of context in the diff. The server where the tree sits is currently unreachable. I'll update with a version with context tomorrow, if needed.

ruiu accepted this revision.Feb 1 2015, 10:04 PM
ruiu edited edge metadata.

LGTM

davide closed this revision.Feb 1 2015, 10:30 PM