[PATCH] D67998: [llvm-readobj/llvm-readelf]: Dump stack-size entry for all function symbols that have a particular value (fixes PR43245)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/stack-sizes.test:95-100
+## object file. Furthermore, check that we report stack sizes for functions that
+## share a stack size entry with other functions (e.g. because several functions
+## have been collapsed into a single function body by the linker). Also, check
+## that if we have duplicate stack size entries for a function, we only print one
+## entry. Lastly, check that we issue a warning should the stack sizes mismatch in
+## duplicate entries.
----------------
I think combining the different cases into a single test run is risking confusion. I'd prefer it if each issue were a separate test (i.e. have a basic single-entry only case, a NOBITS case, a multiple entries for the same address case, and one where the sizes conflict).


================
Comment at: test/tools/llvm-readobj/stack-sizes.test:172-176
+  - Name:    myobj
+    Section: .text
+    Value:   0x20
+    Type:    STT_OBJECT
+    Binding: STB_GLOBAL
----------------
It's not clear from anywhere why this symbol exists. It needs commenting at least. See also my comments above.


================
Comment at: test/tools/llvm-readobj/stack-sizes.test:686
+# RUN:   | FileCheck %s -DFILE=%t18 --check-prefix=DUPLICATE-LLVM
+# RUN: FileCheck %s < %t18-gnu.err --check-prefix=DUPLICATE-ERR -DFILE=%t18
+# RUN: FileCheck %s < %t18-llvm.err --check-prefix=DUPLICATE-ERR -DFILE=%t18
----------------
FileCheck has an --input-file switch, which I think is more commonly used instead of redirecting stdin, when the file exists on disk.


================
Comment at: test/tools/llvm-readobj/stack-sizes.test:717
+      - Size: 0x20
+    Link:    .text
+  - Name: .rela.stack_sizes
----------------
This looks out-of-line in comparison to the other fields.


================
Comment at: test/tools/llvm-readobj/stack-sizes.test:735-737
+  - Name:    .text
+    Section: .text
+    Type:    STT_SECTION
----------------
Using a section symbol is rather confusing. Can't this just be another symbol with value 0 in the same section?

Also, I think relocations are a more complex case to the executable one. In particular, you need to handle two symbols in different sections, but with identical values. In these cases, they aren't duplicates (and you need to show that).


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4731
+        Obj->getFileName());
+    printStackSizeEntry(StackSize, "?");
+  }
----------------
jhenderson wrote:
> Whilst not strictly necessary, it might be good to `return` here. I think that would be less likely to run into surprises due to future code changes below it.
This is marked as done but hasn't been addressed?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:373
   using Elf_Addr = typename ELFT::Addr;
+  using StackSizeEntryMap = std::map<uint64_t, uint64_t>;
 
----------------
Does this map need to be ordered?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4724
+    reportWarning(
+        createError("could not identify function symbol for stack size entry"),
+        Obj->getFileName());
----------------
I think these warnings need more context - which entry can't you identify a function symbol for here? (Add the offset).


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4762
+          createError(
+              "mismatching sizes in stack size entries for the same address"),
+          Obj->getFileName());
----------------
Which entries (include the offsets), and what were their sizes?


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

https://reviews.llvm.org/D67998