[PATCH] D64470: [Object/ELF] - Improve error reporting for notes.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: include/llvm/Object/ELF.h:205
+    if (Phdr.p_type != ELF::PT_NOTE)
+      llvm_unreachable("Phdr is not of type PT_NOTE");
+
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I'm not sure making this llvm_unreachable is correct: if you are using libObject as a library, you could pass in an invalid Phdr, which should report an error, not an assertion.
> > Note that method name is `notes_begin`. I.e it expects to accept note `Phdr` and all
> > code we have do this check now.
> > 
> > I think that It is obviously a error in the code to pass `Phdr` not of type `PT_NOTE` here.
> > Showing such error to user is unuseful, code just should never do that first of all.
> Hmmm... okay, I'm not really convinced, but I don't feel strongly about it.
> 
> Either way, this can be simplified to `assert(Phdr.p_type != ELF::PT_NOTE && "Phdr is not of type PT_NOTE")`.
+1 for assert


================
Comment at: include/llvm/Object/ELF.h:225
+    if (Shdr.sh_type != ELF::SHT_NOTE)
+      llvm_unreachable("Shdr is not of type SHT_NOTE");
+
----------------
jhenderson wrote:
> Same comment about llvm_unreachable applies here.
I think llvm_unreachable should be fine. The precondition is that the section type should match.

How about

`assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");`


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

https://reviews.llvm.org/D64470