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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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);
   }
----------------
ChuanqiXu wrote:
> 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.
> 
> 
You can check Clang's code for making array types - DIBuilder::createArrayType, passed in the char basic type.


================
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
----------------
ChuanqiXu wrote:
> 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.
> > 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.

Not sure I follow - but sure, can handle that in a follow-up patch.

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

Sure - but what I mean is if it tries to create one it could create one that's more similar to the ones the frontend would create - a named pointer type is a weird beast that we wouldn't usually create in the frontend (pointer types don't have names, unless it's via a typedef - the typedef has the name, the pointer type does not).


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

https://reviews.llvm.org/D127625