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

[Prabhu Karthikeyan Rajasekaran via Phabricator via llvm-commits]

Prabhuk added a comment.

Updated the printProgramHeaders section to address the review comment and to fix logic flaws. Updated corresponding tests. Please take a look.



================
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:{{.}}
----------------
jhenderson wrote:
> 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.
This change as it turns out is wrong. The code path that renders only "--section-mapping" is not originally intended to change. 


================
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)
----------------
jhenderson wrote:
> 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.
You're right. This is a duplicate check of what is already handled in section-details.test. Removing this 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.
 
----------------
jhenderson wrote:
> 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.
Once again there was an error in this work flow. Clean up of "Program Headers" section was intended. But the Section to Segment mapping was not intended to be changed so bringing that part back.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3689-3690
+        this->Obj.getSectionStringTable(Sections, this->WarningHandler);
+    if (!SecStrTableOrErr)
+      this->reportUniqueWarning(SecStrTableOrErr.takeError());
+    return;
----------------
jhenderson wrote:
> We need a test case that shows this warning being emitted.
test/tools/llvm-readobj/ELF/many-sections.s line 36,37 (shown below) exercises this code path.

# RUN: llvm-readelf --file-headers --sections %t2 2>&1 | \
# RUN:   FileCheck %s -DFILE=%t2 --check-prefix=GNU2


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

test/tools/llvm-readobj/ELF/many-sections.s line 38,39 (shown below) :
# RUN: llvm-readelf --file-headers --section-details %t2 2>&1 | \
# RUN:   FileCheck %s -DFILE=%t2 --check-prefix=GNU2


================
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();
----------------
jhenderson wrote:
> 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?
This comment helped me find a logical flaw in rendering. The control flow of invocations that request for segment details/mapping without the program header info should not change. I have updated this function to reflect this change and considered some of the cleanups suggested in your comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130670