Page MenuHomePhabricator

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

Authored by MaryamBen on Jul 26 2021, 2:49 AM.

Details

Summary

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

Diff Detail

Event Timeline

MaryamBen requested review of this revision.Jul 26 2021, 2:49 AM
MaryamBen created this revision.
jhenderson added inline comments.Jul 27 2021, 12:29 AM
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 marked 2 inline comments as done.Jul 28 2021, 1:04 AM
MaryamBen added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
139

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

jhenderson added inline comments.Jul 29 2021, 12:08 AM
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.

MaryamBen updated this revision to Diff 363028.Jul 30 2021, 4:07 AM
MaryamBen marked an inline comment as done.
MaryamBen marked an inline comment as done.
jhenderson added inline comments.Aug 2 2021, 3:37 AM
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 updated this revision to Diff 363436.Aug 2 2021, 4:25 AM
MaryamBen marked 6 inline comments as done.
MaryamBen added inline comments.
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 added inline comments.Aug 3 2021, 1:52 AM
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.

MaryamBen updated this revision to Diff 363720.Aug 3 2021, 6:25 AM
MaryamBen marked 3 inline comments as done.

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.

MaryamBen updated this revision to Diff 364008.Aug 4 2021, 2:04 AM
MaryamBen marked 3 inline comments as done.
jhenderson added inline comments.Aug 4 2021, 2:10 AM
llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
2

Use double hash for true comment markers in these tests.

4

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

MaryamBen updated this revision to Diff 364014.Aug 4 2021, 2:32 AM
MaryamBen marked 2 inline comments as done.Aug 4 2021, 2:36 AM
MaryamBen added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
4

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

jhenderson accepted this revision.Aug 4 2021, 3:06 AM

Yup, LGTM with one nit.

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
2
This revision is now accepted and ready to land.Aug 4 2021, 3:06 AM
MaryamBen updated this revision to Diff 364033.Aug 4 2021, 3:56 AM
MaryamBen marked an inline comment as done.
MaryamBen marked an inline comment as done.
sfertile accepted this revision as: sfertile.Aug 17 2021, 8:00 AM
sfertile added a subscriber: 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
9

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

MaryamBen marked an inline comment as done.

Hi,

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

This revision was automatically updated to reflect the committed changes.