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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Think this is alright?



================
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:
> > > > 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.
> > I'm not sure I understand the complexity - I'd asked earlier if it was always a byte multiple, which I think you said it was - in that case I'd always create an array of that many bytes. I don't think this should be complicated.
> > 
> > This would be more likely to be supported by DWARF Consumers (I'm not sure what they'd do with a huge integer type) & probably render more meaningfully for users (rather than as one big integer, array printers in debuggers do smart user-customizable things like printing only the first N array elements and then using `...`, etc).
> > I'm not sure I understand the complexity - I'd asked earlier if it was always a byte multiple, which I think you said it was - in that case I'd always create an array of that many bytes. I don't think this should be complicated.
> 
> Sorry for the confusion. My bad. For the question if it is always a byte multiple, according to my experiments these days, it is always a byte multiple in C++ due to the alignment requirement. I also added an `assert (Size % 8 == 0)` and run it for some workloads. It looked fine. But for LLVM IR, we could make the assertion failure simply by making a vector type of `<9 x i1>`. So I add the `if` part in this revision.
> 
> > This would be more likely to be supported by DWARF Consumers (I'm not sure what they'd do with a huge integer type) & probably render more meaningfully for users (rather than as one big integer, array printers in debuggers do smart user-customizable things like printing only the first N array elements and then using ..., etc).
> 
> You know debugger much than me. I'd follow it.
OK, sounds good!


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

https://reviews.llvm.org/D127625