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

[Alok Kumar Sharma via Phabricator via llvm-commits]

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


================
Comment at: llvm/docs/LangRef.rst:4796
 level of indexing. The ``DIFlagVector`` flag to ``flags:`` indicates that an
-array type is a native packed vector.
+array type is a native packed vector. the ``dataLocation:`` is location
+description, representing location of the data for an object. When this is
----------------
aprantl wrote:
> the -> The
> 
> LLVM IR doesn't know the term location description. Let's call it what it is:
> 
> ```
> The ``dataLocation`` is a DIExpression that describes how to get from an object's address to the actual raw data, if they aren't equivalent. This is only supported for array types, particularly to describe Fortran arrays, which have an array descriptor in addition to the array data.
> ```
Thanks a lot for this suggestion. It looks far better now. I shall include DIVariable along with DIExpression. It can be found in next version.


================
Comment at: llvm/lib/IR/Verifier.cpp:1018
+             "dataLocation can only appear in array type ");
+  }
 }
----------------
aprantl wrote:
> Thanks! Can you add a tiny testcase to tests/Verifier that covers this check?
Thanks for your comment. It shall be included in next version of patch.


================
Comment at: llvm/test/Bitcode/dataLocation.ll:7
+; CHECK:  !DICompositeType(tag: DW_TAG_array_type, baseType: !{{[0-9]+}}, size: 32, align: 32, elements: !{{[0-9]+}}, dataLocation: !{{[0-9]+}})
+; CHECK:  !DICompositeType(tag: DW_TAG_array_type, baseType: !{{[0-9]+}}, size: 32, align: 32, elements: !{{[0-9]+}}, dataLocation: !DIExpression(DW_OP_constu, 3412))
+
----------------
aprantl wrote:
> aprantl wrote:
> > Out of curiosity: This is a placeholder, right? I would have expected something like `!DIExpression(DW_OP_constu, 32, DW_OP_plus)` here.
> Ah.. presumably there will be a DW_OP_push_object_address, too, right?
Yes correct. Since that is a separate patch and this tests objective is just to test if an expression can appear for dataLocation field and not, and not to check what is valid expression. To avoid dependency on other patch i just used a place holder here.


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

https://reviews.llvm.org/D79592