[PATCH] D110320: [XCOFF] refactor error reporting.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D110320#3019605 <https://reviews.llvm.org/D110320#3019605>, @qiucf wrote:

> Can I assume no tests will be affected if this is a refactor patch?

@Esme, the patch summary needs changing. "Refactor" implies there is no observable difference in behaviour. Perhaps "Improve error message context".

In D110320#3019656 <https://reviews.llvm.org/D110320#3019656>, @Higuoxing wrote:

> The code change generally looks good to me. But please make the error messages consistent, see my inline comments.

See my inline comment re. error message styles. Unfortunately, some errors and error code messages are using the wrong style, so this can make things look inconsistent. Stretch goal for someone who has the time is to fix that!



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:46
 
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err, object_error::parse_failed);
----------------
qiucf wrote:
> Seems `inline` here not necessary.
+1 `inline` is only necessary in header files.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:729
+    return createError("symbol index " + Twine(Index) +
+                       " exceeds symbol number " +
+                       Twine(NumberOfSymTableEntries));
----------------
(or "number of symbols")


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:811-814
+        toString(RelocationOrErr.takeError()) + ": relocations with offset 0x" +
+        Twine::utohexstr(Sec.FileOffsetToRelocationInfo) + " and size 0x" +
+        Twine::utohexstr(NumRelocEntries * sizeof(Reloc)) +
+        " goes past the end of the file");
----------------
": relocation data with ... goes past" or ": relocations with ... go past"


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+        toString(ImportTableOrErr.takeError()) +
+        ": the ImportTable with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
----------------
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?


================
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");
+    return createError("the import file table must end with a null terminator");
 
----------------
Add offset and size here too.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:929-932
+                         ": section headers with offset 0x" +
+                         Twine::utohexstr(CurOffset) + " and size 0x" +
+                         Twine::utohexstr(SectionHeadersSize) +
+                         " goes past the end of the file");
----------------
"section header table ... goes past" or "section headers ... go past"


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1053-1054
 
-  return createStringError(
-      object_error::parse_failed,
-      "a csect auxiliary entry is not found for symbol \"" + *NameOrErr + "\"");
+  return createError("a csect auxiliary entry is not found for symbol \"" +
+                     *NameOrErr + "\"");
 }
----------------
Can there be more than one symbol with the same name? If so, you need the symbol index too.

Also "is not found" -> "has not been found"


================
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:   }
----------------
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 ..."


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml:8
 
-# ERROR1: Invalid section index
+# ERROR1: the section index (2) is invalid
 
----------------
Higuoxing wrote:
> 
Ditto.


================
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
 
----------------
Higuoxing wrote:
> 
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110320