Page MenuHomePhabricator

[XCOFF] Improve error message context.
ClosedPublic

Authored by Esme on Sep 23 2021, 4:39 AM.

Details

Summary

This patch is trying to report error messages in more detail for XCOFF interfaces.

Diff Detail

Event Timeline

Esme created this revision.Sep 23 2021, 4:39 AM
Esme requested review of this revision.Sep 23 2021, 4:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 4:39 AM
qiucf added a subscriber: qiucf.Sep 23 2021, 7:38 PM

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

llvm/lib/Object/XCOFFObjectFile.cpp
46

Seems inline here not necessary.

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

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26
llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml
8
22

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".

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!

llvm/lib/Object/XCOFFObjectFile.cpp
46

+1 inline is only necessary in header files.

842

(or "number of symbols")

924–927

": relocation data with ... goes past" or ": relocations with ... go past"

1000

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?

1007–1011

Add offset and size here too.

1046–1049

"section header table ... goes past" or "section headers ... go past"

1172–1175

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"

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

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 ..."

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml
8

Ditto.

22

Ditto.

Higuoxing added inline comments.Sep 24 2021, 1:47 AM
llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

Sorry for the incorrect comments. TIL.

jhenderson added inline comments.Sep 24 2021, 1:54 AM
llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

It's okay! It would have been a good catch if there weren't a style guide rule :)

Esme updated this revision to Diff 375508.Sep 28 2021, 2:57 AM
Esme marked 8 inline comments as done.
Esme retitled this revision from [XCOFF] refactor error reporting. to [XCOFF] Improve error message context..
Esme added a reviewer: qiucf.

Address comments.

llvm/lib/Object/XCOFFObjectFile.cpp
1000

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?

jhenderson added inline comments.Sep 29 2021, 12:25 AM
llvm/lib/Object/XCOFFObjectFile.cpp
841–843

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.

1000

Without knowing the content, it's a little tricky to suggest what makes sense. What is actually in the table?

1007–1011

Looks like a copy/paste error here?

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

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.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml
8

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.

22

See my other inline comments.

Esme updated this revision to Diff 375816.Sep 29 2021, 2:31 AM

Address comments.

llvm/lib/Object/XCOFFObjectFile.cpp
841–843

Oh...I See. Sorry that I misunderstood your comments.

1000

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

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

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.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml
8

The prefix in the output does exist, but I missed checking it.

jhenderson accepted this revision.Sep 30 2021, 12:47 AM

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.

llvm/lib/Object/XCOFFObjectFile.cpp
1000

Okay, how about simply "import file table"?

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test
26

Yeah, exactly. Here, you have the prefix check.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml
8

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]]: ...".

This revision is now accepted and ready to land.Sep 30 2021, 12:47 AM
This revision was landed with ongoing or failed builds.Oct 10 2021, 7:55 PM
This revision was automatically updated to reflect the committed changes.

The createError function introduces here breaks the LLVM_ENABLE_MODULES build:

/Users/teemperor/5llvm/llvm-project/llvm/lib/Object/XCOFFObjectFile.cpp:46:14: error: definition with same mangled name '_ZN4llvm6objectL11createErrorERKNS_5TwineE' as another definition
static Error createError(const Twine &Err) {
             ^
/Users/teemperor/5llvm/llvm-project/llvm/include/llvm/Object/ELF.h:84:21: note: previous definition is here
static inline Error createError(const Twine &Err) {
                    ^
1 error generated.

Could we maybe move both definitions into some utility header (both functions seem to do the same thing)?

D111541 should address the issue.