[PATCH] D43958: [llvm-readobj][ELF] Move ELF note parsing into lib/Object

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: include/llvm/Object/ELFTypes.h:643
+  Elf_Note_Iterator_Impl() {}
+  explicit Elf_Note_Iterator_Impl(Error *Err) : Err(Err) {}
+  Elf_Note_Iterator_Impl(const uint8_t *Start, size_t Size, Error *Err)
----------------
Is this constructor needed?


================
Comment at: include/llvm/Object/ELFTypes.h:647
+    assert(Start && "ELF note iterator starting at NULL");
+    assert(!Err && "ELF note iterator starting with an Error");
+    if (RemainingSize == 0)
----------------
Since this iterator constructor always requires an Err should it be passed as Error&?


================
Comment at: include/llvm/Object/ELFTypes.h:651-654
+    if (sizeof(*Nhdr) > RemainingSize)
+      *Err = makeOverflowError();
+    else
+      Nhdr = reinterpret_cast<const Elf_Nhdr_Impl<ELFT> *>(Start);
----------------
An alternative approach is that all checking for a note being legal is done in the iterator constructor and operator++. The check could be factored into a common helper function called by both. That would include ensuring that the RemainingSize is > the *Nhrd size, getting the pointer to the Nhdr, followed by checking that Nhdr->getSize <= RemainingSize (which is the entire size of the note record). That would avoid needing to do any checking in the access functions for the note name or description which avoids the need for MaxSize, and avoids them needing to return an Expected as they will always return valid values.


https://reviews.llvm.org/D43958