[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:
> > 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?


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


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

https://reviews.llvm.org/D127625