[nfc] Factoring out utility that can be used for other object-level tools

[JestrTulip]

Summary

Related rfc can be found at https://discourse.llvm.org/t/rfc-llvm-cm-cost-model-evaluation-for-object-files-machine-code/71502. We want to reuse the instruction iterator for this tool.

[mtrofin]

lgtm (some comments)

llvm/include/llvm/Object/ObjectFile.h

49

Since this moves now in the llvm namespace, perhaps renaming it to something less generic, like SectionFilterPredicate? By the looks of it, it's only used in the header anyway.

Or move it as a member of SectionFilterIterator

[MaskRay]

Can you state the motivation and link to the follow-up patch?

[JestrTulip]

feedback

[JestrTulip]

In D152869#4419733, @MaskRay wrote:

Can you state the motivation and link to the follow-up patch?

The main motivation behind this (and upcoming) changes is to move towards developing a tool that can intake object files and use them as part of a greater cost model evaluation tool (RFC is a WIP for this tool) . As for this current patch, this works on making the file intake and processing step easier overall. Since it's a refactoring and doesn't affect any current implementations, I saw it as not tied to the upcoming, broader changes that will be outlined in the RFC.

[jhenderson]

As there is currently only one usage, I don't think a moving of the code benefits anything (you wouldn't do the refactoring if you weren't looking to use it elsewhere, for example). I'm not opposed to the patch being part of a patch series that includes the proposed RFC, but it shouldn't land unless the RFC is accepted and the corresponding patch is still using it once the final version is accepted.

[JestrTulip]

In D152869#4423821, @jhenderson wrote:

As there is currently only one usage, I don't think a moving of the code benefits anything (you wouldn't do the refactoring if you weren't looking to use it elsewhere, for example). I'm not opposed to the patch being part of a patch series that includes the proposed RFC, but it shouldn't land unless the RFC is accepted and the corresponding patch is still using it once the final version is accepted.

Added the child revision. The RFC is pending review, as it is my initial post. Will update message with link shortly.

[JestrTulip]

In D152869#4419733, @MaskRay wrote:

Can you state the motivation and link to the follow-up patch?

Added the rfc and intro patch, is there anything else I should do to for this to get accepted?

[MaskRay]

In D152869#4436188, @JestrTulip wrote:

In D152869#4419733, @MaskRay wrote:

Can you state the motivation and link to the follow-up patch?

Added the rfc and intro patch, is there anything else I should do to for this to get accepted?

Yes. I feel the same with @jhenderson .

it shouldn't land unless the RFC is accepted and the corresponding patch is still using it once the final version is accepted.

I am happy to be more lenient to "once the final version is accepted". I am happy once "the patch will clearly land (approved or will eventually be) and the final version or the agreed direction will use the shared code when it lands."
So, sorry, I don't think the patch can land now.

[jhenderson]

Thanks @JestrTulip, this gets a soft LGTM from me, but as noted, the other patch needs accepting before this lands.

llvm/include/llvm/Object/ObjectFile.h

408

Since you're moving this code into the llvm::object namespace, I believe all these llvm::object:: prefixes can disappear. Please do that.

It's also a good chance to tidy up some of the style issues. In particular, LLVM code prefers "west const" (i.e. the const before the type name).

These points apply thoughour the moved code.

[JestrTulip]

removing llvm::object prefixes

[jhenderson]

llvm/include/llvm/Object/ObjectFile.h

408

It's also a good chance to tidy up some of the style issues. In particular, LLVM code prefers "west const" (i.e. the const before the type name).

Please do this too (e.g. const section_iterator & rather than the current section_iterator const &). Also applies in a couple of places in this code.

[JestrTulip]

feedback: west const for non-const functions

[JestrTulip]

feedback: const changes in objdump.h

[jhenderson]

@JestrTulip, it looks like your latest diff isn't the full patch, but rather one of your fixup commits? Please make sure to upload the full diff instead.

[JestrTulip]

resolved patch problem

[JestrTulip]

Hi All, the accompanying patch to this nfc has been accepted. Could we also proceed with accepting this one?

[jhenderson]

