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

[Adrian Prantl]

> On May 12, 2014, at 11:37 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> On Mon, May 12, 2014 at 11:33 AM, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>>> 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 )
> 
> If we're piecing the total struct's value together from arbitrary
> chunks of bytes, we should treat those bytes as just simple, unsigned
> bytes - it doesn't matter what the signedness of the chunk they came
> from is. They're just raw bytes that make up bits of the object.
> 
> Indeed, in your example - we could represent the whole object as a
> single constant immediate and it doesn't make sense to talk about the
> signedness of that constant. It's just the bytes that make up the
> data.

That makes sense to me. If we do it that way, however, the assertion needs to be relaxed to also allow for DW_TAG_structure_type (and unions?) and _then_ return false.

-- adrian
> 
> At least that's how I'd think about that... perhaps there's something
> wrong with the way I'm thinking about it though.
> 
> - David
> 
>> 
>>> - 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 ||