[PATCH] D68462: [llvm-readobj/llvm-readelf] - Add checks for GNU-style to "all.test" test case.

[George Rimar via Phabricator via llvm-commits]

grimar marked 14 inline comments as done.
grimar added inline comments.


================
Comment at: test/tools/llvm-readobj/all.test:27-28
+# GNU-ALL: Symbol table '.symtab' contains {{.*}} entries:
+# GNU-ALL: EH_FRAME Header [
+# GNU-ALL: Dynamic section at offset {{.*}} contains {{.*}} entries:
+# GNU-ALL: Program Headers:
----------------
jhenderson wrote:
> These two appear to be missing from the LLVM list above?
Yes. LLVM output seems to be inconsistent, incomplete and ugly sometimes. I'd review and refine it separatelly.


================
Comment at: test/tools/llvm-readobj/all.test:33-34
+# GNU-ALL: Version needs section '.gnu.version_r' contains {{.*}} entries:
+# GNU-ALL: COMDAT group section [    {{.*}}] `.group' [foo] contains {{.*}} sections:
+# GNU-ALL: Histogram for bucket list length (total of 1 buckets)
+# GNU-ALL: Displaying notes found at file offset {{.*}} with length {{.*}}:
----------------
jhenderson wrote:
> These two are also missing from the LLVM list.
The same.


================
Comment at: test/tools/llvm-readobj/all.test:49
+    Relocations:
+  - Name:    .gnu.version
+    Type:    SHT_GNU_versym
----------------
jhenderson wrote:
> I take it the version stuff is needed to make GNU mode print anything?
Yep, GNU style prints nothing if there is no version section.
See: https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L3785


================
Comment at: test/tools/llvm-readobj/all.test:63
+        Entries: []
+  - Name: .group
+    Type: SHT_GROUP
----------------
jhenderson wrote:
> Do we need group information to print a header?
Yes.
Logic is:
1) Collect all group sections:
    https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L2842
2) Dump them (header is printed on this step):
    https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L2886


================
Comment at: test/tools/llvm-readobj/all.test:75-76
+    Entries:
+      - Tag:   DT_HASH
+        Value: 0x1100
+      - Tag:   DT_NULL
----------------
jhenderson wrote:
> Can we just get away with the DT_NULL tag, or is DT_HASH required for the hash histogram behaviour?
> is DT_HASH required for the hash histogram behaviour?
Yes, we need it to get the `DT_HASH` content: 
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L1712


================
Comment at: test/tools/llvm-readobj/all.test:86-90
+  - Name:    .eh_frame_hdr
+    Type:    SHT_PROGBITS
+## An arbitrary linker-generated valid content.
+    Content: 011b033b140000000100000000f0ffff30000000
+  - Name:         .eh_frame
----------------
jhenderson wrote:
> Same comments as earlier. Can these be empty?
No. We need to have something valid here, otherwise any
error triggered will fail the dumping.
(e.g. https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/DwarfCFIEHPrinter.h#L127).


================
Comment at: test/tools/llvm-readobj/all.test:100
+## An arbitrary linker-generated valid content.
+    Content: 040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
+Symbols:
----------------
jhenderson wrote:
> This could probably just be an arbitrary note, and much simpler.
Probably. But it is already short enough and I do not want to spend time on optimising it until we have a way to describe
it with YAML. Having a raw content is anyways not optimal. What do you think?


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

https://reviews.llvm.org/D68462