[PATCH] D70959: [ELF] Support for PT_GNU_PROPERTY in program headers and tools

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/elf-pt-gnu-property.test:3
+
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump -p %t | FileCheck %s
----------------
We've started using `yaml2obj %s -o %t` to avoid an unnecessary shell redirection. Probably best here too.


================
Comment at: llvm/test/tools/llvm-objdump/elf-pt-gnu-property.test:7
+# CHECK: Program Header:
+# CHECK-NEXT: GNU_PROPERTY off    0x0000000000000078 vaddr 0x0000000000000000 paddr 0x0000000000000000 align 2**3
+# CHECK-NEXT: filesz 0x0000000000000020 memsz 0x0000000000000020 flags ---
----------------
Does this output match GNU objdump's by the way?


================
Comment at: llvm/test/tools/llvm-objdump/elf-pt-gnu-property.test:17
+Sections:
+  - Name:         .note.gnu.property
+    Type:         SHT_NOTE
----------------
If I'm not mistaken, this section is completely irrelevant for the test. The only thing that matters is that there is a segment with type PT_GNU_PROPERTY.


================
Comment at: llvm/test/tools/llvm-objdump/elf-pt-gnu-property.test:30
+  - Type: PT_GNU_PROPERTY
+    Align: 8
+    Sections:
----------------
The alignment is irrelevant to the interpretation of the program header type. Delete it to keep the test simpler.


================
Comment at: llvm/test/tools/llvm-readobj/elf-pt-gnu-property.test:3
+
+# RUN: yaml2obj %s > %t
+# RUN: llvm-readelf --program-headers %t | FileCheck %s --check-prefix=ELF
----------------
-o


================
Comment at: llvm/test/tools/llvm-readobj/elf-pt-gnu-property.test:7
+
+# ELF:   GNU_PROPERTY   0x000078 0x0000000000000000 0x0000000000000000 0x000020 0x000020     0x8
+
----------------
You can simplify this to just something like {{ }}GNU_PROPERTY{{ }}. The rest of the data is irrelevant. In fact, I wonder if we should just add an equivalent test to "elf-section-types.test" for program headers. You'll see in that test we do the bare minimum to create a section of each section type to show that llvm-readelf/llvm-readobj can display the various section types.


================
Comment at: llvm/test/tools/llvm-readobj/elf-pt-gnu-property.test:11-18
+# OBJ-NEXT:     Offset: 0x78
+# OBJ-NEXT:     VirtualAddress: 0x0
+# OBJ-NEXT:     PhysicalAddress: 0x0
+# OBJ-NEXT:     FileSize: 32
+# OBJ-NEXT:     MemSize: 32
+# OBJ-NEXT:     Flags [ (0x0)
+# OBJ-NEXT:     ]
----------------
You don't need the rest of these fields.


================
Comment at: llvm/test/tools/llvm-readobj/elf-pt-gnu-property.test:27
+Sections:
+  - Name:         .note.gnu.property
+    Type:         SHT_NOTE
----------------
See the other test: the section is irrelevant and can be deleted.


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

https://reviews.llvm.org/D70959