[PATCH] D64470: [Object/ELF] - Improve error reporting for notes.
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 04:33:04 PDT 2019
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
More information about the llvm-commits
mailing list