[PATCH] D106783: [AIX][XCOFF][llvm-readobj] Replace unwrapOrError with reportUniqueWarning

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/report_warning.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --relocs --expand-relocs %t 2>&1 | FileCheck %s
----------------
Two problems with this file name now: 1) use `-` not `_` and 2) the test name needs to say which sort of warning this is testing, namely something like `relocation-invalid.yaml`.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/report_warning.yaml:2-6
+# RUN: llvm-readobj --relocs --expand-relocs %t 2>&1 | FileCheck %s
+
+# CHECK:      Relocations [
+# CHECK-NEXT:   Section (index: 1) .text {
+# CHECK-NEXT:     {{.*}} warning: {{.*}} Invalid symbol index
----------------
You can test the full path by doing the inline edit.

You can also omit the leading `{[.*}}`, since FileCheck doesn't require full line matches by default.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:139
+    if (Error E = ErrOrRelocations.takeError())
+      reportUniqueWarning(std::move(E));
+
----------------
MaryamBen wrote:
> jhenderson wrote:
> > MaryamBen wrote:
> > > jhenderson wrote:
> > > > 1) Is this tested?
> > > > 2) You'll need to break out of or continue this loop here, as `*ErrOrRelocations` will result in referencing invalid memory if there is an error. (reportUniqueWarning reports the warning and continues).
> > > I don't know how to test it. 
> > > None of the existing tests reports the UniqueWarning
> > To test this, you'll probably need to craft an XCOFF object (probably using yaml2obj) to trigger the report of the error from `Obj.relocations`. You may need to expand on yaml2obj's existing behaviour to allow creating an invalid input.
> > 
> > Just because something isn't tested already, doesn't mean it shouldn't be now, and this is a good chance to add the testing.
> I was not able to introduce an error to check this warning but I checked the other one.
I've skimmed the XCOFFObjectFile code, and it looks like there should be a couple of possible ways of triggering an error in it. It might require enhancements to yaml2obj to allow creating an invalid object. There are many good examples of prior art for ELF of how to achieve this sort of thing.


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

https://reviews.llvm.org/D106783