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

[George Rimar via Phabricator via llvm-commits]

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)
----------------
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`.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3979
+    if (uintptr_t(VerdefBuf) % sizeof(uint32_t) != 0)
+      return createError("SHT_GNU_verdef section: found an unaligned version "
+                         "definition entry");
----------------
MaskRay wrote:
> I guess this should be called "misaligned", not "unaligned", but I am not a native speaker and not very confident about this...
> 
> Not checking "misaligned auxiliary entry" should be fine.
> I guess this should be called "misaligned", not "unaligned", but I am not a native speaker and not very confident about this...

Perhaps "misaligned" is correct, since "If something is misaligned, then there's a correct alignment that has not been adhered to" and we have one in mind.
(https://www.reddit.com/r/grammar/comments/6x1sov/difference_between_unaligned_and_misaligned/)



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5825
+
+    if (D.AuxV.empty())
+      continue;
----------------
MaskRay wrote:
> For consistency, shall we simply drop this `if` statement to print an empty list?
I think yes. I was going to suggest a follow-up for this. I find it strange to print or not print a tag basing on a "if empty" condition.
It's like if we would drop printing "Alignment: 0", just because it is not set. And it can be a problem for machine parsing,
so I believe we just should always print tags..

But I did not do it in this patch, because this introduces more changes in the existent tests, which
are probably unrelated to this patch. (I wasn't happy to change `Predecessor` -> `Predecessors` either, but I had to).



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

https://reviews.llvm.org/D70495