[PATCH] D74595: [llvm-readobj] - Report warnings instead of errors for broken relocations.

[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:3326-3327
+    this->reportUniqueWarning(createError(
+        "error dumping the relocation at offset 0x" +
+        Twine::utohexstr(R.r_offset) + ": " + toString(Target.takeError())));
+  else
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > I'm not sure this error message quite makes sense. To me, I read it as dumping the relocation at file offset 0x1234 (i.e. where the relocation is physically stored in the file), whereas the offset is the section-relative offset.
> > > 
> > > Perhaps this error should print the index of the relocation and its section, i.e. something like "unable to print the relocation with index 4 in .rela.text: ...". I'd also recommend the phrasing I've used there regarding "unable to print" instead of "error dumping".
> > > 
> > > Same comment goes for LLVM style.
> > > 
> > > By the way, would it not make sense to print something like "<?>" or "<corrupt>" etc for the invalid values?
> > > By the way, would it not make sense to print something like "<?>" or "<corrupt>" etc for the invalid values?
> > 
> > Yes. I had an experiment about refactoring the `getRelocationTarget` method (used `getFullSymbolName` for `STT_SECTION` symbols path as it has very similar code). As a result I had 2 warnings of this patch gone and  "<?>" displayed.
> > I think we might want both: i.e. to show a warning about what is wrong about the object parsed and to dump a relocation, using "<?>" if needed. It seems to be a good possible follow-up change.
> > 
> > > Perhaps this error should print the index of the relocation and its section, i.e. something like "unable to print the relocation with index 4 in .rela.text: ...". I'd also recommend the phrasing I've used there regarding "unable to print" instead of "error dumping".
> > 
> > I've applied your suggestion with a change: I've used a section index instead of a section name in the error message:
> > this is more consistent with our others errors. Looks fine?
> Looks fine, if a little verbose. Do you think something like "unable to print relocation 1 in section 2 ..." would be sufficient?
It sounds fine to me. I'll apply.


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

https://reviews.llvm.org/D74595