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

Fri Jun 17 00:18:21 PDT 2022

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:
> > > 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.
Thanks! But during the process of implementing it, I find it is more complex than necessary. We need to judge if it is larger than a byte or if it is a multiple of bytes. So that we have at least 3 cases to handle. In my mind, we only need to provide two properties when we meet unknowing types: start address and the size. So that the users could debug in the pretty old fashion (assume the size of the unknown type is 128 in the example):
```
(gdb) x/128x the_variable_address_of_unknow_types
```

And the current implementation looks not bad in this direction. I totally understand that it looks not good in the code: how come we get a char in 128 bits ?! It looks odd indeed. But from my experience, the users wouldn't be strict with the Dwarf Type in the case since the type name starts with 'Unknown'. What I want to say here is that: it might not be good indeed but it is not so bad too. The root problem here might be that we're going to construct debug info in middle end. So we would meet some weird things I guess.

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

Got it. Will do in following patches.

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

https://reviews.llvm.org/D127625