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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In D79592#2026542 <https://reviews.llvm.org/D79592#2026542>, @alok wrote:

> In D79592#2025853 <https://reviews.llvm.org/D79592#2025853>, @aprantl wrote:
>
> > Why is the data location a part of the type and not something dynamically bound via dbg.value or something similar?
> >
> > What's an example for what a typical data location expression?
>
>
> Dynamic arrays are represented by two things
>
> - allocated space
> - descriptor which describes the array bounds (always) and allocated space (not always)
>
>   DW_AT_location attached to DW_TAG_variable denotes descriptor, which is used by bounds (using DW_OP_push_object_address). Since DW_AT_location denotes descriptor, DW_AT_data_location is used to denote allocated space. DWARF has attached that to DW_TAG_array_type. In cases when descriptor also has a pointer to allocated space (as in case of gfortran), DW_AT_data_location can also be described by DW_OP_push_object_address and offset where the pointer to allocated space is. In cases when descriptor does not have the pointer to allocated space (as in case of flang), DW_AT_data_location can be represented by DIE reference which denotes an artificial variable which has the data pointer (dbg.declare in IR). Please let me know if you need more information.


Thanks. I read through the DWARF v5 spec and given that arrays are represented as DICompositeTypes this makes sense to me. Personally I find the DWARF nomenclature weird (it would make more sense if we had a DW_AT_type_descriptor and a DW_AT_location), but it's probably better to stick with the DWARF naming scheme here, or else this raises even more confusion.

Can you please add a check to Verifier.cpp that checks that only arrays get a data location?
Can you please add a round-trip test to test/Assembler/ (see dicompositetype-members.ll for an example)?
Can you please extend the metadata unit test llvm/unittests/IR/MetadataTest.cpp to exercise the changes in LLVMContextImpl.h?
Should this be added to DIBuilder?

Can you add some form of documentation that explains what the new field is used for? Perhaps in the doxygen of DIBuilder, and/or in SourceLevelDebugging.rst or LangRef.rst?



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1391
+      if (Record.size() > 17)
+        if (Tag == dwarf::DW_TAG_array_type)
+          DataLocation = getMDOrNull(Record[17]);
----------------
This check should be removed here. Can you move it into Verifier.cpp?


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:562
     return hash_combine(Name, File, Line, BaseType, Scope, Elements,
-                        TemplateParams);
+                        TemplateParams, DataLocation);
   }
----------------
I don't think it makes sense performance-wise to hash DataLocation.


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

https://reviews.llvm.org/D79592