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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Sorry, I had some draft comments yesterday but had to leave early, here are post-commit comments now:



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4524
+
+  for (const auto &StackSizeMapEntry : StackSizeRelocMap) {
+    PrintHeader();
----------------
Is this iteration order well defined/stable? I tried going down the rabbit hole of making sure this isn't sorting based on arbitrary pointer values, but couldn't figure out what it's keyed off of. Is it sorting by section name/index?

(llvm::MapVector itself is deterministic/sorted on the key, but if the key is a pointer, it's effectively random)


================
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));
----------------
jhenderson wrote:
> 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.
Hmm, good point; definitely there are countless users that invoke+parse the output of `readelf -a` (when probably they only care about a few things, but use `-a` as a lazy way to just get everything). But there's so much output it produces, any strict parser that would break if an additional block is produced is probably so fragile anyway.

Still, might be good to submit this without being included in `--all` (for readelf), and we can add it there that as a separate change after having this feature live in the wild for a bit/get some more opinions.


================
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 with --all (only for LLVM-style).
+    if (opts::Output == opts::LLVM)
+      opts::PrintStackSizes = false;
----------------
I think this block is obsolete now? The default for flags is false, so this just means that `llvm-readobj --all --stack-sizes` won't print stack sizes, but `llvm-readobj --stack-sizes` will (and will print the warning).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65313