[PATCH] D150022: [Object] Fix handling of Elf_Nhdr with sh_addralign=8

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:319
     }
-    return Elf_Note_Iterator(base() + Phdr.p_offset, Phdr.p_filesz, Err);
+    if (Phdr.p_align > 4 && Phdr.p_align != 8) {
+      Err =
----------------
This will permit p_align values of 2 and 3, and treat them as 4. Should we instead reject those too? The error message also is not techincally accurate ("is not 4 or 8"), since it could be 0 (or 1/2/3) and be handled by this code. I'm okay with that not changing if we are excluding other values, for convenience of wording though.


================
Comment at: llvm/include/llvm/Object/ELF.h:325
+    return Elf_Note_Iterator(base() + Phdr.p_offset, Phdr.p_filesz,
+                             std::max<size_t>(Phdr.p_align, 4), Err);
   }
----------------
Nit: this probably wants to be a `uint64_t` rather than `size_t` max, right (since p_align could be `uint64_t` and therefore a larger type on 32-bit hosts than `size_t`)?


================
Comment at: llvm/include/llvm/Object/ELF.h:344
     }
-    return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, Err);
+    // TODO Be stricter about the alignment
+    if (Shdr.sh_addralign > 4 && Shdr.sh_addralign != 8) {
----------------
Nit: missing ".", and I think the tendency is for "TODO: ", i.e. with colon.

The duplication between this and the Phdr version of this makes me sad :( it feels like we could share the code and therefore avoid duplicating these extra checks.


================
Comment at: llvm/include/llvm/Object/ELFTypes.h:647
   StringRef getDescAsStringRef() const {
-    ArrayRef<uint8_t> Desc = getDesc();
+    ArrayRef<uint8_t> Desc = getDesc(4);
     return StringRef(reinterpret_cast<const char *>(Desc.data()), Desc.size());
----------------
Is 4 here correct?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/note-gnu-property2.s:21
 // LLVM-NEXT:       Property [
-// LLVM-NEXT:         <corrupted GNU_PROPERTY_TYPE_0>
 // LLVM-NEXT:       ]
----------------
Changing the logic means that there is no test coverage that hits the "<corrupted GNU_PROPERTY_TYPE_0>" output, as far as I can tell. Do we need a new test to cover that? It may be worth looking at `note-gnu-property.s` to see if that already covers what the new test covers.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5827
     llvm::function_ref<void(std::optional<StringRef>, typename ELFT::Off,
-                            typename ELFT::Addr)>
+                            typename ELFT::Addr, size_t)>
         StartNotesFn,
----------------
Similar to the earlier comment, should this be `uint64_t`? Same goes for other cases in the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150022