[PATCH] D66734: [llvm-readobj/llvm-readelf] - Report a proper warning when dumping a broken dynamic relocation.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Object/ELFTypes.h:253
+        object_error::parse_failed,
+        "st_name (0x" + utohexstr(Offset) +
+            ") is past the end of the string table of size 0x" +
----------------
I think you want to replace `utohextr(Offset)` with `PRIx32` to take advantage of createStringError's printf formatting rules:

```
return createStringError(object_error::parse_failed,
    "st_name (0x" PRIx32 ") is past the end of the string table"
    " of size 0x%zx", Offset, StrTab.size());
```


================
Comment at: test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test:1
+## Check that llvm-readobj/llvm-readelf reports an error when dumping relocations if dynamic
+## symbol name offset is broken (goes past the end of the dynamic symbol string table).
----------------
if a dynamic


================
Comment at: test/tools/llvm-readobj/elf-broken-dynamic-reloc-name.test:14
+
+# GNU:      'RELA' relocation section at offset 0x278 contains 24 bytes:
+# GNU-NEXT: Offset           Info             Type          Symbol's Value   Symbol's Name + Addend
----------------
I think the offset here is irrelevant, right? Can it be replaced with `{{.+}}`?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3537
+                                  const ELFDumper<ELFT> *Dumper,
+                                  const typename ELFT::Rela &R) {
+  uint32_t SymIndex = R.getSymbol(Obj->isMips64EL());
----------------
Maybe R -> Reloc


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3547
+    reportWarning(
+        createError("unable to get name of a dynamic symbol with index " +
+                    Twine(SymIndex) + ": " + toString(ErrOrName.takeError())),
----------------
I'd replace "a dynamic symbol" with "the dynamic symbol"


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

https://reviews.llvm.org/D66734