[llvm] r208502 - DwarfUnit: Make explicit a limitation/bug in enumeration constant emission.

[Adrian Prantl]

> On May 12, 2014, at 11:14 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> On Mon, May 12, 2014 at 11:11 AM, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>>> On May 12, 2014, at 11:05 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> 
>>> On Mon, May 12, 2014 at 10:48 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>> 
>>>>> On May 12, 2014, at 10:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> 
>>>>> On Mon, May 12, 2014 at 10:28 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>>> 
>>>>>>> On May 11, 2014, at 10:04 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>> 
>>>>>>> Author: dblaikie
>>>>>>> Date: Sun May 11 12:04:05 2014
>>>>>>> New Revision: 208502
>>>>>>> 
>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=208502&view=rev
>>>>>>> Log:
>>>>>>> DwarfUnit: Make explicit a limitation/bug in enumeration constant emission.
>>>>>>> 
>>>>>>> Filed as PR19712, LLVM fails to detect the right type of an enum
>>>>>>> constant when a frontend does not provide an underlying type for the
>>>>>>> enumeration type.
>>>>>>> 
>>>>>>> Modified:
>>>>>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>>>>> 
>>>>>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
>>>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=208502&r1=208501&r2=208502&view=diff
>>>>>>> ==============================================================================
>>>>>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
>>>>>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Sun May 11 12:04:05 2014
>>>>>>> @@ -748,12 +748,17 @@ void DwarfUnit::addBlockByrefAddress(con
>>>>>>> /// Return true if type encoding is unsigned.
>>>>>>> static bool isUnsignedDIType(DwarfDebug *DD, DIType Ty) {
>>>>>>> DIDerivedType DTy(Ty);
>>>>>>> -  if (DTy.isDerivedType())
>>>>>>> -    return isUnsignedDIType(DD, DD->resolve(DTy.getTypeDerivedFrom()));
>>>>>>> +  if (DTy.isDerivedType()) {
>>>>>>> +    if (DIType Deriv = DD->resolve(DTy.getTypeDerivedFrom()))
>>>>>>> +      return isUnsignedDIType(DD, Deriv);
>>>>>>> +    // FIXME: Enums without a fixed underlying type have unknown signedness
>>>>>>> +    // here, leading to incorrectly emitted constants.
>>>>>>> +    assert(DTy.getTag() == dwarf::DW_TAG_enumeration_type);
>>>>>>> +    return false;
>>>>>>> +  }
>>>>>> 
>>>>>> This should be relaxed to allow other DIDerivedTypes, including DIComposite types.
>>>>> 
>>>>> What relaxation did you have in mind? "isDerivedType" returns true for
>>>>> DICompositeTypes too, I believe - so perhaps it's already as relaxed
>>>>> as you were suggesting it should be?
>>>> 
>>>> It does, and they will run straight into the assertion (which should be relaxed, in the sense of "removed” ;-).
>>> 
>>> Why would they run into the assertion? The member variables have a
>>> "type derived from" (being their underlying type - int, for example)
>>> and thus they'll go through the recursive call, reach a base type, and
>>> we'll discover that it's a signed or unsigned int, etc. No?
>> 
>> The members would have that, but not the struct itself. We’re not emitting locations for individual members (after all their locations would be shared across the entire CU) but for the variable that in its entirety is of type struct, and may consist of multiple pieces.
> 
> There's no logical signedness for a composite type, only for its
> individual fields, so this function should never be called with the
> composite type itself.
> 

That’s what I was getting at with my example below:
A slightly smarter LLVM would emit a
  DBG_VALUE (constant imm. 42) (Var= (struct S) s)
for the variable in foo(). And then isUnsignedDIType() would have to walk through the members of the composite type to figure out the signedness.

It’s a little more illustrative if the struct had a second member (e.g., struct S { int i; unsigned char c; };), we would get a
 
  DBG_VALUE (constant imm. 42) (Var= (struct S) s, piece (size = 8, ofs=0))
  DBG_VALUE (constant imm. 32) (Var= (struct S) s, piece (size = 1, ofs=8)))

and the DWARF would look roughly like this:

DW_TAG_variable
   DW_AT_name("s")
   DW_AT_location( DW_OP_const 42, DW_OP_piece 8, DW_OP_constu 32, DW_OP_piece 1 )

> - David
> 
>> 
>> -- adrian
>> 
>>> 
>>> - David
>>> 
>>>>> 
>>>>>> I’m imagining a world where SROA does not elide debug info, and a function like this:
>>>>>> 
>>>>>> struct S { int c; };
>>>>>> int foo() {
>>>>>> struct S s = { 42 };
>>>>>> return s.c;
>>>>>> }
>>>>>> 
>>>>>> Walking the struct members to find their signedness will be fun!
>>>>> 
>>>>> Yeah... :)
>>>>> 
>>>>> - David
>>>>> 
>>>>>> 
>>>>>> cheers,
>>>>>> adrian
>>>>>>> 
>>>>>>> DIBasicType BTy(Ty);
>>>>>>> -  if (!BTy.isBasicType())
>>>>>>> -    return false;
>>>>>>> +  assert(BTy.isBasicType());
>>>>>>> unsigned Encoding = BTy.getEncoding();
>>>>>>> assert(Encoding == dwarf::DW_ATE_unsigned ||
>>>>>>>       Encoding == dwarf::DW_ATE_unsigned_char ||