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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, aside from a possible new name as suggested inline.

Also, remove "trying" from the patch description - either the patch does something or it doesn't. There is no try.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:887
+        toString(ImportTableOrErr.takeError()) +
+        ": the ImportTable with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
----------------
Esme wrote:
> 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
> 
> 
> 
Okay, how about simply "import file 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:   }
----------------
Esme wrote:
> 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.
Yeah, exactly. Here, you have the prefix check.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml:8
 
-# ERROR1: Invalid section index
+# ERROR1: the section index (2) is invalid
 
----------------
Esme wrote:
> 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.
Thanks. Looks like obj2yaml doesn't use the standard LLVM functions for reporting errors. Perhaps something to change in the future, if you fancy it, so that the output is simply "error: [[FILE]]: ...".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110320