[AIX][XCOFF][llvm-readobj] Replace unwrapOrError with reportUniqueWarning

[MaryamBen]

Summary

Report warnings rather than errors, so that llvm-readobj doesn't bail out on malformed inputs.

[jhenderson]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

139

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

153

Same comments as above.

[MaryamBen]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

139

I don't know how to test it.
None of the existing tests reports the UniqueWarning

[jhenderson]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

139

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.

[jhenderson]

llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml

1 ↗

(On Diff #363028)

This comment looks like it's been copied from somewhere else and doesn't make sense in this context. Please rewrite it.

The test name suggests this is for generic dumping of 32-bit relocations, but I think your patch is about reporting of warnings, so the test name doesn't make much sense to me.

5–8 ↗

(On Diff #363028)

If this test is just about checking the warnings, you can omit these 4 lines, as they aren't important.

11 ↗

(On Diff #363028)

What's the full message here that you are matching?

17 ↗

(On Diff #363028)

We generally remove execessive whitespace, so that values within a block line up vertically, but as far left as possible.

20 ↗

(On Diff #363028)

I don't think you need this section?

llvm/tools/llvm-readobj/XCOFFDumper.cpp

153

This still seems to be without a test?

[MaryamBen]

llvm/test/tools/llvm-readobj/XCOFF/relocations32.yaml

11 ↗

(On Diff #363028)

The warning occurs due to the invalid symbol index used to obtain the symbol name.

Expected<StringRef> ErrOrSymbolName =
          Obj.getSymbolNameByIndex(Reloc.SymbolIndex);

The full message is the following :
full_path/llvm-readobj: warning: full_path/report_warning.yaml.tmp: Invalid symbol index

llvm/tools/llvm-readobj/XCOFFDumper.cpp

139

I was not able to introduce an error to check this warning but I checked the other one.

[jhenderson]

llvm/test/tools/llvm-readobj/XCOFF/report_warning.yaml

1 ↗

(On Diff #363436)

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.

2–6 ↗

(On Diff #363436)

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.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

139

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.

[jhenderson]

Just some test tidying up now.

llvm/test/tools/llvm-readobj/XCOFF/relocation-invalid.test

1 ↗

(On Diff #363720)

Add a comment to the top of this file explaining the purposes of the tests within.

15–16 ↗

(On Diff #363720)

Delete excessive blank lines at EOF.

llvm/test/tools/llvm-readobj/XCOFF/relocation-symbol-invalid.test

1 ↗

(On Diff #363720)

This can be merged into relocation-invalid.test, so you have two related test cases in the same file.

I'd also name it relocations-invalid.test thinking about it.

[jhenderson]

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

1 ↗

(On Diff #364008)

Use double hash for true comment markers in these tests.

3 ↗

(On Diff #364008)

Give each test case its own comment explaining exactly what is invalid.

[MaryamBen]

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

3 ↗

(On Diff #364008)

I have seen this way of commenting in some ELF tests.
Is it ok now?

[jhenderson]

Yup, LGTM with one nit.

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

1 ↗

(On Diff #364014)

[sfertile]

Hi Maryam.

Do you need someone to commit this for you? I don't think we were waiting for any further approvals (FWIW it LGTM also though). I made one small comment about removing some trailing whitespace, if you need someone to commit for you I can fix that and commit, just let me know.

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

8 ↗

(On Diff #364033)

Real minor nit: there is trailing whitespace at the end of this line.

[MaryamBen]

Hi,

I removed the trailing whitespace.
It would be great if you could commit it, thank you.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

void XCOFFDumper::printRelocations(ArrayRef<XCOFFSectionHeader32> Sections) {
   ListScope LS(W, "Relocations");
   uint16_t Index = 0;
   for (const auto &Sec : Sections) {
     ++Index;
     // Only the .text, .data, .tdata, and STYP_DWARF sections have relocation.
     if (Sec.Flags != XCOFF::STYP_TEXT && Sec.Flags != XCOFF::STYP_DATA &&
         Sec.Flags != XCOFF::STYP_TDATA && Sec.Flags != XCOFF::STYP_DWARF)
       continue;
-    auto Relocations = unwrapOrError(Obj.getFileName(), Obj.relocations(Sec));
+    auto ErrOrRelocations = Obj.relocations(Sec);
+    if (Error E = ErrOrRelocations.takeError()) {
+      reportUniqueWarning(std::move(E));
+      continue;
+    }
+
+    auto Relocations = *ErrOrRelocations;
     if (Relocations.empty())
       continue;
 
     W.startLine() << "Section (index: " << Index << ") " << Sec.getName()
                   << " {\n";
     for (auto Reloc : Relocations) {
-      StringRef SymbolName = unwrapOrError(
-          Obj.getFileName(), Obj.getSymbolNameByIndex(Reloc.SymbolIndex));
+      Expected<StringRef> ErrOrSymbolName =
+          Obj.getSymbolNameByIndex(Reloc.SymbolIndex);
+      if (Error E = ErrOrSymbolName.takeError()) {
+        reportUniqueWarning(std::move(E));
+        continue;
+      }
 
+      StringRef SymbolName = *ErrOrSymbolName;
       DictScope RelocScope(W, "Relocation");
       W.printHex("Virtual Address", Reloc.VirtualAddress);
       W.printNumber("Symbol", SymbolName, Reloc.SymbolIndex);
       W.printString("IsSigned", Reloc.isRelocationSigned() ? "Yes" : "No");
       W.printNumber("FixupBitValue", Reloc.isFixupIndicated() ? 1 : 0);
       W.printNumber("Length", Reloc.getRelocatedLength());
       W.printEnum("Type", (uint8_t)Reloc.Type,
                   makeArrayRef(RelocationTypeNameclass));