[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

409

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

409

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(const SectionRef &)> SectionFilterPredicate;
+typedef std::function<bool(const SectionRef &)> SectionFilterPredicate;
 /// 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:
   createWasmObjectFile(MemoryBufferRef Object);
 };
 
 /// A filtered iterator for SectionRefs that skips sections based on some given
 /// predicate.
 class SectionFilterIterator {
 public:
   SectionFilterIterator(SectionFilterPredicate Pred,
                         const section_iterator &Begin,
                         const section_iterator &End)
       : Predicate(std::move(Pred)), Iterator(Begin), End(End) {
     scanPredicate();
   }
   const SectionRef &operator*() const { return *Iterator; }
   SectionFilterIterator &operator++() {
     ++Iterator;
     scanPredicate();

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

 // 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).
-object::SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O,
+object::SectionFilter ToolSectionFilter(const llvm::object::ObjectFile &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,