[llvm-objdump] Use a counter for llvm-objdump -h instead of the section index.

[rupprecht]

Summary

When listing the index in llvm-objdump -h, use a zero-based counter instead of the actual section index (e.g. shdr->sh_index for ELF).

While this is effectively a noop for now (except one unit test for XCOFF), the index values will change in a future patch that filters certain sections out (e.g. symbol tables). See D68669 for more context. Note: the test case in test/tools/llvm-objdump/X86/section-index.s already covers the case of incrementing the section index counter when sections are skipped.

[rupprecht]

Rebase against D68730

[grimar]

A few ideas/suggestions.

llvm/tools/llvm-objdump/llvm-objdump.cpp

345–358

It's common to wrap such things (helper types) into anonymous namespaces:
I think you can also add a helper function too:

namespace {
struct FilterResult {
  bool Keep;
  bool IncrementIndex;
};

FilterResult checkSectionFilter(object::SectionRef S) {
...
}
};

347

I'd comment these fields. It wasn't obvious to me what they are used for.

360

I think we use a full variable name usually, i.e. Increment -> IncrementIndex

383

nit: if (Idx) would be shorer.

387

Please avoid using auto for return types that are not obvious.

1708

Looking at this,
should ToolSectionFilter just return [(SectionRef&)Ref, (uint64_t )Index] struct/pair instead? It seems could make the whole logic simper.

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

90

I think this needs a full comment (it does not give an information about what this helper avtually do atm):

I.e. would be nice to see something like:

// Function is used to ...
// Idx is a optional output parameter that ...

[rupprecht]

• Add/update several comments
• Use simplified pointer check
• Avoid auto as the returned type is not obvious

[rupprecht]

Rebase against D68730 again

[rupprecht]

llvm/tools/llvm-objdump/llvm-objdump.cpp

345–358

Wrapped just the struct per http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

360

Sorry, I originally had the variable named Increment but forgot to update these comments.

1708

That's one of the paths I considered while writing this patch. A couple thoughts:

1. This is the only place that needs this counter value, and loop iteration in every other case (7 other places in this file, 1 in the MachO dumper) would now be a little more complicated even when callers don't care about the index, e.g. it would now be:

