[PATCH] D110320: [XCOFF] Improve error message context.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:728
   if (Index >= NumberOfSymTableEntries)
-    return errorCodeToError(object_error::invalid_symbol_index);
+    return createError("error: symbol index " + Twine(Index) +
+                       " exceeds symbol count " +
----------------
Don't add "error:" to the error message text here. If the tool is using the correct LLVM functions for reporting errors, this will be added automatically. Otherwise, it will either have a good reason not to add it at the point where the error is actually printed (e.g. because the tool is matching the format of an existing tool, such as GNU binutils), or it needs fixing at that point. It definitely shouldn't be fixed at this low a level.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:894
   if (ImportTablePtr[LengthOfImportFileTable - 1] != '\0')
-    return createStringError(
-        object_error::parse_failed,
-        "the import file table must end with a null terminator");
+  if (ImportTablePtr[LengthOfImportFileTable - 1] != '\0')
+    return createError(
----------------
Looks like a copy/paste error here?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+        toString(ImportTableOrErr.takeError()) +
+        ": the ImportTable with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
----------------
Esme wrote:
> jhenderson wrote:
> > Slightly surprised to see the name written as "ImportTable" here, but you don't use e.g. "StringTable". Is "ImportTable" how it is written in the XCOFF spec? What about string tables?
> It's called  `Import File ID Name Table` in  the XCOFF spec. To make messages consistent with `string table`, I will use `import file name table` here, what do you think?
Without knowing the content, it's a little tricky to suggest what makes sense. What is actually in the table?


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test:26
 # INVALID-SYM-NEXT:   Section (index: 1) .text {
-# INVALID-SYM-NEXT:     warning: '[[FILE]]': Invalid symbol index
+# INVALID-SYM-NEXT:     warning: '[[FILE]]': symbol index 33 exceeds symbol number 0
 # INVALID-SYM-NEXT:   }
----------------
jhenderson wrote:
> Higuoxing wrote:
> > jhenderson wrote:
> > > Higuoxing wrote:
> > > > 
> > > Don't do this. Error messages should not have a leading capital letter as per the style guide.
> > > 
> > > Do consider prefixing this check with "error:", e.g. "error: the section index ..."
> > Sorry for the incorrect comments. TIL.
> It's okay! It would have been a good catch if there weren't a style guide rule :)
> Do consider prefixing this check with "error:", e.g. "error: the section index ..."

I was specifically referring to test checks in general, i.e. when checking warnings and errors, check for any "warning:"/"error:" that should be present.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml:8
 
-# ERROR1: Invalid section index
+# ERROR1: the section index (2) is invalid
 
----------------
jhenderson wrote:
> Higuoxing wrote:
> > 
> Ditto.
This is an example of where I'd expect there to be "error:" before the message. Don't update obj2yaml in this patch though if the prefix is missing.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml:22
 
-# ERROR2: Invalid data was encountered while parsing the file
+# ERROR2: entry with offset 0x4 in a string table with size 0x4 is invalid
 
----------------
jhenderson wrote:
> Higuoxing wrote:
> > 
> Ditto.
See my other inline comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110320