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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've only done a fairly brief review, so don't wait for me for an LGTM if anybody else gives it. I'm happy to defer to others judgment on the --all behaviour. My instinct there is that the --all behaviour change is a wider issue beyond --stack-sizes. For example, --addrsig, --hash-symbols etc are switches that don't exist in GNU readelf, but might be printed if they did.



================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:194
+
+## Check that we report an error when a stack sizes section ends with a complete stack size entry.
+
----------------
"with a complete" sounds like there's a mistake here. Should it be "within"?


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:434
+# ARCHIVEWARN:      Stack Sizes:
+# ARCHIVEWARN-NEXT: Size Function
+# ARCHIVEWARN:      8 foo
----------------
Let's tidy up the spacing in this block here so that it better matches what is printed (and is therefore more readable).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4317-4318
+    const StringRef SectionName, DataExtractor Data, uint64_t *Offset) {
+  // We are ignoring potentially erroneous input, unless it is directly
+  // related to stack size reporting.
+  SymbolRef FuncSym;
----------------
If I'm not mistaken, we tend to write comments in the third person i.e. "This function ignores..."


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4380
+                                     DataExtractor Data) {
+  // We are ignoring potentially erroneous input, unless it is directly
+  // related to stack size reporting.
----------------
See my comment above.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4405-4406
+                    " is not in the expected section");
+      // Pretend that the symbol is in the correct section. We will report
+      // its stack size anyway.
+      FunctionSec = **SectionOrErr;
----------------
Perhaps rephrase this "correct section and report its stack size anyway."


================
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));
----------------
wolfgangp wrote:
> 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.
I'm in two minds about this. My hope is that we could ignore GNU compatibility on this, and indeed I'd expect people to not be parsing --all output in such a way as it can't work if more stuff was emitted, but I could be wrong. Absolutely it should be printed for LLVM output. I'm happy to defer to others on this one.


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

https://reviews.llvm.org/D65313