[llvm-readelf] Symbol index in symbol table printing is not reset

[mikkqu]

Summary

Stop using static variables for keeping track of symbol indices.

Bugfix for: https://bugs.llvm.org/show_bug.cgi?id=46777

[lattner]

Wow, yeah, static variables are wildly inappopriate for that. That said, I'm not familiar with the code, so someone else should review.

[grimar]

Generally looks good. A few comments inlined.
But please do not upload patches without a context, see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

llvm/test/tools/llvm-readobj/ELF/section-symbols-index.test

1 ↗

(On Diff #280750)

Probably this test should be a part of symbols.test.

4 ↗

(On Diff #280750)

We prefer long form for options in tests I think. I.e. -s -> --symbols.

18 ↗

(On Diff #280750)

I'd expect to have just 6 lines here probably. E.g:

# CHECK:      Symbol table '.symtab' contains 3 entries:
# CHECK-NEXT:  Num: {{.*}}
# CHECK-NEXT:    0: {{.*}}
# CHECK:      Symbol table '.symtab' contains 3 entries:
# CHECK-NEXT:  Num: {{.*}}
# CHECK-NEXT:    0: {{.*}}

30 ↗

(On Diff #280750)

You can just have Symbols: [] here. It is enough for the test.

llvm/tools/llvm-readobj/ELFDumper.cpp

3932

You can inline it:

Fields[0].Str = to_string(format_decimal(Symbol - FirstSym, 6)) + ":";

[mikkqu]

Changes on comments from @grimar:

• Updated the test case and moved it to symbols.test
• Inlined the index evaluation
• Uploaded full context

[grimar]

llvm/test/tools/llvm-readobj/ELF/symbols.test

133

You probably do not need a new YAML anymore. You can just reuse the %t64, used in cases 1-5:
llvm-readelf --symbols %t64 %t64

[mikkqu]

• Reuse YAML in test case

[grimar]

LGTM

[jhenderson]

Thanks, this change looks good to me too. Do you need @grimar or I to commit it for you?

P.S, you can mark inline comments as "Done" by selecting the appropriate option for them. If you do this before uploading a new diff, it will submit that "Mark as Done" automatically, and it will be easier to see what changes you intended to address with your patch.

[mikkqu]

Yes, could you please commit it, as I don't have the access.

[grimar]

I can commit it, but I need to know a name and email you'd like to use as author. E.g:

--author "Name Surname <[e-mail]>"

[mikkqu]

That is:

--author "Mikhail Kalashnikov <[e-mail]>"

llvm/test/tools/llvm-readobj/ELF/symbols.test

 ## Case 4: --symbols implies --dyn-symbols for llvm-readelf.
 # RUN: llvm-readobj --symbols %t64 | FileCheck %s --implicit-check-not="DynamicSymbols ["
 # RUN: llvm-readelf --symbols %t64 | FileCheck %s --check-prefix=BOTH-GNU
 
 ## Case 5: Test that `--dyn-symbols` does not trigger `--symbols` implicitly.
 # RUN: llvm-readobj --dyn-symbols %t64 | FileCheck %s --implicit-check-not="{{^}}Symbols ["
 # RUN: llvm-readelf --dyn-symbols %t64 | FileCheck %s --implicit-check-not="Symbol table '.symtab'"
 
+## Case 6: Test that the Num index starts from zero at every new symbol table.
+# RUN: llvm-readelf --symbols %t64 %t64 | FileCheck %s --check-prefix=NUM-INDEX
+
+# NUM-INDEX:      Symbol table '.symtab' contains 3 entries:
+# NUM-INDEX-NEXT:    Num: {{.*}}
+# NUM-INDEX-NEXT:      0: {{.*}}
+# NUM-INDEX:      Symbol table '.symtab' contains 3 entries:
+# NUM-INDEX-NEXT:    Num: {{.*}}
+# NUM-INDEX-NEXT:      0: {{.*}}
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS[[BITS]]
   Data:    ELFDATA2LSB
   Type:    [[TYPE]]
   Machine: EM_X86_64
 Symbols:
   - Name:  foo
     Value: 0x1
   - Name:  bar
     Value: 0x2
 DynamicSymbols:
   - Name:  zed
 
 ## Check the behavior when we are unable to print symbols due to an error.
 
 ## Case 1: check we are able to dump symbols even when can't get the string table for the
 ## SHT_SYMTAB section because of invalid sh_link value.
 # RUN: yaml2obj --docnum=2 -DLINK=0xff %s -o %t64.err1
 # RUN: llvm-readobj --symbols %t64.err1 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t64.err1 --check-prefix=STRTAB-LINK-ERR-LLVM
 # RUN: llvm-readelf --symbols %t64.err1 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t64.err1 --check-prefix=STRTAB-LINK-ERR-GNU
 
 # STRTAB-LINK-ERR-LLVM:      Symbols [
 # STRTAB-LINK-ERR-LLVM-NEXT: warning: '[[FILE]]': unable to get the string table for the SHT_SYMTAB section: invalid section index: 255

llvm/tools/llvm-readobj/ELFDumper.cpp

std::string GNUStyle<ELFT>::getSymbolSectionNdx(const ELFO *Obj,
   }
 }
 
 template <class ELFT>
 void GNUStyle<ELFT>::printSymbol(const ELFO *Obj, const Elf_Sym *Symbol,
                                  const Elf_Sym *FirstSym,
                                  Optional<StringRef> StrTable, bool IsDynamic,
                                  bool NonVisibilityBitsUsed) {
-  static int Idx = 0;
-  static bool Dynamic = true;
-
-  // If this function was called with a different value from IsDynamic
-  // from last call, happens when we move from dynamic to static symbol
-  // table, "Num" field should be reset.
-  if (!Dynamic != !IsDynamic) {
-    Idx = 0;
-    Dynamic = false;
-  }
-
   unsigned Bias = ELFT::Is64Bits ? 8 : 0;
   Field Fields[8] = {0,         8,         17 + Bias, 23 + Bias,
                      31 + Bias, 38 + Bias, 48 + Bias, 51 + Bias};
-  Fields[0].Str = to_string(format_decimal(Idx++, 6)) + ":";
+  Fields[0].Str = to_string(format_decimal(Symbol - FirstSym, 6)) + ":";
   Fields[1].Str = to_string(
       format_hex_no_prefix(Symbol->st_value, ELFT::Is64Bits ? 16 : 8));
   Fields[2].Str = to_string(format_decimal(Symbol->st_size, 5));
 
   unsigned char SymbolType = Symbol->getType();
   if (Obj->getHeader()->e_machine == ELF::EM_AMDGPU &&
       SymbolType >= ELF::STT_LOOS && SymbolType < ELF::STT_HIOS)
     Fields[3].Str = printEnum(SymbolType, makeArrayRef(AMDGPUSymbolTypes));
   else
     Fields[3].Str = printEnum(SymbolType, makeArrayRef(ElfSymbolTypes));