[PATCH] D65313: [llvm-readelf] Dump the stack sizes sections with --stack-sizes

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

I'm not familiar with the `.stack_sizes` functionality in general, but it seems useful & could have saved me countless hours of debugging (large stack frames => stack overflows in recursive parsers) if I knew more about how it worked.



================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:8-12
+#      RELOC:         Size     Function
+# RELOC-NEXT:           16     referenced_by_symbol_foo
+# RELOC-NEXT:           32     referenced_via_section_bar
+# RELOC-NEXT:            8     separate_text_section_baz
+#  RELOC-NOT:{{.}}
----------------
Out of curiousity, how did you decide on this format? Does this number of columns line up with other readelf options? (There is no GNU readelf --stack-size option I'm aware of that this could be emulating).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4315
+      // issues directly related to stack size dumping.
+      consumeError(SymAddrOrErr.takeError());
+      continue;
----------------
wolfgangp wrote:
> grimar wrote:
> > This looks worrying. Why do you need this?
> I am scanning the symbol table to figure out which function symbol the stack size entry corresponds to. Symbol.getAddress() returns an 'Expected', which means that it may possibly return an error. I need to test the Expected and once I do that, and an error has been returned, the 'Expected' interface mandates that we do something with this error, i.e. either report it (by calling takeError()) or ignore it (by calling consumeError()). If we don't do that, the  'Expected' destructor asserts and we get a compiler crash.
> 
> As far as choosing whether to report or consume the error, I have decided that unless the error is directly related to stack size dumping, I would rather ignore it and leave the reporting to other aspects of dumping, e.g. when explicitly dumping the symbol table. Hence the consumeError() here. 
> 
> This also goes for all the other occurrences of consumeError().
I think this is fine, but instead of just having the comment for this one `consumeError` call, it might be better to have one at the top of each method that explains the blanket exception


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:391-397
+  void printStackSize(const ELFObjectFile<ELFT> *Obj, uint64_t SymValue,
+                      SectionRef FunctionSec, const StringRef SectionName,
+                      DataExtractor Data, uint64_t *Offset);
+  void printStackSize(const ELFObjectFile<ELFT> *Obj, RelocationRef Rel,
+                      SectionRef FunctionSec,
+                      const StringRef &StackSizeSectionName,
+                      const RelocationResolver &Resolver, DataExtractor Data);
----------------
Why two methods with the same name? It's confusing trying to figure out what is calling what.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4441
+  StringRef FileStr = Obj->getFileName();
+  if (!Obj->isRelocatableObject()) {
+    for (const SectionRef &Sec : Obj->sections()) {
----------------
Why is this check here? This check happens before the method is called.

If you want to keep it in (I'm not sure I care either way), put it in an assert instead.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4459
+                    Contents.size()),
+          true, sizeof(Elf_Addr));
+      // A .stack_sizes section header's sh_link field is supposed to point
----------------
Should `true` be `Obj->isLittleEndian()`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4496-4501
+  for (const SectionRef &Sec : Obj->sections()) {
+    StringRef SectionName;
+    Sec.getName(SectionName);
+    // A stack size section that we haven't encountered yet is mapped to the
+    // null section until we find its corresponding relocation section.
+    if (SectionName.startswith(".stack_sizes"))
----------------
I may just be missing something obvious, but I don't see why this wildly differs from the previous method. Why does the previous method go through all these hoops:
```
      const Elf_Shdr *ElfSec = Obj->getSection(Sec.getRawDataRefImpl());
      Expected<StringRef> NameOrErr = EF->getSectionName(ElfSec);
      // Ignore sections whose name we cannot find.
      if (!NameOrErr) {
        consumeError(NameOrErr.takeError());
        continue;
      }
      StringRef SectionName = *NameOrErr;
      if (!SectionName.startswith(".stack_sizes"))
```
... but then this one skips all that and just calls `getName(SectionName)`?


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:58
                    "--dynamic-table, --notes, --version-info, --unwind, "
-                   "--section-groups and --elf-hash-histogram."));
+                   "--stack-sizes, --section-groups and --elf-hash-histogram."));
   cl::alias AllShort("a", cl::desc("Alias for --all"), cl::aliasopt(All));
----------------
`--stack-sizes` is not a GNU readelf options, so I don't think it should be implied when running `llvm-readelf -a`


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:737-740
+    // FIXME: As soon as we implement LLVM-style printing of the .stack_size
+    // section, we will enable it unconditionally with --all.
+    if (opts::Output == opts::GNU)
+      opts::PrintStackSizes = true;
----------------
(I guess my comment earlier actually applies here)


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

https://reviews.llvm.org/D65313