[PATCH] D60312: [llvm-readobj] Add GNU style dumper for .gnu.version_d section

Fri Apr 5 03:09:45 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3417

+  const uint8_t *SecStartAddress =
+      reinterpret_cast<const uint8_t *>(Obj->base() + Sec->sh_offset);
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Given that much of this code is a copy of the LLVMStyle, that indicates that it should be shared somehow. I don't know if in this file, or in the Object library somewhere makes sense, but I think the only distinction should be how to print the output.
> Yes, I agree. I would like to improve this in the future.
I don't think we should delay it to the future. It should be done either as part of, or more likely prior to this patch.

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3453
+    if (Verdef->vd_cnt > 2)
+      report_fatal_error("more than one predecessor is not expected");
+
----------------
Higuoxing wrote:
> jhenderson wrote:
> > Higuoxing wrote:
> > > In `GNU readelf`, there's no limit on the number of predecessors. For example, if one symbol has more than one parents, `GNU readelf` will give:
> > > 
> > > ```
> > > Parent 1: VERSION1
> > > Parent 2: VERSION0
> > > ...
> > > ```
> > > 
> > > Shall we do the same thing in `llvm-readelf` ?
> > Is there a good reason to limit it? If not, I don't think we should. However, I get the feeling that this is a different patch, since we should do it in LLVM style too, unless there's a good reason not to.
> I had asked @grimar about this in https://reviews.llvm.org/D21552
> 
> I think it's good to be robust when dumping some uncommon object files.
By robust, I think we should handle all cases we reasonably can. However, as this is already a limitation, I don't think we need to change it in this patch, but it can be in a follow-up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60312