[PATCH] D127625: [Debug] [Coroutines] Get rid of DW_ATE_address

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:925
     RetType = Builder.createBasicType(OS.str(), Layout.getTypeSizeInBits(Ty),
-                                      dwarf::DW_ATE_address,
-                                      llvm::DINode::FlagArtificial);
+      dwarf::DW_ATE_unsigned_char, llvm::DINode::FlagArtificial);
   }
----------------
dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > How come this one became `unsigned char` rather than pointer type?
> > The intention of the 'else' section is to create a DIType for an unresolved type. And the unresolved type lives in the coroutine frame. So it is a little bit odd for me to treat it as a pointer type. Since the size of the unresolved type might be larger than a pointer. 
> > 
> > The choice to use `DW_ATE_unsigned_char` might not be correct. Here is my thoughts. I want to treat the unresolved type as bytes. I want to make it as something like "I don't know what you are so I will present the bytes directly". And `DW_ATE_unsigned_char` is the closest thing I found in DW_ATE* things. How do you think about it?
> > 
> > This is covered in the test as vector types. Since we failed to recognize vector types now, we would treat it as a unresolved type with 128 size.
> So this is going to create an arbitrarily large "unsigned char" type (that might be tens of bytes, etc?) - is it at least a multiple of whole bytes? Maybe it should be a char array instead?
> So this is going to create an arbitrarily large "unsigned char" type (that might be tens of bytes, etc?)

Yes.

>  is it at least a multiple of whole bytes? Maybe it should be a char array instead?

>From my understanding, I feel like the allowed choices is listed in https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/Dwarf.def#L917-L937. I feel like a char array might be good. But I am not sure how to make it.




================
Comment at: llvm/test/Transforms/Coroutines/coro-debug-coro-frame.ll:36
+; CHECK-DAG: ![[INT64_PTR]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_64_Ptr_3",{{.*}} baseType: ![[INT64_PTR_BASE:[0-9]+]]
+; CHECK-DAG: ![[INT64_PTR_BASE]] = !DIDerivedType(tag: DW_TAG_pointer_type, name: "__int_64_Ptr", baseType: null, size: 64
+; CHECK-DAG: ![[INT32_2]] = !DIDerivedType(tag: DW_TAG_member, name: "__int_32_4", scope: ![[FRAME_TYPE]], file: ![[FILE]], line: [[PROMISE_VAR_LINE]], baseType: ![[I32_BASE:[0-9]+]]{{.*}}, flags: DIFlagArtificial
----------------
dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > Why does this type have a name? I think it should probably be nameless - so it should merge with other instances of the same pointer type created for the original source code, etc.
> > The name here is try to provide more information as much as possible. Ideally, I think this one should be the `int*` type. It shows the the ability of type resolver in current coroutine compiler is not strong enough. Given this revision aims to get rid of DW_ATE_address, I would try to fix the problem in other revisions.
> > 
> > For the duplicated DITypes problem, this is tested in the following function 'bar' that the generated DIType would be reused.
> > The name here is try to provide more information as much as possible. Ideally, I think this one should be the int* type. 
> 
> Any reason it can't be int*? Currently it's void*.
> 
> > Given this revision aims to get rid of DW_ATE_address, I would try to fix the problem in other revisions.
> 
> Sure
> 
> > For the duplicated DITypes problem, this is tested in the following function 'bar' that the generated DIType would be reused.
> 
> I was thinking of the types created from the original C++ source that has pointers in it too - not other types created by the coroutine lowering code.
> Any reason it can't be int*? Currently it's void*.

It is due to the current mechanism to solve LLVM types to DITypes is not mature enough. My bad. Let's enhance them later.

> I was thinking of the types created from the original C++ source that has pointers in it too - not other types created by the coroutine lowering code.

It would try to find @dbg.declare intrinsics for variables in the frame. If founded, it would reuse that one. But if it failed to find one, it would only try to create one.


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

https://reviews.llvm.org/D127625