This is an archive of the discontinued LLVM Phabricator instance.

Differential D66476

[llvm-objcopy][test] Add a test to show that argv[0] is included in error/warning messages
ClosedPublic

Authored by MaskRay on Aug 20 2019, 7:43 AM.

Details

Reviewers
abrachet
alexander-shaposhnikov
grimar
jhenderson
rupprecht
• espindola
Commits
rG494a4d16809e: [llvm-objcopy][test] Add a test to show that argv[0] is included in…
rL369392: [llvm-objcopy][test] Add a test to show that argv[0] is included in…
Summary

test/llvm-objcopy/ELF/error-format.test is similar to test/llvm-readobj/error-format.test added in D66425.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 20 2019, 7:43 AM
Herald added a reviewer: • espindola. · View Herald TranscriptAug 20 2019, 7:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, seiya, s.egerton and 4 others. · View Herald Transcript
Harbormaster completed remote builds in B37037: Diff 216146.Aug 20 2019, 7:43 AM
grimar accepted this revision.Aug 20 2019, 7:51 AM

LGTM.

test/tools/llvm-objcopy/ELF/error-format.test
18

Seems not important at all, but EM_RISCV? I'd use something more common instead, probably.

This revision is now accepted and ready to land.Aug 20 2019, 7:51 AM
jhenderson added inline comments.Aug 20 2019, 7:52 AM
test/tools/llvm-objcopy/ELF/error-format.test
9

Maybe worth a comment saying why you don't test llvm-objcopy too?

MaskRay updated this revision to Diff 216151.Aug 20 2019, 8:01 AM

+ ## Currently llvm-objcopy does not issue warnings, so it is not tested.

MaskRay marked 2 inline comments as done.Aug 20 2019, 8:02 AM
MaskRay added inline comments.
test/tools/llvm-objcopy/ELF/error-format.test
18

I think this is probably not important... It just adds some variety for unimportant fields..

Harbormaster completed remote builds in B37039: Diff 216151.Aug 20 2019, 8:05 AM
grimar added inline comments.Aug 20 2019, 8:12 AM
test/tools/llvm-objcopy/ELF/error-format.test
18

OK, I have no strong opinion about this (just EM_RISCV made me wonder why it was used, but it is fine generally).

jhenderson accepted this revision.Aug 20 2019, 8:20 AM

LGTM.

Closed by commit rL369392: [llvm-objcopy][test] Add a test to show that argv[0] is included in… (authored by MaskRay). · Explain WhyAug 20 2019, 8:33 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
test/
tools/
llvm-objcopy/
ELF/
18 lines

Diff 216146

test/tools/llvm-objcopy/ELF/error-format.test

  • This file was added.
## This test shows that we include the tool name in error/warning messages.
# RUN: not llvm-objcopy %S/non-existent 2>&1 | FileCheck --check-prefix=ERR %s -DTOOL=objcopy
# RUN: not llvm-strip %S/non-existent 2>&1 | FileCheck --check-prefix=ERR %s -DTOOL=strip
# ERR: llvm-[[TOOL]]{{(\.exe)?}}: error: '{{.*}}': {{[Nn]}}o such file or directory
# RUN: yaml2obj %s -o %t
# RUN: llvm-strip %t %t 2>&1 | FileCheck --check-prefix=WARN %s
jhendersonUnsubmitted
Done
ReplyInline Actions

Maybe worth a comment saying why you don't test llvm-objcopy too?

jhenderson: Maybe worth a comment saying why you don't test llvm-objcopy too?
# WARN: llvm-strip{{(\.exe)?}}: warning: '{{.*}}' was already specified
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_DYN
Machine: EM_RISCV
grimarUnsubmitted
Not Done
ReplyInline Actions

Seems not important at all, but EM_RISCV? I'd use something more common instead, probably.

grimar: Seems not important at all, but `EM_RISCV`? I'd use something more common instead, probably.
MaskRayAuthorUnsubmitted
Done
ReplyInline Actions

I think this is probably not important... It just adds some variety for unimportant fields..

MaskRay: I think this is probably not important... It just adds some variety for unimportant fields..
grimarUnsubmitted
Not Done
ReplyInline Actions

OK, I have no strong opinion about this (just EM_RISCV made me wonder why it was used, but it is fine generally).

grimar: OK, I have no strong opinion about this (just `EM_RISCV` made me wonder why it was used, but it…