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

[Esme]

Summary

Followup for D105522

[Esme]

LGTM. Thanks for doing this!

llvm/tools/llvm-readobj/ObjDumper.cpp

57

[jhenderson]

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]

llvm/tools/llvm-readobj/ObjDumper.cpp

57

@Esme Would you like to follow up on this?

[Esme]

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]

The warning output would be:

StringTable {
llvm-readobj: warning: '[[FILE]]': offset (0x4) is past the end of the contents size (0x0)
}

[Esme]

Add an early out if the string list is empty.

[jhenderson]

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]

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

[jhenderson]

In D107398#2930618, @Esme wrote:

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.

llvm/tools/llvm-readobj/ObjDumper.cpp

 
 static void printAsPrintable(raw_ostream &W, const uint8_t *Start, size_t Len) {
   for (size_t i = 0; i < Len; i++)
     W << (isPrint(Start[i]) ? static_cast<char>(Start[i]) : '.');
 }
 
 void ObjDumper::printAsStringList(StringRef StringContent,
                                   size_t StringDataOffset) {
-  if (StringContent.size() < StringDataOffset) {
-    reportUniqueWarning("error: offset is out of string contents");
+  size_t StrSize = StringContent.size();
+  if (StrSize == 0)
+    return;
+  if (StrSize < StringDataOffset) {
+    reportUniqueWarning("offset (0x" + Twine::utohexstr(StringDataOffset) +
+                        ") is past the end of the contents (size 0x" +
+                        Twine::utohexstr(StrSize) + ")");
     return;
   }
+
   const uint8_t *StrContent = StringContent.bytes_begin();
   // Some formats contain additional metadata at the start which should not be
   // interpreted as strings. Skip these bytes, but account for them in the
   // string offsets.
   const uint8_t *CurrentWord = StrContent + StringDataOffset;
   const uint8_t *StrEnd = StringContent.bytes_end();
 
   while (CurrentWord <= StrEnd) {