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

[David Blaikie]

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?

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