[PATCH] D130670: [llvm-readelf] Render messages similar to that of `GNU binutils readelf` when no sections and/or no headers.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-section-mapping-no-phdrs.test:3-6
 # RUN: llvm-readelf --section-mapping %t | FileCheck %s --strict-whitespace --match-full-lines
 
-#      CHECK: Section to Segment mapping:
-# CHECK-NEXT:  Segment Sections...
-# CHECK-NEXT:   None   .foo .strtab .shstrtab 
-#  CHECK-NOT:{{.}}
+# CHECK:There are no program headers in this file.
+# CHECK-NOT:{{.}}
----------------
As we're changing this test anyway, I think it would be good to show that no other output is printed before or after, not just after the checked line. The easiest way to do this is to add `--implicit-check-not={{.}}` to the FileCheck command and then just get rid of the existing CHECK-NOT line.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/many-sections.s:11
 # RUN: llvm-readelf --file-headers -S %t1 | FileCheck %s --check-prefix=GNU1
+# RUN: llvm-readelf --file-headers --section-details %t1 | FileCheck %s --check-prefix=GNU1
 # GNU1: Number of section headers:         0 (3)
----------------
I'm not sure I follow what coverage this line is adding? We need a test case with --section-details but no sections, but you've added this to no-shdrs.test.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/no-phdrs.test:11
 
-# GNU:       There are 0 program headers
-# GNU:       Program Headers:
-# GNU-NEXT:    Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
-# GNU-EMPTY:
-# GNU-NEXT:  Section to Segment mapping:
-# GNU-NEXT:    Segment Sections...
-# GNU-NEXT:     None   .strtab .shstrtab
+# GNU:       There are no program headers in this file.
 
----------------
I think we need to show that the section segment mapping is not printed in this case. I'm thinking something like `GNU-NOT: {{.}}` or better yet, `--implicit-check-not={{.}}` should do the trick.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3685
   ArrayRef<Elf_Shdr> Sections = cantFail(this->Obj.sections());
+  if (Sections.size() == 0) {
+    OS << "\nThere are no sections in this file.\n";
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3689-3690
+        this->Obj.getSectionStringTable(Sections, this->WarningHandler);
+    if (!SecStrTableOrErr)
+      this->reportUniqueWarning(SecStrTableOrErr.takeError());
+    return;
----------------
We need a test case that shows this warning being emitted.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4073
   ArrayRef<Elf_Shdr> Sections = cantFail(this->Obj.sections());
+  if (Sections.size() == 0) {
+    OS << "\nThere are no sections in this file.\n";
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4077-4078
+          this->Obj.getSectionStringTable(Sections, this->WarningHandler);
+    if (!SecStrTableOrErr)
+      this->reportUniqueWarning(SecStrTableOrErr.takeError());
+    return;
----------------
Ditto: this warning needs checking, I believe.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4263
 void GNUELFDumper<ELFT>::printProgramHeaders(
     bool PrintProgramHeaders, cl::boolOrDefault PrintSectionMapping) {
+  const Elf_Ehdr &Header = this->Obj.getHeader();
----------------
I feel like this function could do with some early outs. I'm thinking something along the following lines would be much cleaner:
```
template <class ELFT>
void GNUELFDumper<ELFT>::printProgramHeaders(
    bool PrintProgramHeaders, cl::boolOrDefault PrintSectionMapping) {
  if (!PrintProgramHeaders && PrintSectionMapping == cl::BOU_FALSE)
    return;

  const Elf_Ehdr &Header = this->Obj.getHeader();
  if (Header.e_phnum == 0) {
    OS << "\nThere are no program headers in this file.\n";
    return;
  }

  if (PrintProgramHeaders)
    printProgramHeaders();
  if (PrintSectionMapping != cl::BOU_FALSE)
    printSectionMapping();
}
```
Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130670