Page MenuHomePhabricator

[llvm-readobj] Prepend argv[0] to error messages
ClosedPublic

Authored by MaskRay on Aug 19 2019, 9:20 AM.

Details

Summary

Currently, we report:

error: ...

Prepend argv[0] (tool name):

llvm-readobj: error: ...

This is consistent with most GNU binutils/clang/lld. It gives a bit more
context in a long build log.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 19 2019, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 9:20 AM
rupprecht accepted this revision.Aug 19 2019, 10:29 AM
This revision is now accepted and ready to land.Aug 19 2019, 10:29 AM
grimar added inline comments.Aug 20 2019, 1:12 AM
test/tools/llvm-readobj/error-format.test
7 ↗(On Diff #215925)

It fails under windows for me. Because of .exe suffix:

D:\Work2\llvm\llvm\test\tools\llvm-readobj\error-format.test:6:12: error: READELF: expected string not found in input
# READELF: llvm-readelf: error: '{{.*}}':
           ^
<stdin>:2:1: note: scanning from here
d:\work2\llvm\llvm_build\debug\bin\llvm-readelf.exe: error: 'D:\Work2\llvm\llvm\
test\tools\llvm-readobj/non-existent': no such file or directory
^
grimar added inline comments.Aug 20 2019, 1:16 AM
test/tools/llvm-readobj/error-format.test
7 ↗(On Diff #215925)

The following helps:

# READELF: llvm-readelf{{(.exe)?}}: error: '{{.*}}':
# READOBJ: llvm-readobj{{(.exe)?}}: error: '{{.*}}':

(no more other readelf tests are failing for me, btw).

I see that out other tests sometimes using something like: llvm-ar{{(.exe|.EXE)?}},
but I am not sure why handling the uppercase .EXE is useful/important.

MaskRay updated this revision to Diff 216065.Aug 20 2019, 1:27 AM

Use llvm-readelf{{(\.exe)?}}: error: '{{.*}}': for Windows. Thanks grimar for pointing that out.

The concrete error message is OS specific "No such file or ..." so we can't test it.

MaskRay marked 3 inline comments as done.Aug 20 2019, 1:33 AM
MaskRay added inline comments.
test/tools/llvm-readobj/error-format.test
7 ↗(On Diff #215925)

Thanks for testing! I was indeed thinking what Windows would do...

So the error message is ineed OS dependent. On Linux:

% errno 2
ENOENT 2 No such file or directory

On Windows it is not capitalized: no such file or directory

I think .EXE is probably 8.3 filename remnant from DOS or Windows 95... I think it is not useful unless the basename also follows 8.3, i.e. LLVM-R~1.EXE LLVM-R~2.EXE

grimar accepted this revision.Aug 20 2019, 2:08 AM

LGTM, I can confirm it works now. A minor suggestion is inlined.

test/tools/llvm-readobj/error-format.test
7 ↗(On Diff #215925)

Some of the our tests use: {{[N|n]}}o such file or directory.

Perhaps I would do that:

# READELF: llvm-readelf{{(\.exe)?}}: error: '{{.*}}': {{[N|n]}}o such file or directory
# READOBJ: llvm-readobj{{(\.exe)?}}: error: '{{.*}}': {{[N|n]}}o such file or directory
MaskRay updated this revision to Diff 216078.Aug 20 2019, 2:13 AM
MaskRay marked 2 inline comments as done.

Add {{[N|n]}}o such file or directory

jhenderson added inline comments.Aug 20 2019, 3:03 AM
test/tools/llvm-readobj/error-format.test
1 ↗(On Diff #216078)

How about a warning case too?

MaskRay updated this revision to Diff 216091.Aug 20 2019, 3:24 AM

Add a warning test

MaskRay marked an inline comment as done.Aug 20 2019, 3:25 AM
jhenderson added inline comments.Aug 20 2019, 4:29 AM
test/tools/llvm-readobj/error-format.test
6–7 ↗(On Diff #216091)

Have you considered folding these two together using FileCheck's -D option?

# RUN: ... | FileCheck --check-prefix=ERR %s -DTOOL=readelf
# RUN: ... | FileCheck --check-prefix=ERR %s -DTOOL=readobj

# ERR: llvm-[[TOOL]]{{(\.exe)?}}: ...

Same goes for the warning case.

9–12 ↗(On Diff #216091)

I think we should probably just use yaml2obj here instead of relying on pre-built binaries. I don't feel strongly about it though.

MaskRay updated this revision to Diff 216101.Aug 20 2019, 4:48 AM
MaskRay marked 2 inline comments as done.

Use -DTOOL=

MaskRay added inline comments.Aug 20 2019, 5:17 AM
test/tools/llvm-readobj/error-format.test
9–12 ↗(On Diff #216091)

This is the simplest way to get a warning. I don't want to be distracted by the lengthy YAML...

jhenderson added inline comments.Aug 20 2019, 5:27 AM
include/llvm/BinaryFormat/ELF.h
1422 ↗(On Diff #216101)

Unrelated?

grimar added inline comments.Aug 20 2019, 5:29 AM
test/tools/llvm-readobj/error-format.test
9–12 ↗(On Diff #216091)

If we do not want to convert the precomipled binaries in test cases to YAML generally, then I'll stop my attemps to improve that. Generally I supposed that having precompiled binaries is too bad to have.

MaskRay updated this revision to Diff 216110.Aug 20 2019, 5:36 AM
MaskRay marked 4 inline comments as done.

Use yaml2obj

grimar added inline comments.Aug 20 2019, 5:39 AM
test/tools/llvm-readobj/error-format.test
19 ↗(On Diff #216110)

Thanks :)

This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
llvm/trunk/tools/llvm-readobj/llvm-readobj.cpp
708

lld and clang only print the basename, see D49189 D49189 for lld for example (clang has similar code).

Sorry the 2nd link should've been D51133.