for (const SomeWrapperType &Foo : ToolSectionFilter(*Obj)) {
  const SectionRef &Section = Foo.Section;

2. llvm has make_filter_range which probably did not exist at the time this code was first written, and it would be nice to completely remove SectionFilter and SectionFilterIterator from llvm-objdump.h in favor of those standard llvm libraries, but AIUI in order to use that, the return type would need to be SectionRef, not some wrapper type. (I'm trying to do that in a separate branch, but I'm dealing with template woes).
3. But on the plus side, it does avoid out parameters which would be very nice.

I think 1&2 outweigh 3, so I'm leaning towards this approach. But I'm still exploring alternatives.

[grimar]

llvm/tools/llvm-objdump/llvm-objdump.cpp

376

Can we have a test for this logic? I.e. for a case when Keep=false, IncrementIndex=true.
(Doesn't seem we have it)

1708

I see. I have no much better/different ideas atm. Perhaps the current approach is OK for now.

[rupprecht]

llvm/tools/llvm-objdump/llvm-objdump.cpp

376

This is already covered by llvm/test/tools/llvm-objdump/X86/section-index.s:

# RUN: llvm-objdump -section-headers %t | FileCheck %s
...
# CHECK-NEXT: 4  .bar

# RUN: llvm-objdump -section-headers -section=.bar %t \
# RUN:   | FileCheck %s --check-prefix=BAR
...
# BAR-NEXT:  4  .bar

I'll mention this in the patch description.

[grimar]

LGTM

llvm/tools/llvm-objdump/llvm-objdump.cpp

376

Ah, OK.

[rupprecht]

Thanks for the review!

llvm/test/tools/llvm-objdump/xcoff-section-headers.test

 # RUN: llvm-objdump --section-headers %p/Inputs/xcoff-section-headers.o | \
 # RUN: FileCheck %s
 
 # RUN: llvm-objdump --section-headers %p/Inputs/xcoff-long-sec-names.o | \
 # RUN: FileCheck --check-prefix=LONG %s
 
 # RUN: not llvm-objdump --section-headers 2>&1 \
 # RUN: %p/Inputs/xcoff-section-headers-truncate.o | FileCheck \
 # RUN: --check-prefix=ERROR %s
 
 # ERROR: The end of the file was unexpectedly encountered
 
 # CHECK: xcoff-section-headers.o:      file format aixcoff-rs6000
 # CHECK: Sections:
 # CHECK: Idx Name          Size     VMA      Type
-# CHECK:   1 .text         00000080 00000000 TEXT
-# CHECK:   2 .data         00000024 00000080 DATA
-# CHECK:   3 .bss          00000004 000000a4 BSS
-# CHECK:   4 .tdata        00000008 00000000 DATA
-# CHECK:   5 .tbss         00000004 00000008 BSS
+# CHECK:   0 .text         00000080 00000000 TEXT
+# CHECK:   1 .data         00000024 00000080 DATA
+# CHECK:   2 .bss          00000004 000000a4 BSS
+# CHECK:   3 .tdata        00000008 00000000 DATA
+# CHECK:   4 .tbss         00000004 00000008 BSS
 
 # xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 # test.c:
 # int a;
 # int b = 12345;
 # __thread int c;
 # __thread double d = 3.14159;
 #
 # int func(void)  {
 #   return a;
 # }
 
 # LONG: xcoff-long-sec-names.o:      file format aixcoff-rs6000
 # LONG: Sections:
-# LONG: Idx Name        Size     VMA      Type
-# LONG: 1 .dwarnge      00000004 00000000
-# LONG: 2 .dwpbnms      00000004 00000000
-# LONG: 3 .dwpbtyp      00000004 00000000
+# LONG: Idx Name          Size     VMA      Type
+# LONG:   0 .dwarnge      00000004 00000000
+# LONG:   1 .dwpbnms      00000004 00000000
+# LONG:   2 .dwpbtyp      00000004 00000000
 
 # xcoff-long-sec-names.o was generated by assembling the following .s file:
 #  .dwsect 0x30000 # .dwpbnms section
 #  .dwsect 0x40000 # .dwpbtyp section
 #  .dwsect 0x50000 # .dwarnge section

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

 class MachOUniversalBinary;
 class RelocationRef;
 }
 
 extern cl::opt<bool> Demangle;
 
 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();
   }
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.
-SectionFilter ToolSectionFilter(llvm::object::ObjectFile const &O);
+
+/// 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,
+                                uint64_t *Idx = nullptr);
 
 Error getELFRelocationValueString(const object::ELFObjectFileBase *Obj,
                                   const object::RelocationRef &Rel,
                                   llvm::SmallVectorImpl<char> &Result);
 Error getCOFFRelocationValueString(const object::COFFObjectFile *Obj,
                                    const object::RelocationRef &Rel,
                                    llvm::SmallVectorImpl<char> &Result);
 Error getWasmRelocationValueString(const object::WasmObjectFile *Obj,

llvm/tools/llvm-objdump/llvm-objdump.cpp

static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 static StringSet<> DisasmFuncsSet;
 static StringSet<> FoundSectionSet;
 static StringRef ToolName;
 
 typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
 
-static bool shouldKeep(object::SectionRef S) {
+namespace {
+struct FilterResult {
+  // True if the section should not be skipped.
+  bool Keep;
+
+  // True if the index counter should be incremented, even if the section should
+  // be skipped. For example, sections may be skipped if they are not included
+  // in the --section flag, but we still want those to count toward the section
+  // count.
+  bool IncrementIndex;
+};
+} // namespace
+
+static FilterResult checkSectionFilter(object::SectionRef S) {
   if (FilterSections.empty())
-    return true;
+    return {/*Keep=*/true, /*IncrementIndex=*/true};
 
   Expected<StringRef> SecNameOrErr = S.getName();
   if (!SecNameOrErr) {
     consumeError(SecNameOrErr.takeError());
-    return false;
+    return {/*Keep=*/false, /*IncrementIndex=*/false};
   }
   StringRef SecName = *SecNameOrErr;
 
   // StringSet does not allow empty key so avoid adding sections with
   // no name (such as the section with index 0) here.
   if (!SecName.empty())
     FoundSectionSet.insert(SecName);
-  return is_contained(FilterSections, SecName);
-}
 
-SectionFilter ToolSectionFilter(object::ObjectFile const &O) {
-  return SectionFilter([](object::SectionRef S) { return shouldKeep(S); }, O);
+  // Only show the section if it's in the FilterSections list, but always
+  // increment so the indexing is stable.
+  return {/*Keep=*/is_contained(FilterSections, SecName),
+          /*IncrementIndex=*/true};
+}
+
+SectionFilter ToolSectionFilter(object::ObjectFile const &O, uint64_t *Idx) {
+  // Start at UINT64_MAX so that the first index returned after an increment is
+  // zero (after the unsigned wrap).
+  if (Idx)
+    *Idx = UINT64_MAX;
+  return SectionFilter(
+      [Idx](object::SectionRef S) {
+        FilterResult Result = checkSectionFilter(S);
+        if (Idx != nullptr && Result.IncrementIndex)
+          *Idx += 1;
+        return Result.Keep;
+      },
+      O);
 }
 
 std::string getFileNameForError(const object::Archive::Child &C,
                                 unsigned Index) {
   Expected<StringRef> NameOrErr = C.getName();
   if (NameOrErr)
     return NameOrErr.get();
   // If we have an error getting the name then we print the index of the archive

 }
 
 // Returns a map from sections to their relocations.
 static std::map<SectionRef, std::vector<RelocationRef>>
 getRelocsMap(object::ObjectFile const &Obj) {
   std::map<SectionRef, std::vector<RelocationRef>> Ret;
   for (SectionRef Sec : Obj.sections()) {
     section_iterator Relocated = Sec.getRelocatedSection();
-    if (Relocated == Obj.section_end() || !shouldKeep(*Relocated))
+    if (Relocated == Obj.section_end() || !checkSectionFilter(*Relocated).Keep)
       continue;
     std::vector<RelocationRef> &V = Ret[*Relocated];
     for (const RelocationRef &R : Sec.relocations())
       V.push_back(R);
     // Sort relocations by address.
     llvm::stable_sort(V, isRelocAddressLess);
   }
   return Ret;
outs() << "Sections:\n"
            << left_justify("VMA", AddressWidth) << " "
            << left_justify("LMA", AddressWidth) << " Type\n";
   else
     outs() << "Sections:\n"
               "Idx "
            << left_justify("Name", NameWidth) << " Size     "
            << left_justify("VMA", AddressWidth) << " Type\n";
 
-  for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
+  uint64_t Idx;
+  for (const SectionRef &Section : ToolSectionFilter(*Obj, &Idx)) {
     StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());
     uint64_t VMA = Section.getAddress();
     if (shouldAdjustVA(Section))
       VMA += AdjustVMA;
 
     uint64_t Size = Section.getSize();
 
     std::string Type = Section.isText() ? "TEXT" : "";
     if (Section.isData())
       Type += Type.empty() ? "DATA" : " DATA";
     if (Section.isBSS())
       Type += Type.empty() ? "BSS" : " BSS";
 
     if (HasLMAColumn)
-      outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),
-                       NameWidth, Name.str().c_str(), Size)
+      outs() << format("%3d %-*s %08" PRIx64 " ", Idx, NameWidth,
+                       Name.str().c_str(), Size)
              << format_hex_no_prefix(VMA, AddressWidth) << " "
              << format_hex_no_prefix(getELFSectionLMA(Section), AddressWidth)
              << " " << Type << "\n";
     else
-      outs() << format("%3d %-*s %08" PRIx64 " ", (unsigned)Section.getIndex(),
-                       NameWidth, Name.str().c_str(), Size)
+      outs() << format("%3d %-*s %08" PRIx64 " ", Idx, NameWidth,
+                       Name.str().c_str(), Size)
              << format_hex_no_prefix(VMA, AddressWidth) << " " << Type << "\n";
   }
   outs() << "\n";
 }
 
 void printSectionContents(const ObjectFile *Obj) {
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     StringRef Name = unwrapOrError(Section.getName(), Obj->getFileName());