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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:295
+# RUN: yaml2obj --docnum=8 %s > %t08
+# RUN: llvm-readelf --stack-sizes %t08 2> %t08.err | FileCheck %s --check-prefix=NORELOCSECTION-OUT
+# RUN: FileCheck %s < %t08.err --check-prefix=NORELOCSECTION-ERR -DFILE=%t08
----------------
wolfgangp wrote:
> grimar wrote:
> > We usually use 2>&1. I am not sure it worth to chech the output separatelly here.
> It was suggested by a different reviewer to check the output as well. If you do not feel strongly about it, I would like to keep it in place.
Sure, NP.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:323
+# RUN: llvm-readelf --stack-sizes %t09 %t10 | FileCheck %s --check-prefix=MULTIPLE
+
+## Check that we handle an archive.
----------------
wolfgangp wrote:
> grimar wrote:
> > I'd expect to see "#      MULTIPLE:" lines right here. You can probably test `ARCHIVE` just separatelly.
> I collapsed the 2 test cases and removed some unnecessary yaml2obj invocations.
> I'd like to intertwine the ARCHIVE and MULTIPLE check lines to check the proper
> ordering. Please let me know if that's OK.
Looks good.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4550
+
+    bool (*Supports)(uint64_t);
+    RelocationResolver Resolver;
----------------
wolfgangp wrote:
> grimar wrote:
> > `Supports` probably not a very good name for function. May be `IsSupportedFn`?
> But it's so cool to be able to say 
> 
> ```
> if (Supports(...))
> ```
> Well, if you insist...
Well, may be other reviewers have a different opinion here. `Supports` just doesn't look like a function name for me.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4554
+    StringRef RelocSectionName;
+    RelocSec.getName(RelocSectionName);
+
----------------
So you're using `RelocSectionName` only for the error reporting.
And `reportError` is no return function, so it is called only once.
Then you can just move these lines right to the place where you construct the error.


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

https://reviews.llvm.org/D65313