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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added a comment.

In D65313#1611137 <https://reviews.llvm.org/D65313#1611137>, @rupprecht wrote:

> In D65313#1609287 <https://reviews.llvm.org/D65313#1609287>, @wolfgangp wrote:
>
> > Left the question about whether --stack-sizes should be include in --all open. This is still a discussion point.
>
>
> Do you have a discussion going on somewhere you can link to?


Not really. I'd like to ask @jhenderson for input on this, but your argument below about being as close to GNU readelf as possible sounds convincing.

> Personally, I think it's neat & wouldn't mind it being included as part of `--all` for personal use, but generally we want to make `llvm-readelf` as close to GNU `readelf` as possible. (This has been feedback given at llvm dev meetings too).



> Maybe it could be part of `--all` only when invoked as `llvm-readobj`? Though it doesn't make much since *now* because this only has the GNU-style dumper implemented, and in fact prints a warning for the LLVM style -- but in the future, we could do that.





================
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:{{.}}
----------------
rupprecht wrote:
> 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).
I settled on this format in consultation with @jhenderson . Basically to maximize readability: The size is right-justified in its column, the function name left-justified. That leaves some room on the left for larger stack sizes and room on the right for long function names (or signatures). Other readelf options weren't really under consideration.

Any suggestions are of course welcome.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4315
+      // issues directly related to stack size dumping.
+      consumeError(SymAddrOrErr.takeError());
+      continue;
----------------
rupprecht wrote:
> 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
Added a comment to each function where consumeError() is used.


================
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);
----------------
rupprecht wrote:
> Why two methods with the same name? It's confusing trying to figure out what is calling what.
My design thought was that one was called from a relocatable context, and the other from a non-relocatable context, so the relocatable one (the one with the 'Rel' parameter), would resolve the relocation and delegate the rest to the non-relocation function. There is no reason for them to have the same name, though. I'll change it.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4441
+  StringRef FileStr = Obj->getFileName();
+  if (!Obj->isRelocatableObject()) {
+    for (const SectionRef &Sec : Obj->sections()) {
----------------
rupprecht wrote:
> 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.
Thanks, oversight during a refactor.


================
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"))
----------------
rupprecht wrote:
> 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)`?
You're not missing anything, I didn't discover until later that SectionRef::getName() encapsulates what I'm doing in the segment above and forgot to update it. Thanks for pointing this out.


================
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));
----------------
rupprecht wrote:
> `--stack-sizes` is not a GNU readelf options, so I don't think it should be implied when running `llvm-readelf -a`
I have no particularly strong opinion on this, but since the stack size functionality originated in LLVM, it's not surprising that GNU readelf would not support it. However, it does seem reasonable to include it with an 'all' option, unless the goal is exact compatibility with GNU.


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

https://reviews.llvm.org/D65313