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

[Esme Yi via Phabricator via llvm-commits]

Esme 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 " +
----------------
jhenderson wrote:
> 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.
Oh...I See. Sorry that I misunderstood your comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+        toString(ImportTableOrErr.takeError()) +
+        ": the ImportTable with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
----------------
jhenderson wrote:
> 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?
The Import File ID Name Table is a part of the loader section as in the spec: https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__vra3i31ejbau


> Loader Import File ID Name Table Definition:
> The loader section import file ID name strings of a module provide a list of dependent modules that the system loader must load in order for the module to load successfully. However, this list does not contain the names of modules that the named modules depend on.
> 
> Each entry in the import file ID name table consists of:
> Import file ID path name
> Null delimiter (ASCII null character)
> Import file ID base name
> Null delimiter
> Import file ID archive-file-member name
> Null delimiter





================
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:
> 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.
So the format of `warning: '[[FILE]]': symbol index 33 exceeds symbol count 0` should be fine? It seems that the warning/error messages for ELF, such as llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test, follow this format as well.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml:8
 
-# ERROR1: Invalid section index
+# ERROR1: the section index (2) is invalid
 
----------------
jhenderson wrote:
> 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.
The prefix in the output does exist, but I missed checking it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110320