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

[Scott Linder via Phabricator via llvm-commits]

scott.linder 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)
----------------
t-tye wrote:
> Is this constructor needed?
Not strictly, but for code in `ELF.h` it makes the intent of "construct a failing end iterator" clearer, like in `return Elf_Note_Iterator(&Err);`


================
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)
----------------
t-tye wrote:
> Since this iterator constructor always requires an Err should it be passed as Error&?
This was a typo, I was also intending to check the `bool` of the `Error`, not just the `Error *`. Thank you for the catch, I will update the patch.

There are cases (such as constructing the end iterator) where there is no chance for error, and so having a non-nullable reference is not appropriate (and complicates the calling code for no benefit). The alternative is an `Optional<Error &>`, which I can update the patch to use. I could also do the same for the pointer to the `Nhdr`, and in both cases it would make the fact that they are optional clearer.


================
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);
----------------
t-tye wrote:
> 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.
Yes, I would also be fine with this approach, and the checks being in `Note` currently is in part left over from starting with a public constructor for the type.

The reason I did not hoist the checks up into the iterator is that I am not confident we want the constructor to be private. If we are OK with it, I can move the checks up to avoid the extra `size_t` and duplication of bound checks.


https://reviews.llvm.org/D43958