This is an archive of the discontinued LLVM Phabricator instance.

llvm-dwarfdump: Don't crash if DW_AT_{decl,call}_{file,line} uses signed form
ClosedPublic

Authored by palves on Jul 8 2022, 12:58 PM.

Details

Summary

The DWARF spec says:

Any debugging information entry representing the declaration of an object,
module, subprogram or type may have DW_AT_decl_file, DW_AT_decl_line and
DW_AT_decl_column attributes, each of whose value is an unsigned integer

							 ^^^^^^^^

constant.

If however, a producer happens to emit DW_AT_decl_file /
DW_AT_decl_line using a signed integer form, llvm-dwarfdump crashes,
like so:

(... snip ...)
0x000000b4:   DW_TAG_structure_type
                DW_AT_name      ("test_struct")
                DW_AT_byte_size (136)
                DW_AT_decl_file (llvm-dwarfdump: (... snip ...)/llvm/include/llvm/ADT/Optional.h:197: T& llvm::optional_detail::OptionalStorage<T, true>::getValue() &

[with T = long unsigned int]: Assertion `hasVal' failed.

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /opt/rocm/llvm/bin/llvm-dwarfdump ./testsuite/outputs/gdb.rocm/lane-pc-vega20/lane-pc-vega20-kernel.so
 #0 0x000055cc8e78315f PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x000055cc8e780d3d SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f8f2cae8420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
 #3 0x00007f8f2c58d00b raise /build/glibc-SzIz7B/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #4 0x00007f8f2c56c859 abort /build/glibc-SzIz7B/glibc-2.31/stdlib/abort.c:81:7
 #5 0x00007f8f2c56c729 get_sysdep_segment_value /build/glibc-SzIz7B/glibc-2.31/intl/loadmsgcat.c:509:8
 #6 0x00007f8f2c56c729 _nl_load_domain /build/glibc-SzIz7B/glibc-2.31/intl/loadmsgcat.c:970:34
 #7 0x00007f8f2c57dfd6 (/lib/x86_64-linux-gnu/libc.so.6+0x33fd6)
 #8 0x000055cc8e58ceb9 llvm::DWARFDie::dump(llvm::raw_ostream&, unsigned int, llvm::DIDumpOptions) const (/opt/rocm/llvm/bin/llvm-dwarfdump+0x2e0eb9)
 #9 0x000055cc8e58bec3 llvm::DWARFDie::dump(llvm::raw_ostream&, unsigned int, llvm::DIDumpOptions) const (/opt/rocm/llvm/bin/llvm-dwarfdump+0x2dfec3)
#10 0x000055cc8e5b28a3 llvm::DWARFCompileUnit::dump(llvm::raw_ostream&, llvm::DIDumpOptions) (.part.21) DWARFCompileUnit.cpp:0:0

Likewise with DW_AT_call_file / DW_AT_call_line.

The problem is that the code in llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
dumping these attributes assumes that
FormValue.getAsUnsignedConstant() returns an armed optional. If in
debug mode, we get an assertion line the above. If in release mode,
and asserts are compiled out, then we proceed as if the optional had a
value, running into undefined behavior, printing whatever random
value.

Fix this by checking whether the optional returned by
FormValue.getAsUnsignedConstant() has a value, like done in other
places.

In addition, DWARFVerifier.cpp is validating DW_AT_call_file /
DW_AT_decl_file, but not AT_call_line / DW_AT_decl_line. This commit
fixes that too.

The llvm-dwarfdump/X86/verify_file_encoding.yaml testcase is extended
to cover these cases. Current llvm-dwarfdump crashes running the
newly-extended test.

"make check-llvm-tools-llvm-dwarfdump" shows no regressions, on x86-64
GNU/Linux.

Diff Detail

Event Timeline

palves created this revision.Jul 8 2022, 12:58 PM
Herald added a project: Restricted Project. · View Herald Transcript
palves requested review of this revision.Jul 8 2022, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 12:58 PM
palves added a comment.Jul 8 2022, 1:04 PM

This is the first time I've ever uploaded an llvm patch, and I've used arc from the command line. Hope I've not messed this up too badly.

BTW, I ran into this with hand-coded DWARF, using GDB's DWARF-assembler machinery.

For example, here's a GDB testcase fix for a test that was using DW_FORM_sdata forms for decl_line:
https://sourceware.org/pipermail/gdb-patches/2022-July/190570.html

dblaikie accepted this revision.Jul 8 2022, 1:40 PM

Sure, sounds OK.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
154–158

Generally LLVM code doesn't put {} on single-line blocks like these.

This revision is now accepted and ready to land.Jul 8 2022, 1:40 PM
palves added a comment.Jul 8 2022, 1:46 PM

Sure, sounds OK.

Thanks! Could someone merge it for me then? I don't have write access.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
154–158

I think I put those in to avoid dangling-else warnings.

palves updated this revision to Diff 443351.Jul 8 2022, 2:21 PM

Remove unnecessary braces.

palves added inline comments.Jul 8 2022, 2:23 PM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
154–158

Turns out there's no dangling-else warning here. I've uploaded an update to removes the unnecessary braces.

palves updated this revision to Diff 443356.Jul 8 2022, 2:34 PM

Yes more unnecessary braces removed. I think this is ready now.

This revision was landed with ongoing or failed builds.Jul 8 2022, 2:35 PM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Jul 8 2022, 2:38 PM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
139–148

I /think/ LLVM's style guide now doesn't encourage omitting braces from single statement but multi-line blocks like this, so I left these ones in.

154–158

Ah, removed these in a follow-up commit ( 84960896236338eb1c7b30f34041214e64db0bd1 )

palves added inline comments.Jul 8 2022, 2:43 PM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
154–158

Cool, thank you very much, and sorry for the extra work. Much appreciated!

clayborg added inline comments.Jul 8 2022, 3:53 PM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
139–148

Do we want to dump the attribute as a number if we can't get the lint table of the value as unsigned here? What happens when we dump a DW_AT_decl_file or DW_AT_call_file that has a signed integer encoding? Nothing comes out? Seems like we should do a "FormValue.dump(OS, DumpOpts);" if either the lit table or optional uint64_t fail to get extracted

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
707–708

Do we want to handle DW_AT_decl_column and DW_AT_call_column here as well?

dblaikie added inline comments.Jul 8 2022, 4:06 PM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
139–148

Do we want to dump the attribute as a number if we can't get the lint table of the value as unsigned here?

I believe we do that - at least looks like it according to the test touched in this patch & the fallback at the end of the if/else if/else chain below.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
707–708

Sure - I'd be open to that.