Page MenuHomePhabricator

Convert other drivers to use WrapperNode.
ClosedPublic

Authored by ruiu on Jan 7 2015, 5:23 PM.

Details

Summary

Convert other drivers to use WrapperNode. I think this is the last
complex patch in the series of refactoring patches, all the following
ones are in my opinion straightforward. You can see the final form
of the code at https://github.com/rui314/lld/commits/parse

Diff Detail

Event Timeline

ruiu updated this revision to Diff 17882.Jan 7 2015, 5:23 PM
ruiu retitled this revision from to Convert other drivers to use WrapperNode..
ruiu updated this object.
ruiu edited the test plan for this revision. (Show Details)
ruiu set the repository for this revision to rL LLVM.
ruiu added a project: lld.
ruiu added a subscriber: Unknown Object (MLST).
Bigcheese edited edge metadata.Jan 7 2015, 6:58 PM

The gnuld parts look good to me.

kledzik edited edge metadata.Jan 7 2015, 7:29 PM

This seems to have lost the parallel reading/parsing of input files. It looks like parseFile() called by the driver is triggering the actual parsing and parse() called by the Driver in parallel just returns the error code from the parse.

shankarke added inline comments.Jan 7 2015, 8:06 PM
include/lld/ReaderWriter/ELFLinkingContext.h
293–313

How is the attribute accessed from the file ? I didnt see a accessor function, am I missing some detail ?

lib/Driver/GnuLdDriver.cpp
261–282

how is attributes going to be passed ?

ruiu updated this revision to Diff 17903.Jan 8 2015, 10:10 AM
ruiu edited edge metadata.
ruiu removed rL LLVM as the repository for this revision.
  • Do not trigger actual file parsing when parseFile is called
include/lld/ReaderWriter/ELFLinkingContext.h
293–313

Well, that would be a question for you. :) The code was incomplete. I moved this piece of code from other file to this file. It's not new. Apparently we need more code to support these attributes but that's out of scope of this patch.

lib/Driver/GnuLdDriver.cpp
261–282

Attributes are not passed at this moment at least, and that's not actually a new behavior. ELFFileNode didn't use that attribute (other than printing a debug message). Maybe we should remove that. I'd think that doesn't have to be done in this patch.

It still looks like mach-o files are parsed immediately (not in parallel). If that is intentional, because reworking the mach-o logic is complicated, then leave a FIXME comment there.

It would be helpful to rename parseFile() if its goal is no longer to parse files. I seems like its goal is to load the file into memory, and later parse()/doParse() are supposed to do the actual parsing.

ruiu added a comment.Jan 12 2015, 5:06 PM

It's not intentional and I believe it's not actually parsed immediately.
Can you elaborate a bit why you think files would be parsed not in parallel?

As to the name, I totally agree with that. parseFile is no longer a good
name for that function. I'll rename loadFile (or readFile, but read could
imply that we are going to parse it as part of reading, so I'm inclined to
"load").

I see. Given my comment in the previous patch about parallelism, I was expecting to see a change in the latest patch about when parsing happens. But you changed mach-o parseFile() a while ago to instantiate a File object, but not parse it until doParse() was invoked. So all is good. Just the parseFile() name confusing me.

ruiu added a comment.Jan 12 2015, 8:37 PM

Thanks. I renamed loadFile and submitted as r225764.

Jean-Daniel accepted this revision.Mar 4 2015, 12:08 PM
Jean-Daniel edited edge metadata.
This revision is now accepted and ready to land.Mar 4 2015, 12:08 PM
Jean-Daniel closed this revision.Mar 4 2015, 12:08 PM