[PATCH] D89304: [llvm-readelf] - Implement --section-details option.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

`llvm-readelf -S` is actually an equivalent to `readelf -S --wide` in terms of formatting. It probably makes sense for us to do the same with `-t` (i.e. match the wide formatting), with all the content except the flags on one line.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:3
+
+## Check the output for 64-bit case.
+# RUN: yaml2obj %s -DBITS=64 -o %t64.o
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:19
+#  GNU64-NEXT:       [0000000000000000]: {{$}}
+#  GNU64-NEXT:  [ 1] allflags
+#  GNU64-NEXT:       PROGBITS          1111111111111111 2222222222222222  3
----------------
We should have at least one section with a long name that goes beyond the first column width, I think.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:21
+#  GNU64-NEXT:       PROGBITS          1111111111111111 2222222222222222  3
+#  GNU64-NEXT:       4444444444444444  0000000000000005 6                 7
+#  GNU64-NEXT:       [0000000080000ff7]: WRITE, ALLOC, EXEC, MERGE, STRINGS, INFO LINK, LINK ORDER, OS NONCONF, GROUP, TLS, COMPRESSED, EXCLUDE
----------------
Any reason not to have all 5s for the entsize? What about a high alignment (since the addralign field is much wider than one byte)?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:75
+#  GNU64-NEXT:       [0000000080000000]: EXCLUDE
+#  GNU64-NEXT:  [15] known_and_unknown
+#  GNU64-NEXT:       PROGBITS          0000000000000000 0000000000000046  0
----------------
Is UNKNOWN printed for "known" OS/Processor specific flags with GNU readelf?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:158
+
+## When --section-details and --sections are both specified, the latter one is ignored.
+# RUN: llvm-readelf %t64.o --section-details --sections | FileCheck %s --check-prefix=GNU64
----------------
When reading this comment, I was expecting to see --sections win when specified first (latter could mean the latter as specified on the command-line).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/section-details.test:167
+
+## Check the output for 32-bit case.
+# RUN: yaml2obj %s -DBITS=32 -o %t32.o
----------------
I'd move this case to be immediately after the "standard" 64-bit case.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4205
+  else
+    this->reportUniqueWarning(SecStrTableOrErr.takeError());
+
----------------
Test case?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4213-4214
+
+    StringRef Name = unwrapOrError<StringRef>(
+        this->FileName, this->Obj.getSectionName(S, SecStrTable));
+    PrintFields({{Name, 7}});
----------------
Shouldn't this use `reportUniqueWarning`? Also, test case?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4267
+        First = false;
+        OS << (*It).second;
+      } else {
----------------



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

https://reviews.llvm.org/D89304