[PATCH] D79592: [DebugInfo] support for DW_AT_data_location in llvm

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 8 inline comments as done.
alok added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1393
+      if (Tag == dwarf::DW_TAG_array_type)
+        DataLocation = getMDOrNull(Record[17]);
     DICompositeType *CT = nullptr;
----------------
vsk wrote:
> Is this the right thing to do in the ThinLTO case discussed above?
Thanks for pointing this out, I completely overlooked it. It will be corrected in next version.


================
Comment at: llvm/test/DebugInfo/dwarfdump-dataLocationExp.ll:1
+;; This test checks DW_AT_data_location attribute
+
----------------
vsk wrote:
> Could you expand the comment to describe what aspect of DW_AT_data_location this is testing?
Thanks for your comment. I shall update this in next version.


================
Comment at: llvm/test/DebugInfo/dwarfdump-dataLocationExp.ll:3
+
+; RUN: llc %s -O2 -filetype=obj -o %t.o
+; RUN: llvm-dwarfdump  %t.o | FileCheck %s
----------------
vsk wrote:
> Why do optimizations need to run in order to validate dwarfdump output? I'd expect it to be possible to write a test like this:
> 
> ```
> define void @f() {
>   dbg.declare(%arrayVal, !arrayMD, ...)
>   ret void
> }
> !arrayMD = !DILocalVariable(..., !typeMD)
> !typeMD = !DICompositeType(array_type, ...)
> ```
> 
> Everything else makes the test harder to understand and maintain.
Thanks for your suggestion. test will be trimmed in next version.


================
Comment at: llvm/test/DebugInfo/dwarfdump-dataLocationVar.ll:1
+;; This test checks DW_AT_data_location attribute
+
----------------
vsk wrote:
> ditto
I shall update the test case for your comments.


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

https://reviews.llvm.org/D79592