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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

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



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:96
+                             DIDumpOptions DumpOpts) {
+  DWARFContext &Ctx = U->getContext();
+  const MCRegisterInfo *MRI = Ctx.getRegisterInfo();
----------------
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");
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:769
   case DW_AT_byte_size:
   case DW_AT_bit_size:
   case DW_AT_string_length:
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:784
   case DW_AT_associated:
   case DW_AT_byte_stride:
   case DW_AT_rank:
----------------
Missing `DW_AT_data_location` between associated and byte_stride.


================
Comment at: llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s:10
+#     return arr[0] + arr[50];
+#   }
+# Compile with:
----------------
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.


================
Comment at: llvm/test/tools/llvm-dwarfdump/ELF/formclass3.s:14
+
+# RUN: llvm-mc %s -filetype obj -triple powerpc64le-unknown-unknown -o %t.o
+# RUN: llvm-dwarfdump -debug-info %t.o | FileCheck %s -check-prefix=AT-COUNT
----------------
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105613/new/

https://reviews.llvm.org/D105613