This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj][XCOFF] Warn about invalid offset
ClosedPublic

Authored by Esme on Aug 3 2021, 1:15 PM.

Diff Detail

Event Timeline

vitalybuka created this revision.Aug 3 2021, 1:15 PM
vitalybuka requested review of this revision.Aug 3 2021, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2021, 1:15 PM
Esme accepted this revision.Aug 3 2021, 7:13 PM

LGTM. Thanks for doing this!

llvm/tools/llvm-readobj/ObjDumper.cpp
57
This revision is now accepted and ready to land.Aug 3 2021, 7:13 PM
This revision was automatically updated to reflect the committed changes.
vitalybuka marked an inline comment as done.

I'd request changes if this hadn't landed...

reportUniqueWarning automatically prepends warning to the start of the message. If you actually look at your output with this current patch, you'd get warning: error: ... which is clearly not whwat you want.

llvm/tools/llvm-readobj/ObjDumper.cpp
57

Please change to "offset X is past the end of the contents (size Y)" with X and Y replaced with the respective values. Errors and warnings should provide useful additional context.

Also, I don't think the case of no string table should be picked up by this warning: in such a case, the behaviour should be to not dump anything (apart from the StringTable {}). From my understanding no string table is a perfectly valid XCOFF construct, and should not result in any warnings or errors.

What this warning SHOULD pick up is the case where the section is too small for the offset.

vitalybuka added inline comments.Aug 4 2021, 12:43 AM
llvm/tools/llvm-readobj/ObjDumper.cpp
57

@Esme Would you like to follow up on this?

Esme added inline comments.Aug 4 2021, 12:47 AM
llvm/tools/llvm-readobj/ObjDumper.cpp
57

Yes, of course.@vitalybuka

Thanks @jhenderson, I think the warning report should be added in this function and I will change the warning message according to your comments.

From my understanding no string table is a perfectly valid XCOFF construct, and should not result in any warnings or errors.

Yes, as for XCOFF, I've added an early out in rG737e27f6236f.

Esme reopened this revision.Aug 4 2021, 1:15 AM
This revision is now accepted and ready to land.Aug 4 2021, 1:15 AM
Esme commandeered this revision.Aug 4 2021, 1:15 AM
Esme edited reviewers, added: vitalybuka; removed: Esme.
This revision now requires review to proceed.Aug 4 2021, 1:15 AM
Esme updated this revision to Diff 363994.Aug 4 2021, 1:18 AM

The warning output would be:

StringTable {
llvm-readobj: warning: '[[FILE]]': offset (0x4) is past the end of the contents size (0x0)
}
Esme updated this revision to Diff 364004.Aug 4 2021, 1:41 AM

Add an early out if the string list is empty.

vitalybuka accepted this revision.Aug 4 2021, 11:19 AM
This revision is now accepted and ready to land.Aug 4 2021, 11:19 AM

With this version of the patch, we don't need the check added in rG737e27f6236f. By reverting that, we can then test this warning properly, with a string table that is not the right size (e.g. 3 bytes for XCOFF).

llvm/tools/llvm-readobj/ObjDumper.cpp
58

Personally, I find it clearer if we're explicit about this comparison.

62
Esme added a comment.Aug 6 2021, 1:53 AM

I will commit this patch first and add a test case after rG737e27f6236f is reverted and D107421 is ready.

This revision was automatically updated to reflect the committed changes.

I will commit this patch first and add a test case after rG737e27f6236f is reverted and D107421 is ready.

Yes, that sounds fine. LGTM, with the requested changes.