[llvm] r202188 - Debug info: Support variadic functions.

[Eric Christopher]

On Tue, Feb 25, 2014 at 2:33 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Feb 25, 2014, at 13:54, Eric Christopher <echristo at gmail.com> wrote:
>
> Thanks, Eric! I addressed everything unless explicitly stated in r202199.
>

Cool. I'll look.

>> Would be nice to have more description what the bugfix is here. What
>> was the difference? Did you add more to the testcase or a comment in
>> some place?
>
> With the refactoring in the follow-up commit the fix more or less became obsolete. All three places now use the canonical (and correct) implementation in DwarfUnit::constructSubprogramArguments().

Excellent.

>>> +          // FIXME: Use DwarfUnit::constructSubprogramArguments() here.
>>
>> What's the problem here?
>
> Code duplication in three places.

Ah, I meant "why hadn't you?", but you seem to have fixed that.

>>> +    DIArray FnArgs = SP.getType().getTypeArray();
>>> +    if (FnArgs.getElement(FnArgs.getNumElements()-1).isUnspecifiedParameter()) {
>>> +      DIE *Ellipsis = new DIE(dwarf::DW_TAG_unspecified_parameters);
>>> +      Children.push_back(Ellipsis);
>>> +    }
>>> +  }
>>> +
>>
>> This feels awkward here. Thoughts?
>
> We have to insert it immediately after the formal parameters and before the variables. There does not seem to be a more appropriate place than this one.
>

Hrm. Sad. It's inelegant. :)

-eric

>>
>>> +/// constructSubprogramArguments - Construct function argument DIEs.
>>> +void DwarfUnit::constructSubprogramArguments(DIE &Buffer, DIArray Args) {
>>> +    for (unsigned i = 1, N = Args.getNumElements(); i < N; ++i) {
>>> +      DIDescriptor Ty = Args.getElement(i);
>>> +      if (Ty.isUnspecifiedParameter()) {
>>> +        assert(i == N-1 && "ellipsis must be the last argument");
>>
>> Same comment as above about the text here.
>>
>>>     // Add arguments. Do not add arguments for subprogram definition. They will
>>>     // be handled while processing variables.
>>> -    for (unsigned i = 1, N = Args.getNumElements(); i < N; ++i) {
>>> -      DIE *Arg = createAndAddDIE(dwarf::DW_TAG_formal_parameter, *SPDie);
>>> -      DIType ATy(Args.getElement(i));
>>> -      addType(Arg, ATy);
>>> -      if (ATy.isArtificial())
>>> -        addFlag(Arg, dwarf::DW_AT_artificial);
>>> -    }
>>> +    constructSubprogramArguments(*SPDie, Args);
>>
>> The comment here seems like it should go onto the function or change somewhat.
>>
>>>   }
>>>
>>>   if (SP.isArtificial())
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h?rev=202188&r1=202187&r2=202188&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.h Tue Feb 25 13:57:42 2014
>>> @@ -476,6 +476,9 @@ protected:
>>>   DIE *getOrCreateStaticMemberDIE(DIDerivedType DT);
>>>
>>> private:
>>> +  /// constructSubprogramArguments - Construct function argument DIEs.
>>> +  void constructSubprogramArguments(DIE &Buffer, DIArray Args);
>>> +
>>
>> This refactoring could have been done in a separate patch.
>>
>>>   /// constructTypeDIE - Construct basic type die from DIBasicType.
>>>   void constructTypeDIE(DIE &Buffer, DIBasicType BTy);
>>>
>>>
>>> Added: llvm/trunk/test/DebugInfo/X86/varargs.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/varargs.ll?rev=202188&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/DebugInfo/X86/varargs.ll (added)
>>> +++ llvm/trunk/test/DebugInfo/X86/varargs.ll Tue Feb 25 13:57:42 2014
>>> @@ -0,0 +1,90 @@
>>> +; RUN: llc -O0 -filetype=obj -o %t.o %s
>>> +; RUN: llvm-dwarfdump -debug-dump=info %t.o | FileCheck %s
>>> +;
>>> +; Normal variadic function.
>>> +;
>>> +; CHECK: DW_TAG_subprogram
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_formal_parameter
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_unspecified_parameters
>>> +;
>>> +; Variadic C++ member function.
>>> +;
>>> +; CHECK: DW_TAG_subprogram
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_formal_parameter
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_formal_parameter
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_unspecified_parameters
>>> +;
>>> +; Variadic function pointer.
>>> +;
>>> +; CHECK: DW_TAG_subroutine_type
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_formal_parameter
>>> +; CHECK-NOT: DW_TAG
>>> +; CHECK: DW_TAG_unspecified_parameters
>>
>> Please add more details (CHECK lines with names perhaps?) to what
>> you're testing here and comments. The check and check-not lines are
>> inscrutable. Also is there any reason that this is X86 only? You don't
>> appear to be checking addresses or sizes.
>>
>> -eric
>