LGTM, but please wait for @MaskRay (NB, I've put a swathe of comments on your other patch).

llvm/include/llvm/Object/ObjectFile.h

 class ObjectFile;
 class SectionRef;
 class SymbolRef;
 class symbol_iterator;
 class WasmObjectFile;
 
 using section_iterator = content_iterator<SectionRef>;
 
+typedef std::function<bool(llvm::object::SectionRef const &)> FilterPredicate;
 /// This is a value type class that represents a single relocation in the list
 /// of relocations in the object file.
 class RelocationRef {
   DataRefImpl RelocationPimpl;
   const ObjectFile *OwningObject = nullptr;
 
 public:
   RelocationRef() = default;
public:
 
   static Expected<std::unique_ptr<ObjectFile>>
   createGOFFObjectFile(MemoryBufferRef Object);
 
   static Expected<std::unique_ptr<WasmObjectFile>>
   createWasmObjectFile(MemoryBufferRef Object);
 };
 
+/// A filtered iterator for SectionRefs that skips sections based on some given
+/// predicate.
+class SectionFilterIterator {
+public:
+  SectionFilterIterator(FilterPredicate Pred,
+                        llvm::object::section_iterator const &Begin,
+                        llvm::object::section_iterator const &End)
+      : Predicate(std::move(Pred)), Iterator(Begin), End(End) {
+    scanPredicate();
+  }
+  const llvm::object::SectionRef &operator*() const { return *Iterator; }
+  SectionFilterIterator &operator++() {
+    ++Iterator;
+    scanPredicate();
+    return *this;
+  }
+  bool operator!=(SectionFilterIterator const &Other) const {
+    return Iterator != Other.Iterator;
+  }
+
+private:
+  void scanPredicate() {
+    while (Iterator != End && !Predicate(*Iterator)) {
+      ++Iterator;
+    }
+  }
+  FilterPredicate Predicate;
+  llvm::object::section_iterator Iterator;
+  llvm::object::section_iterator End;
+};
+
+/// Creates an iterator range of SectionFilterIterators for a given Object and
+/// predicate.
+class SectionFilter {
+public:
+  SectionFilter(FilterPredicate Pred, llvm::object::ObjectFile const &Obj)
+      : Predicate(std::move(Pred)), Object(Obj) {}
+  SectionFilterIterator begin() {
+    return SectionFilterIterator(Predicate, Object.section_begin(),
+                                 Object.section_end());
+  }
+  SectionFilterIterator end() {
+    return SectionFilterIterator(Predicate, Object.section_end(),
+                                 Object.section_end());
+  }
+
+private:
+  FilterPredicate Predicate;
+  llvm::object::ObjectFile const &Object;
+};
+
 // Inline function definitions.
 inline SymbolRef::SymbolRef(DataRefImpl SymbolP, const ObjectFile *Owner)
     : BasicSymbolRef(SymbolP, Owner) {}
 
 inline Expected<StringRef> SymbolRef::getName() const {
   return getObject()->getSymbolName(getRawDataRefImpl());
 }
 

llvm/tools/llvm-objdump/llvm-objdump.h

 //===--- llvm-objdump.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 #define LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 
 #include "llvm/ADT/StringSet.h"
 #include "llvm/DebugInfo/DIContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/Object/Archive.h"
+#include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataTypes.h"
 
 namespace llvm {
 class StringRef;
 class Twine;
 
 namespace opt {

 extern bool ShowRawInsn;
 extern bool SymbolDescription;
 extern bool SymbolTable;
 extern std::string TripleName;
 extern bool UnwindInfo;
 
 extern StringSet<> FoundSectionSet;
 
-typedef std::function<bool(llvm::object::SectionRef const &)> FilterPredicate;
-
-/// A filtered iterator for SectionRefs that skips sections based on some given
-/// predicate.
-class SectionFilterIterator {
-public:
-  SectionFilterIterator(FilterPredicate P,
-                        llvm::object::section_iterator const &I,
-                        llvm::object::section_iterator const &E)
-      : Predicate(std::move(P)), Iterator(I), End(E) {
-    ScanPredicate();
-  }
-  const llvm::object::SectionRef &operator*() const { return *Iterator; }
-  SectionFilterIterator &operator++() {
-    ++Iterator;
-    ScanPredicate();
-    return *this;
-  }
-  bool operator!=(SectionFilterIterator const &Other) const {
-    return Iterator != Other.Iterator;
-  }
-
-private:
-  void ScanPredicate() {
-    while (Iterator != End && !Predicate(*Iterator)) {
-      ++Iterator;
-    }
-  }
-  FilterPredicate Predicate;
-  llvm::object::section_iterator Iterator;
-  llvm::object::section_iterator End;
-};
-
-/// Creates an iterator range of SectionFilterIterators for a given Object and
-/// predicate.
-class SectionFilter {
-public:
-  SectionFilter(FilterPredicate P, llvm::object::ObjectFile const &O)
-      : Predicate(std::move(P)), Object(O) {}
-  SectionFilterIterator begin() {
-    return SectionFilterIterator(Predicate, Object.section_begin(),
-                                 Object.section_end());
-  }
-  SectionFilterIterator end() {
-    return SectionFilterIterator(Predicate, Object.section_end(),
-                                 Object.section_end());
-  }
-
-private:
-  FilterPredicate Predicate;
-  llvm::object::ObjectFile const &Object;
-};
-
 // Various helper functions.
 
 /// Creates a SectionFilter with a standard predicate that conditionally skips
 /// sections when the --section objdump flag is provided.
 ///
 /// Idx is an optional output parameter that keeps track of which section index
 /// this is. This may be different than the actual section number, as some
 /// sections may be filtered (e.g. symbol tables).
-SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,
+object::SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,
                                         uint64_t *Idx = nullptr);
 
 bool isRelocAddressLess(object::RelocationRef A, object::RelocationRef B);
 void printRelocations(const object::ObjectFile *O);
 void printDynamicRelocations(const object::ObjectFile *O);
 void printSectionHeaders(object::ObjectFile &O);
 void printSectionContents(const object::ObjectFile *O);
 void printSymbolTable(const object::ObjectFile &O, StringRef ArchiveName,
                       StringRef ArchitectureName = StringRef(),