[PATCH] D93362: [llvm-elfabi] Support ELF file that lacks .gnu.hash section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Is it important that the .dynsym is used directly when .hash/.gnu.hash is available? At the moment, your test doesn't really show whether it is used or whether you go straight to the dynamic tags for details, I believe - to do that, you'd need to have one given bogus values for the size, and show that this is not sued. Additionally, you have no testing for your error messages, nor do you have testing that shows that the .gnu.hash is used above the .hash section.



================
Comment at: llvm/include/llvm/Object/ELF.h:541-542
+  while (It <= BufEnd && (*It & 1) == 0) {
+    LastSymIdx++;
+    It++;
+  }
----------------
LLVM usually uses preincrement (i.e. `++LastSymIdx` etc).


================
Comment at: llvm/include/llvm/Object/ELF.h:547
+        object_error::parse_failed,
+        "malformed ELF, .gnu.hash section does not have a boundary");
+  }
----------------
I think this message would be better off being:

"no terminator found for GNU hash section before buffer end"

I don't think saying "malformed ELF" adds anything to the message. Also, don't use ".gnu.hash" unless you know that is definitely the section's name (it could be other names too technically). Ideally use the section name itself if it is easily available. Otherwise, just refer to it in general terms as in the example.


================
Comment at: llvm/include/llvm/Object/ELF.h:566
+            object_error::parse_failed,
+            "malformed ELF, sh_size % sh_entsize is not 0");
+      }
----------------
Similar to above, but also I'd include the sh_size and sh_entsize values in this message, e.g. like the following pseudo code.

`SecName + " section has sh_size (" + Sec.sh_size + ") % sh_entsize (" + Sec.sh_size + ") that is not 0"`


================
Comment at: llvm/include/llvm/Object/ELF.h:602
+    return getDynSymtabSizeFromGnuHash<ELFT>(*Table,
+                                             (void *)this->Buf.bytes_end());
+  }
----------------
I don't think you need this cast?


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:1
+## Test reading ELF with .dynsym under following conditions
+##  * Section headers are available
----------------



================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:2
+## Test reading ELF with .dynsym under following conditions
+##  * Section headers are available
+##  * Section headers are stripped but .gnu.hash section is available.
----------------



================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:3-4
+##  * Section headers are available
+##  * Section headers are stripped but .gnu.hash section is available.
+##  * Section headers are stripped but .hash section is available.
+
----------------
Alternatively, refer to them by their dynamic tags:
```
##  * Section headers are stripped but there is a DT_GNU_HASH dynamic tag.
##  * Section headers are stripped but there is a DT_HASH dynamic tag.
```
I think I prefer this form.


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:10
+# RUN: yaml2obj --docnum=1 %s -o %tw.gnu.hash
+# RUN: llvm-strip --strip-sections %tw.gnu.hash
+# RUN: llvm-elfabi --elf %tw.gnu.hash --emit-tbe=- | FileCheck %s
----------------
You don't need to call llvm-strip - yaml2obj can emit an object without the section header table, as pointed out in my earlier comment:

>>! In D93362#2486135, @jhenderson wrote:
> For example, yaml2obj has recently gained the ability to not emit the section header table, rather than needing to use llvm-strip to remove it. See llvm\test\tools\yaml2obj\ELF\section-headers.yaml for an example.



================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:39-41
+  - Name:            .gnu.hash
+    Type:            SHT_GNU_HASH
+    Flags:           [ SHF_ALLOC ]
----------------



================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:140-141
+        Value: 0x400
+      - Tag:   DT_HASH
+        Value: 0x600
+      - Tag:   DT_NULL
----------------
By using yaml2obj macros, you can avoid having two nearly-identical YAML inputs. See the `-D` option in yaml2obj. This will also work with the "section headers present/not present options".


================
Comment at: llvm/test/tools/llvm-elfabi/read-elf-dynsym.test:169-175
+# CHECK:      --- !tapi-tbe
+# CHECK-NEXT: TbeVersion:      1.0
+# CHECK-NEXT: Arch:            AArch64
+# CHECK-NEXT: Symbols:
+# CHECK-NEXT:  bar:             { Type: Object, Size: 0 }
+# CHECK-NEXT:  foo:             { Type: Func }
+# CHECK-NEXT: ...
----------------
I think this should be above the YAML input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93362