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

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 4 inline comments as done.
alok added a comment.

In D79592#2027165 <https://reviews.llvm.org/D79592#2027165>, @aprantl wrote:

> 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.


Thanks for your attention and comments.

> Can you please add a check to Verifier.cpp that checks that only arrays get a data location?

I shall update the patch with this.

> Can you please add a round-trip test to test/Assembler/ (see dicompositetype-members.ll for an example)?

I have added the round trip test named llvm/test/Bitcode/dataLocation.ll, should I move this to Assembler directory?

> Can you please extend the metadata unit test llvm/unittests/IR/MetadataTest.cpp to exercise the changes in LLVMContextImpl.h?

I shall update the patch to incorporate this.

> Should this be added to DIBuilder?

As of now, flang front-end doesnt utilize these routines, we can defer it or update it now itself provided f18 is now part of LLVM and later we may need.

> 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]);
----------------
aprantl wrote:
> This check should be removed here. Can you move it into Verifier.cpp?
Thanks for your comment, I shall move that in next version of patch.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:562
     return hash_combine(Name, File, Line, BaseType, Scope, Elements,
-                        TemplateParams);
+                        TemplateParams, DataLocation);
   }
----------------
aprantl wrote:
> I don't think it makes sense performance-wise to hash DataLocation.
Thanks for your comment. I shall incorporate this in next version of patch.


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

https://reviews.llvm.org/D79592