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

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:186
+    Section: .text
+    Type:    STT_SECTION
+  - Name:    foo
----------------
grimar wrote:
> Do you need section symbol?
No, it's not needed.


================
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
----------------
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.


================
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.
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:359
+# RUN: llvm-readelf --all %t14 2> %t14.err | FileCheck %s --check-prefix=NONFUNCTIONSYM
+# RUN: FileCheck %s < %t14.err --check-prefix=NONFUNCTIONSYM-ERR -DFILE=%t14
+
----------------
grimar wrote:
> Again, probably I'd not test errors separatelly. (i.e. would use 2>&1)
Testing the output separately was suggested by someone else. Please let me know if you feel strongly about it.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4315
+      // issues directly related to stack size dumping.
+      consumeError(SymAddrOrErr.takeError());
+      continue;
----------------
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().


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4336
+    else
+      consumeError(FuncNameOrErr.takeError());
+  } else
----------------
grimar wrote:
> Why?
Please see my comment above.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4378
+    else
+      consumeError(NameOrErr.takeError());
+
----------------
grimar wrote:
> Why?
Please see my comment above.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4398
+    else
+      consumeError(RelocSymValueOrErr.takeError());
+  }
----------------
grimar wrote:
> Why?
Please see my comment above.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4416
+template <class ELFT>
+void GNUStyle<ELFT>::printStackSizes(const ELFObjectFile<ELFT> *Obj) {
+  bool HeaderHasBeenPrinted = false;
----------------
grimar wrote:
> This method is very long. If there is any way to make it shorter, that would be great.
This morphed into a somewhat larger refactoring. I will explain this in detail in the commit message.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4433
+    return SectionRef(DRI, Obj);
+  };
+
----------------
grimar wrote:
> Seems you can use `ELFObjectFile::toDRI()` to hide work with `DRI.p` and move this out to make it static helper probably?
I extracted it into a helper due to the refactoring, but unfortunately ELFObjectFile::toDRI() is protected, so I can't use it here. Now, this could be refactored, but I don't want to lump it into the same patch.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4550
+
+    bool (*Supports)(uint64_t);
+    RelocationResolver Resolver;
----------------
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...


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

https://reviews.llvm.org/D65313