This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Fix the Assertion failure when providing invalid --debug-vars or --dwarf values
ClosedPublic

Authored by gbreynoo on Oct 20 2021, 2:17 PM.

Details

Summary

As seen in https://bugs.llvm.org/show_bug.cgi?id=52213 llvm-objdump asserts if either the --debug-vars or the --dwarf options are provided with invalid values. As suggested, this fix adds use of a default value to these options and errors when given bad input.

Diff Detail

Event Timeline

gbreynoo created this revision.Oct 20 2021, 2:17 PM
gbreynoo requested review of this revision.Oct 20 2021, 2:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2021, 2:17 PM
jhenderson added inline comments.Oct 21 2021, 12:09 AM
llvm/test/tools/llvm-objdump/X86/debug-vars.yaml
1 ↗(On Diff #381094)

Copy/paste error.

There's already testing in ARM/debug-vars-dwarf4.yaml for the valid values for --debug-vars, so I'd just simplify by putting the error check in there.

llvm/test/tools/llvm-objdump/X86/eh_frame.yaml
1 ↗(On Diff #381094)

I'd suggest calling this test dwarf_frames.yaml, since that's the switch under test.

jhenderson added inline comments.Oct 21 2021, 12:12 AM
llvm/test/tools/llvm-objdump/X86/debug-vars.yaml
1 ↗(On Diff #381094)

To clarify further: I think the test there is just a generic test, so the fact that it's in ARM doesn't mean we need another test case for X86, since the functionality is generic.

llvm/test/tools/llvm-objdump/X86/eh_frame.yaml
1 ↗(On Diff #381094)

Actually, having looked at what testing there already is, I'd consider naming this dwarf_invalid.yaml, and putting it outside the X86 directory: this test contains no X86-specific functionality, so it doesn't need to be there.

17 ↗(On Diff #381094)

I believe you can drop the Machine line, since you're not trying to do anything that needs a target (but you might not be able to).

gbreynoo updated this revision to Diff 381543.Oct 22 2021, 7:33 AM

Updated after James' comments and fixed the format issue.

gbreynoo marked 5 inline comments as done.Oct 22 2021, 7:33 AM
jhenderson added inline comments.Nov 1 2021, 4:17 AM
llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s
40

I don't think this is going to do what you think it will.

I think you're trying to feed the llvm-mc output into llvm-objdump (the - means read input from stdin), but you aren't piping any llvm-mc output into llvm-objdump.

llvm/test/tools/llvm-objdump/dwarf_invalid.yaml
2

This comment is out-of-date, given the contents and test name.

There is already testing of --dwarf=frames output for llvm-objdump. You only need to check the invalid value case.

gbreynoo updated this revision to Diff 384035.Nov 2 2021, 4:08 AM

Fixed comment and added input to llvm-objdump call as suggested by James.

gbreynoo marked 2 inline comments as done.Nov 2 2021, 4:10 AM

I have also removed the successful test case from dwarf_invalid.yaml.

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s
40

As the commandline parsing gave the error before the validity of the input was checked I didn't think we needed this line. To avoid confusion I have added the input.

jhenderson accepted this revision.Nov 2 2021, 4:18 AM

LGTM, with nit.

llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s
40

We want to ensure that there are no other errors being emitted too, otherwise, we don't know that this error actually prevents continuation of the process, hence we need a valid input.

llvm/test/tools/llvm-objdump/dwarf_invalid.yaml
2

Nit: add missing trailing full stop.

This revision is now accepted and ready to land.Nov 2 2021, 4:18 AM
gbreynoo updated this revision to Diff 384478.Nov 3 2021, 8:59 AM
gbreynoo marked an inline comment as done.

Fixed James' Nit and reduced the multiple identical calls to llvm-mc

jhenderson accepted this revision.Nov 3 2021, 9:07 AM

LGTM again.

jhenderson added inline comments.Nov 4 2021, 1:03 AM
llvm/test/tools/llvm-objdump/ELF/ARM/debug-vars-dwarf4.s
14

Please indent this and all similar lines in this file. The logic is that by indenting it, you show that this is a continuation of the previous RUN line.

gbreynoo marked 2 inline comments as done.Nov 4 2021, 4:05 AM