[PATCH] D57365: [llvm-readobj] Add a flag to dump just the section-to-segment mapping.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:21-24
+RUN: llvm-readelf -section-mapping %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | FileCheck %s -check-prefix ELF64-MAPPING
+RUN: llvm-readelf -section-mapping %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | FileCheck %s -check-prefix ELF64-JUSTMAPPING
----------------
You can combine these two into a single test, by using `--implicit-check-not` which I'd forgotten about until now. Just do the following:

```
RUN: llvm-readelf -section-mapping %p/Inputs/phdrs-elf.exe-x86_64 \
RUN:   --implicit-check-not="Program Headers:" | FileCheck %s -check-prefix ELF64-MAPPING
```


================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:27-30
+RUN: llvm-readelf -section-mapping=false -program-headers %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | FileCheck %s -check-prefix ELF64-PHDRS
+RUN: llvm-readelf -section-mapping=false -program-headers %p/Inputs/phdrs-elf.exe-x86_64 \
+RUN:   | FileCheck %s -check-prefix ELF64-JUSTPHDRS
----------------
Same as above, but with --implicit-check-not="Section to Segment mapping".


================
Comment at: llvm/test/tools/llvm-readobj/gnu-phdrs.test:106
 
-ELF64:  Section to Segment mapping:
-ELF64-NEXT:   Segment Sections...
-ELF64-NEXT:    00
-ELF64-NEXT:    01     .interp
-ELF64-NEXT:    02     .interp .note.ABI-tag .note.gnu.build-id .hash .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
-ELF64-NEXT:    03     .tdata .init_array .fini_array .jcr .dynamic .got .got.plt .data .bss
-ELF64-NEXT:    04     .dynamic
-ELF64-NEXT:    05     .note.ABI-tag .note.gnu.build-id
-ELF64-NEXT:    06     .tdata .tbss
-ELF64-NEXT:    07     .eh_frame_hdr
-ELF64-NEXT:    08
-ELF64-NEXT:    09     .tdata .init_array .fini_array .jcr .dynamic .got
+ELF64-ONEMAPPING-COUNT-1: Section to Segment mapping:
----------------
I didn't know about CHECK-COUNT before, but my reading of the documentation is that this doesn't prevent further matches after this. You'll need a CHECK-NOT pattern after it (in which case, you don't need the COUNT).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:158
+                           cl::boolOrDefault PrintSectionMapping) override;
+  void printSectionMapping() override;
   void printHashTable() override;
----------------
Do you need this? I think it's dead code.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:345
+                                   cl::boolOrDefault PrintSectionMapping) = 0;
+  virtual void printSectionMapping(const ELFFile<ELFT> *Obj) = 0;
   virtual void printHashHistogram(const ELFFile<ELFT> *Obj) = 0;
----------------
Can you remove this?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:380
+                           cl::boolOrDefault PrintSectionMapping) override;
+  void printSectionMapping(const ELFO *Obj) override;
   void printHashHistogram(const ELFFile<ELFT> *Obj) override;
----------------
This could be private.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:474
+                           cl::boolOrDefault PrintSectionMapping) override;
+  void printSectionMapping(const ELFO *Obj) override {}
   void printHashHistogram(const ELFFile<ELFT> *Obj) override;
----------------
This could be private, or even not exist.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3338-3339
+
+  // Display the section mappings along with the program headers,
+  // unless -section-mapping is explicitly set to false.
+  if (PrintSectionMapping != cl::BOU_FALSE)
----------------
Nit: looks like you manually wrapped this comment rather than let clang-format do, and the first line ends unnecessarily early.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4540-4541
+    printProgramHeaders(Obj);
+  if (PrintSectionMapping == cl::BOU_TRUE)
+    printSectionMapping(Obj);
+}
----------------
Given we don't currently support section mapping in LLVM style, does it make sense to have this here?


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

https://reviews.llvm.org/D57365