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

[Adrian Prantl]

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

-- 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 ||
>>