This is an archive of the discontinued LLVM Phabricator instance.

[Debug-Info] [llvm-dwarfdump] Don't try to dump location list for attributes that don't have the loclist class.
ClosedPublic

Authored by Esme on Jul 8 2021, 1:38 AM.

Details

Summary

As what we discussed in D105469, the overflow error occurs when we try to dump location list for those attributes that do not have the loclist class, like DW_AT_count and DW_AT_byte_size
After re-reviewed the entire list, I sorted those attributes into two parts, one for dumping location list and one for dumping the location expression.

Diff Detail

Event Timeline

Esme created this revision.Jul 8 2021, 1:38 AM
Esme requested review of this revision.Jul 8 2021, 1:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
Esme edited the summary of this revision. (Show Details)Jul 8 2021, 2:09 AM
Esme updated this revision to Diff 357171.Jul 8 2021, 2:19 AM

Overall the patch looks pretty reasonable, needs some polishing up.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
96

Should verify the right kind of FORM here, as in:

assert((FormValue.isFormClass(DWARFFormValue::FC_Block) ||
        FormValue.isFormClass(DWARFFormValue::FC_Exprloc)) &&
       "bad FORM for location expression");
770

Missing DW_AT_bit_offset here (between byte_size and bit_size). I know this isn't really directly what you were doing, but if we're reviewing all the exprloc/loclist attributes, might as well fix both lists.

785

Missing DW_AT_data_location between associated and byte_stride.

llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s
11

I think the following single source line should be sufficient:
unsigned char arr[0x100000];
This is a global definition (in C++ anyway), you don't need any statements to be referencing it. This will shorten the test code noticeably. We do try to minimize test cases, it helps readers to understand the goal of the test without wading through a lot of extra fluff.

15

If you simplify the source to have no executable instructions, then I *think* you can remove the -triple which will allow this test to be run in all environments. (A number of bots do not build all targets, so it's preferable to have tests be as generic as possible.)

Esme updated this revision to Diff 360750.Jul 22 2021, 3:10 AM

Thank you Paul!
Address comments.

llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s
15

Some targets, like AIX, have not yet supported llvm-mc. So I think we should still keep the -triple?

probinson added inline comments.Jul 22 2021, 6:22 AM
llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s
8

It's more common to pipe the object file instead of writing it to the file system. The test runs a little faster that way, and across many thousands of tests these little things add up.

# RUN: llvm-mc ... -o - | llvm-dwarfdump -debug-info - | FileCheck %s
15

If you keep -triple then you need a REQUIRES to restrict the test to bots that include the correct target. A number of bots do not build all targets.

dblaikie added inline comments.Jul 22 2021, 12:13 PM
llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s
15

If you move the test to the llvm/test/tools/llvm-dwarfdump/X86 directory you shouldn't need the REQUIRES - the whole directory has that implemented generically.

Esme updated this revision to Diff 361080.Jul 22 2021, 9:35 PM

Address comments.

Esme updated this revision to Diff 361082.Jul 22 2021, 9:36 PM
This revision is now accepted and ready to land.Jul 23 2021, 5:44 AM
This revision was landed with ongoing or failed builds.Jul 27 2021, 12:29 AM
This revision was automatically updated to reflect the committed changes.