[PATCH] [lld][inputGraph] Implement flavor specific resolver
Shankar Kalpathi Easwaran
shankarke at gmail.com
Mon Oct 7 19:26:59 PDT 2013
Will address these comments in future commits.
================
Comment at: include/lld/Core/Error.h:76
@@ +75,3 @@
+
+struct inputGraph_error {
+ enum _ {
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Rui Ueyama wrote:
> > > Does inputGraph_error follow the LLVM naming convention? It's a mix of camel case and underscore.
> > input_graph_error to match existing. We should probably switch them all over to LLVM style later.
> Will change.
I have changed this to input_graph_error.
================
Comment at: include/lld/Core/LinkingContext.h:238
@@ -238,3 +237,3 @@
/// the actual input files.
- virtual std::vector<std::unique_ptr<lld::File> > createInternalFiles();
+ virtual bool createInternalFiles(std::vector<std::unique_ptr<File> > &) const;
----------------
Rui Ueyama wrote:
> What is the meaning of the return value? If the reason to move the vector from return value to parameter is to avoid copying overhead, you could use std::move.
this was not for avoiding the move. Targets dont need to create internal files at all times. The return value is supposed to indicate that. Possibly it should be changed to ErrorOr<void>.
================
Comment at: include/lld/Core/LinkingContext.h:307
@@ +306,3 @@
+ /// no more files to be processed an appropriate inputGraph_error is returned.
+ virtual ErrorOr<File &> nextFile() const;
+
----------------
Rui Ueyama wrote:
> It shouldn't be const-qualified. Subclasses may want to update their internal state in nextFile(). PECOFFReaderContext would update its internal flags as it interprets its ".drectve" section, for example.
Context is const all over. Even ELFLinkingContext update the _currentInputElement. Its set to mutable now. The next change is going to remove const from LinkingContext, at which time all these methods will turn to non-const.
================
Comment at: include/lld/Core/LinkingContext.h:314
@@ +313,3 @@
+ /// atoms.
+ virtual void setResolverState(int32_t resolverState) const;
+
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Shankar Kalpathi Easwaran wrote:
> > > kledzik at apple.com wrote:
> > > > Rui Ueyama wrote:
> > > > > I'd use typedef'd type instead of int32_t. At least, using an unsigned type would be better.
> > > > You can eliminate the ResolveState enum and all the bit values by just making this method be:
> > > > void setResolverState(bool addedDefines, bool addedUndefs, bool addedShared, bool addedAbsolute);
> > > > though I am not sure why anything needs to now what kind of atoms was added.
> > > >
> > > I want to keep the method as is, as its convenient to add new types of atoms that are found.
> > >
> > > The reason I thought it might be useful, was
> > >
> > > a) Depending on if weak atoms were found in the process, and if the linker flag dictated to resolve weak atoms within the group.
> > > b) To derive stats of how many atoms were added/resolved from the inputelement (the weight function)
> > This seems really complicated. Isn't the main purpose of this to know if the resolver added anything? Why not just an argument to nextFile?
> The original intention is to know if the resolver added anything. This is only a secondary intention. This was discussed on the llvm dev list and we thought it would be more flexible to add a function to set the resolver state.
>
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-September/065774.html
I will change that to unsigned.
================
Comment at: include/lld/Driver/CoreInputGraph.h:47
@@ +46,3 @@
+ if (!filePath &&
+ error_code(filePath) == llvm::errc::no_such_file_or_directory)
+ return make_error_code(llvm::errc::no_such_file_or_directory);
----------------
Rui Ueyama wrote:
> Why do we handle only no_such_file_or_directory? Can't other type of error happen?
path can return only no_such_file_or_directory. I cant think of other errors.
================
Comment at: include/lld/Driver/CoreInputGraph.h:54
@@ +53,3 @@
+
+ if ((ec = llvm::MemoryBuffer::getFileOrSTDIN(*filePath, opmb)))
+ return ec;
----------------
Rui Ueyama wrote:
> I'd write it as "if (error_code ec = ...)".
ok will change.
================
Comment at: include/lld/Driver/CoreInputGraph.h:60
@@ +59,3 @@
+
+ ec = _ctx.getDefaultReader().parseFile(_buffer, _files);
+ return ec;
----------------
Rui Ueyama wrote:
> You could just return the return value.
ok.
================
Comment at: include/lld/Driver/DarwinInputGraph.h:45
@@ +44,3 @@
+ llvm::error_code parse(const LinkingContext &ctx) {
+ auto filePath = path(ctx);
+ if (!filePath &&
----------------
Rui Ueyama wrote:
> Ditto
As above.
================
Comment at: include/lld/Driver/GnuLdInputGraph.h:75
@@ +74,3 @@
+ llvm::error_code parse(const LinkingContext &ctx) {
+ auto filePath = path(ctx);
+ if (!filePath &&
----------------
Rui Ueyama wrote:
> Ditto
as above.
================
Comment at: include/lld/Driver/CoreInputGraph.h:45
@@ +44,3 @@
+ llvm::error_code parse(const LinkingContext &ctx) {
+ auto filePath = path(ctx);
+ if (!filePath &&
----------------
Rui Ueyama wrote:
> Please don't use auto for this kind of variable as other programmers can't get the type by looking at it.
I will add the type to the variable name. filePathStr.
================
Comment at: include/lld/Driver/GnuLdInputGraph.h:88
@@ +87,3 @@
+ std::unique_ptr<MemoryBuffer> mb(opmb.take());
+ _buffer = std::move(mb);
+
----------------
Rui Ueyama wrote:
> The above code is repeated in all the flavors of input graphs. These need to be consolidated.
Yes. Possibly move to a protected virtual function in FileNode.
================
Comment at: include/lld/Driver/GnuLdInputGraph.h:156
@@ +155,3 @@
+ return *_files[_nextFileIndex++];
+ }
+
----------------
Rui Ueyama wrote:
> This method seems to be a copy-n-paste of DarwinInputGraph's. This code duplication does not look good.
This should possibly move to the base implementation of getNextFile for the FileNode.
================
Comment at: include/lld/Driver/InputGraph.h:59
@@ +58,3 @@
+ END
+ };
+
----------------
Rui Ueyama wrote:
> I feel like InputGraph is overly generalized. It's generality reflected the need that Resolver have to handle input files in a platform-neutral manner, and input files can be complicated because of --{start,end}-group, -z something, etc. Now these complexities are hidden behind each flavor's getNextFile() method. That would give us a chance to reduce its complexity by representing input files using simpler data structure.
>
> This is not a review comment that needs to be resolved soon though.
It has to be a common infrastructure, that is used over all flavors. Its important to have these functions to insert input elements at any place in the graph.
================
Comment at: include/lld/Driver/InputGraph.h:90
@@ +89,3 @@
+ // \brief Does the inputGraph contain any elements
+ int64_t size() const { return _inputArgs.size(); }
+
----------------
Rui Ueyama wrote:
> Should return a value of type size_t?
Ok.
================
Comment at: include/lld/Driver/InputGraph.h:188
@@ +187,3 @@
+ int64_t _ordinal; // The ordinal value
+ int64_t _weight; // Weight of the file
+ int32_t _resolveState; // The resolve state
----------------
Rui Ueyama wrote:
> What is this filed? If it's dead we should remove.
setWeight function handles this and will be used shortly. I can discuss with you on IRC tomorrow.
================
Comment at: include/lld/Driver/InputGraph.h:330
@@ +329,3 @@
+/// \brief Represents Internal Input files
+class SimpleFileNode : public InputElement {
+public:
----------------
Rui Ueyama wrote:
> Does SimpleFileNode contains multiple files? The name is confusing.
It resembles SimpleFile. It does contain multiple files, but is usually a place holder for files that are internally added by the Linker. See Driver.
================
Comment at: include/lld/Driver/WinLinkInputGraph.h:55
@@ +54,3 @@
+ std::unique_ptr<MemoryBuffer> mb(opmb.take());
+ _buffer = std::move(mb);
+
----------------
Rui Ueyama wrote:
> Looks like it's also duplicate.
Ok.
http://llvm-reviews.chandlerc.com/D1830
More information about the llvm-commits
mailing list