[PATCH] D92923: [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:74
+      if (N >= *Size)
+        return createError("the index is greater than the number of entries (" +
+                           Twine(*Size) + ")");
----------------
Maybe: "the index N is greater than..." (where N is the specified index)?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:74
+        Value: 0
+## By naming SHT_SYMTAB_SHNDX to .foo and .bar we verify that we
+## don't use their names to locate them.
----------------
I found the original wording a little hard to follow initially. This phrasing would make it easier.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:112
+## dynamic tag when dumping dynamic symbols. In this case we make the value of
+## DT_SYMTAB_SHNDX to point to the SHT_SYMTAB_SHNDX section that is
+## linked with the static symbol table and check that we use it.
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:134-135
+
+## Check that when report a warning when we dump the dynamic symbol table that
+## contains symbols with extended indices, but we don't have the DT_SYMTAB_SHNDX tag to locate
+## the corresponding extended indexes table.
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:171-172
+
+## In this case we have the SHT_SYMTAB_SHNDX section with the sh_link field that has a
+## value that is larger than the number of section. Check we report a warning.
+
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:198
+
+## Check we report a warning when multiple SHT_SYMTAB_SHNDX are linked to a symbol table.
+## In this case 2 sections are linked to the dynamic symbol table. Check it doesn't affect
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:199
+## Check we report a warning when multiple SHT_SYMTAB_SHNDX are linked to a symbol table.
+## In this case 2 sections are linked to the dynamic symbol table. Check it doesn't affect
+## anything, because the SHT_SYMTAB_SHNDX section specified by the DT_SYMTAB_SHNDX tag is still used.
----------------
We probably also need a case for two linked to .symtab, where the dynamic tag isn't used, to show which is picked.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:234
+
+## In this case we have the SHT_SYMTAB_SHNDX section placed right before
+## the end of the file. This table is broken: it contains less entries than
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:235
+## In this case we have the SHT_SYMTAB_SHNDX section placed right before
+## the end of the file. This table is broken: it contains less entries than
+## the number of dynamic symbols.
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test:240
+
+# RUN: yaml2obj --docnum=2 -DSYMTABSHNDX=0x2000 %s -o %t.pastend
+# RUN: llvm-readelf --dyn-syms %t.pastend 2>&1 | \
----------------
You only use the second docnum once. Can you get rid of the macro?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6588
             this->FileName, this->Obj.getStringTableForSymtab(*Symtab));
-
+        ArrayRef<Elf_Word> ShNdxTable = this->dumper().getShndxTable(Symtab);
         typename ELFT::SymRange Symbols =
----------------
etc, for consistency with the method name.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92923/new/

https://reviews.llvm.org/D92923