[PATCH] D70495: [llvm-readobj/llvm-readelf] - Reimplement dumping of the SHT_GNU_verdef section.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3954
+
+    // FIXME: this error is untested.
+    if (uintptr_t(VerdauxBuf) % sizeof(uint32_t) != 0)
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > MaskRay wrote:
> > > > We should probably only use FIXME for real code problems. Here we should "TODO".
> > > > 
> > > > Actually I think the alignment check should be done outside the function. Maybe we should check the alignments of both `VerdefBuf` and `D->vd_aux`.
> > > > We should probably only use FIXME for real code problems. Here we should "TODO".
> > > Done.
> > > 
> > > > Actually I think the alignment check should be done outside the function. 
> > > Done.
> > > 
> > > >Maybe we should check the alignments of both VerdefBuf and D->vd_aux.
> > > This is checked already:
> > > 1) `VerdefBuf` is checked to be 4-bytes aligned. 
> > > 2) I do `VerdauxBuf = VerdefBuf + D->vd_aux` and check `VerdauxBuf`. This checks `D->vd_aux`.
> > > 3) I do `VerdefBuf += D->vd_next` and check `VerdefBuf`. This checks `D->vd_next`.
> > > 
> > Is there a reason we can't just test this as part of this change?
> > Is there a reason we can't just test this as part of this change?
> I believe it is impossible to use yaml2obj here. So the only way to do that is probably to craft and checking a binary. Do you want me to do that?
checking -> commit


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

https://reviews.llvm.org/D70495