From: Rui Ueyama Date: Wed, 2 Apr 2014 21:02:44 +0000 (+0000) Subject: Simplify communication between Resolver and Input Graph. X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=edd5c0a9eaccb0a5e878d5b8ad9ec54e2a0bbb6c;p=platform%2Fupstream%2Fllvm.git Simplify communication between Resolver and Input Graph. Resolver is sending too much information to Input Graph than Input Graph actually needs. In order to collect the detailed information, which wouldn't be consumed by anyone, we have a good amount of code in Resolver, Input Graph and Input Elements. This patch is to simplify it. No functionality change. Specifically, this patch replaces ResolverState enum with a boolean value. The enum defines many bits to notify the progress about linking to Input Graph using bit masks, however, what Input Graph actually does is to compare a given value with 0. The details of the bit mask is simply being ignored, so the efforts to collect such data is wasted. This patch also changes the name of the notification interface from setResolverState to notifyProgress, to make it sounds more like message passing style. It's not a setter but something to notify of an update, so the new name should be more appropriate than before. Differential Revision: http://llvm-reviews.chandlerc.com/D3267 llvm-svn: 205463 --- diff --git a/lld/include/lld/Core/InputGraph.h b/lld/include/lld/Core/InputGraph.h index a50ae11..2cd0c82 100644 --- a/lld/include/lld/Core/InputGraph.h +++ b/lld/include/lld/Core/InputGraph.h @@ -62,12 +62,11 @@ public: /// resolved. ErrorOr nextFile(); - /// Set the resolver state for the current Input element. This is used by the - /// InputGraph to decide the next file that needs to be processed for various - /// types of nodes in the InputGraph. The resolver state is nothing but a - /// bitmask of various types of states that the resolver handles when adding - /// atoms. - void setResolverState(uint32_t resolverState); + /// Notifies the current input element of Resolver made some progress on + /// resolving undefined symbols using the current file. Group (representing + /// --start-group and --end-group) uses that notification to make a decision + /// whether it should iterate over again or terminate or not. + void notifyProgress(); /// \brief Adds a node into the InputGraph bool addInputElement(std::unique_ptr); @@ -153,11 +152,9 @@ public: /// Get the next file to be processed by the resolver virtual ErrorOr getNextFile() = 0; - /// \brief Set the resolve state for the element - virtual void setResolveState(uint32_t state) = 0; - - /// \brief Get the resolve state for the element - virtual uint32_t getResolveState() const = 0; + /// Refer InputGraph::notifyProgress(). By default, it does nothing. Only + /// Group is interested in this message. + virtual void notifyProgress() {}; /// \brief Reset the next index virtual void resetNextIndex() = 0; @@ -183,7 +180,7 @@ class Group : public InputElement { public: Group(int64_t ordinal) : InputElement(InputElement::Kind::Group, ordinal), - _currentElementIndex(0), _nextElementIndex(0) {} + _currentElementIndex(0), _nextElementIndex(0), _madeProgress(false) {} static inline bool classof(const InputElement *a) { return a->kind() == InputElement::Kind::Group; @@ -200,7 +197,9 @@ public: } void resetNextIndex() override { - _currentElementIndex = _nextElementIndex = 0; + _madeProgress = false; + _currentElementIndex = 0; + _nextElementIndex = 0; for (auto &elem : _elements) elem->resetNextIndex(); } @@ -213,14 +212,21 @@ public: return error_code::success(); } - uint32_t getResolveState() const override; - void setResolveState(uint32_t) override; + /// If Resolver made a progress using the current file, it's ok to revisit + /// files in this group in future. + void notifyProgress() override { + for (auto &elem : _elements) + elem->notifyProgress(); + _madeProgress = true; + } + ErrorOr getNextFile() override; protected: InputGraph::InputElementVectorT _elements; uint32_t _currentElementIndex; uint32_t _nextElementIndex; + bool _madeProgress; }; /// \brief Represents an Input file in the graph @@ -267,15 +273,7 @@ public: /// \brief Reset the file index if the resolver needs to process /// the node again. - void resetNextIndex() override; - - /// \brief Set the resolve state for the FileNode. - void setResolveState(uint32_t resolveState) override { - _resolveState = resolveState; - } - - /// \brief Retrieve the resolve state of the FileNode. - uint32_t getResolveState() const override { return _resolveState; } + void resetNextIndex() override { _nextFileIndex = 0; } protected: /// \brief Read the file into _buffer. diff --git a/lld/include/lld/Core/Resolver.h b/lld/include/lld/Core/Resolver.h index ebd9eae..109dfd4 100644 --- a/lld/include/lld/Core/Resolver.h +++ b/lld/include/lld/Core/Resolver.h @@ -28,14 +28,6 @@ class LinkingContext; /// and producing a merged graph. class Resolver { public: - enum ResolverState { - StateNoChange = 0, // The default resolver state - StateNewDefinedAtoms = 1, // New defined atoms were added - StateNewUndefinedAtoms = 2, // New undefined atoms were added - StateNewSharedLibraryAtoms = 4, // New shared library atoms were added - StateNewAbsoluteAtoms = 8 // New absolute atoms were added - }; - Resolver(LinkingContext &context) : _context(context), _symbolTable(context), _result(new MergedFile()) {} diff --git a/lld/include/lld/Driver/GnuLdInputGraph.h b/lld/include/lld/Driver/GnuLdInputGraph.h index c10edda..f689a45 100644 --- a/lld/include/lld/Driver/GnuLdInputGraph.h +++ b/lld/include/lld/Driver/GnuLdInputGraph.h @@ -89,7 +89,6 @@ public: (_files[0]->kind() == File::kindSharedLibrary)) { _nextFileIndex = 0; } - setResolveState(Resolver::StateNoChange); } /// \brief Return the file that has to be processed by the resolver @@ -146,14 +145,6 @@ public: return make_range(_expandElements.begin(), _expandElements.end()); } - void setResolveState(uint32_t) override { - llvm_unreachable("cannot use this function: setResolveState"); - } - - uint32_t getResolveState() const override { - llvm_unreachable("cannot use this function: getResolveState"); - } - // Do nothing here. void resetNextIndex() override {} diff --git a/lld/lib/Core/InputGraph.cpp b/lld/lib/Core/InputGraph.cpp index 7d85021..1c713dd 100644 --- a/lld/lib/Core/InputGraph.cpp +++ b/lld/lib/Core/InputGraph.cpp @@ -47,9 +47,7 @@ ErrorOr InputGraph::nextFile() { } } -void InputGraph::setResolverState(uint32_t state) { - _currentInputElement->setResolveState(state); -} +void InputGraph::notifyProgress() { _currentInputElement->notifyProgress(); } bool InputGraph::addInputElement(std::unique_ptr ie) { _inputArgs.push_back(std::move(ie)); @@ -123,7 +121,7 @@ InputElement::InputElement(Kind type, int64_t ordinal) /// FileNode FileNode::FileNode(StringRef path, int64_t ordinal) : InputElement(InputElement::Kind::File, ordinal), _path(path), - _resolveState(Resolver::StateNoChange), _nextFileIndex(0) {} + _nextFileIndex(0) {} /// \brief Read the file into _buffer. error_code FileNode::getBuffer(StringRef filePath) { @@ -135,33 +133,6 @@ error_code FileNode::getBuffer(StringRef filePath) { return error_code::success(); } -// Reset the next file that would be be processed by the resolver. -// Reset the resolve state too. -void FileNode::resetNextIndex() { - _nextFileIndex = 0; - setResolveState(Resolver::StateNoChange); -} - -/// ControlNode - -/// \brief Get the resolver State. The return value of the resolve -/// state for a control node is the or'ed value of the resolve states -/// contained in it. -uint32_t Group::getResolveState() const { - uint32_t resolveState = Resolver::StateNoChange; - for (auto &elem : _elements) - resolveState |= elem->getResolveState(); - return resolveState; -} - -/// \brief Set the resolve state for the current element -/// thats processed by the resolver. -void Group::setResolveState(uint32_t resolveState) { - if (_elements.empty()) - return; - _elements[_currentElementIndex]->setResolveState(resolveState); -} - /// Group /// \brief Return the next file that need to be processed by the resolver. @@ -173,14 +144,15 @@ ErrorOr Group::getNextFile() { return make_error_code(InputGraphError::no_more_files); for (;;) { - // If we have processed all the elements as part of this node - // check the resolver status for each input element and if the status - // has not changed, move onto the next file. + // If we have processed all the elements, and have made no progress on + // linking, we cannot resolve any symbol from this group. Continue to the + // next one by returning no_more_files. if (_nextElementIndex == _elements.size()) { - if (getResolveState() == Resolver::StateNoChange) + if (!_madeProgress) return make_error_code(InputGraphError::no_more_files); resetNextIndex(); } + _currentElementIndex = _nextElementIndex; auto file = _elements[_nextElementIndex]->getNextFile(); // Move on to the next element if we have finished processing all diff --git a/lld/lib/Core/Resolver.cpp b/lld/lib/Core/Resolver.cpp index 7f131e8..8716036 100644 --- a/lld/lib/Core/Resolver.cpp +++ b/lld/lib/Core/Resolver.cpp @@ -69,19 +69,19 @@ private: } // namespace void Resolver::handleFile(const File &file) { - uint32_t resolverState = Resolver::StateNoChange; const SharedLibraryFile *sharedLibraryFile = dyn_cast(&file); - for (const DefinedAtom *atom : file.defined()) { + for (const DefinedAtom *atom : file.defined()) doDefinedAtom(*atom); - resolverState |= StateNewDefinedAtoms; - } + bool progress = false; + if (!sharedLibraryFile || _context.addUndefinedAtomsFromSharedLibrary(sharedLibraryFile)) { + progress = (file.undefined().size() > 0); + for (const UndefinedAtom *undefAtom : file.undefined()) { doUndefinedAtom(*undefAtom); - resolverState |= StateNewUndefinedAtoms; // If the undefined symbol has an alternative name, try to resolve the // symbol with the name to give it a second chance. This feature is used // for COFF "weak external" symbol. @@ -93,15 +93,19 @@ void Resolver::handleFile(const File &file) { } } } - for (const SharedLibraryAtom *shlibAtom : file.sharedLibrary()) { + for (const SharedLibraryAtom *shlibAtom : file.sharedLibrary()) doSharedLibraryAtom(*shlibAtom); - resolverState |= StateNewSharedLibraryAtoms; - } - for (const AbsoluteAtom *absAtom : file.absolute()) { + + for (const AbsoluteAtom *absAtom : file.absolute()) doAbsoluteAtom(*absAtom); - resolverState |= StateNewAbsoluteAtoms; + + // If we make some progress on linking, notify that fact to the input file + // manager, because it may want to know that for --start-group/end-group. + progress = progress || file.sharedLibrary().size() || + file.absolute().size() || file.defined().size(); + if (progress) { + _context.inputGraph().notifyProgress(); } - _context.inputGraph().setResolverState(resolverState); } void Resolver::forEachUndefines(UndefCallback callback, @@ -302,7 +306,6 @@ bool Resolver::resolveUndefines() { for (;;) { ErrorOr file = _context.inputGraph().nextFile(); - _context.inputGraph().setResolverState(Resolver::StateNoChange); error_code ec = file.getError(); if (ec == InputGraphError::no_more_files) return true; diff --git a/lld/unittests/DriverTests/InputGraphTest.cpp b/lld/unittests/DriverTests/InputGraphTest.cpp index b119cf5..a6bb3bc 100644 --- a/lld/unittests/DriverTests/InputGraphTest.cpp +++ b/lld/unittests/DriverTests/InputGraphTest.cpp @@ -315,7 +315,7 @@ TEST_F(InputGraphTest, AddNodeWithGroupIteration) { EXPECT_NE(InputGraphError::no_more_files, objfile.getError()); EXPECT_EQ("group_objfile2", (*objfile).path()); - group->setResolveState(Resolver::StateNewDefinedAtoms); + group->notifyProgress(); objfile = group->getNextFile(); EXPECT_NE(InputGraphError::no_more_files, objfile.getError());