[llvm] r203309 - Remove unnecessary test for Darwin and update testcase to be a little less

Fri Jun 13 14:29:25 PDT 2014

On Fri, Jun 13, 2014 at 2:00 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>> On Jun 13, 2014, at 1:48 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> On Fri, Mar 7, 2014 at 3:07 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Author: adrian
>>> Date: Fri Mar  7 17:07:21 2014
>>> New Revision: 203309
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=203309&view=rev
>>> Log:
>>> Remove unnecessary test for Darwin and update testcase to be a little less
>>> horrible/fragile.
>>> rdar://problem/16264854
>>>
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>>    llvm/trunk/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
>>>    llvm/trunk/test/DebugInfo/X86/dbg-value-location.ll
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp?rev=203309&r1=203308&r2=203309&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp Fri Mar  7 17:07:21 2014
>>> @@ -2845,7 +2845,7 @@ void DwarfDebug::addDwarfTypeUnitType(Dw
>>> void DwarfDebug::attachLowHighPC(DwarfCompileUnit *Unit, DIE *D,
>>>                                  MCSymbol *Begin, MCSymbol *End) {
>>>   Unit->addLabelAddress(D, dwarf::DW_AT_low_pc, Begin);
>>> -  if (DwarfVersion < 4 || Triple(Asm->getTargetTriple()).isOSDarwin())
>>> +  if (DwarfVersion < 4)
>>>     Unit->addLabelAddress(D, dwarf::DW_AT_high_pc, End);
>>>   else
>>>     Unit->addLabelDelta(D, dwarf::DW_AT_high_pc, End, Begin);
>>>
>>> Modified: llvm/trunk/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/dbg-value-inlined-parameter.ll?rev=203309&r1=203308&r2=203309&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/DebugInfo/X86/dbg-value-inlined-parameter.ll (original)
>>> +++ llvm/trunk/test/DebugInfo/X86/dbg-value-inlined-parameter.ll Fri Mar  7 17:07:21 2014
>>> @@ -8,8 +8,7 @@
>>> ;CHECK: DW_TAG_inlined_subroutine
>>> ;CHECK-NEXT: DW_AT_abstract_origin
>>> ;CHECK-NEXT: DW_AT_low_pc [DW_FORM_addr]
>>> -;DARWIN-NEXT: DW_AT_high_pc [DW_FORM_addr]
>>> -;LINUX-NEXT: DW_AT_high_pc [DW_FORM_data4]
>>> +;CHECK-NEXT: DW_AT_high_pc [DW_FORM_data4]
>>> ;CHECK-NEXT: DW_AT_call_file
>>> ;CHECK-NEXT: DW_AT_call_line
>>>
>>>
>>> Modified: llvm/trunk/test/DebugInfo/X86/dbg-value-location.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/dbg-value-location.ll?rev=203309&r1=203308&r2=203309&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/test/DebugInfo/X86/dbg-value-location.ll (original)
>>> +++ llvm/trunk/test/DebugInfo/X86/dbg-value-location.ll Fri Mar  7 17:07:21 2014
>>> @@ -1,14 +1,16 @@
>>> -; RUN: llc < %s | FileCheck %s
>>> -; RUN: llc < %s -regalloc=basic | FileCheck %s
>>> +; RUN: llc -filetype=obj %s -o - | llvm-dwarfdump -debug-dump=info - | FileCheck %s
>>> +; RUN: llc -filetype=obj %s -regalloc=basic -o - | llvm-dwarfdump -debug-dump=info - | FileCheck %s
>>> target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
>>> target triple = "x86_64-apple-darwin10.0.0"
>>> -;Radar 8950491
>>> +; Test that the type for the formal parameter "var" makes it into the debug info.
>>
>> I don't think this was the intended purpose of this test, judging by
>> the original commit message in r124845. Though the rdar may have more
>> information that I'm not privy to.
>>
>> The original commit message was:
>>
>> "DebugLoc associated with a machine instruction is used to emit
>> location entries. DebugLoc associated with a DBG_VALUE is used to
>> identify lexical scope of the variable. After register allocation,
>> while inserting DBG_VALUE remember original debug location for the
>> first instruction and reuse it, otherwise dwarf writer may be mislead
>> in identifying the variable's scope."
>>
>> Judging by that, while I am a little confused how this test relates to
>> that "mislead in identifying the variable's scope", I /think/ the test
>> was trying to test that the formal parameter has a file and line?
>
> From the radar it looks like before r124845 the backend would emit a (wrong) DW_AT_abstract_origin and neither type nor file/line.

OK, I could believe that.

>
>> Honestly it seems more like the test probably wanted to ensure the
>> parameter was at the top level scope, or had some specific file/line,
>> but I don't really know...
>>
>> CHECK: DW_TAG_formal_parameter
>> CHECK-NOT: DW_TAG
>> CHECK:   DW_AT_name {{.*}} "var"
>> CHECK-NOT: DW_TAG
>> CHECK:   DW_AT_file
>> CHECK-NOT: DW_TAG
>> CHECK:   DW_AT_line
>>
>> (this also happens to make this test compatible with a change I'm
>> about to make that would cause the DW_AT_location to go from being the
>> last attribute to being the first - I don't think this test needs to
> That sounds like a painful change. What’s the motivation?

Fixing abstract variables as mentioned here:
http://llvm.org/viewvc/llvm-project?rev=209677&view=rev

It's actually not too painful - only about 20 test cases. Not as bad
as doing this for subprograms.

>
>> be examining the DW_AT_location at all, so it shouldn't be affected by
>> that change)
>
> So what we need to preserve in this test is that the variable has type/line/file, obviously the exact order of those attributes is irrelevant. We could add a CHECK-NOT: DW_AT_abstract_origin.

Sounds more sensible.

>
>>
>> And update the comment to describe something related to the original
>> commit message.
>>
>> Or we could just delete the test...
>
> Deleting a test because we don’t fully understand it is never the right thing :-)

I don't entirely agree. If a test case has been lost to the sands of
time such that we might just as well cause it not to test its intended
thing if we try to update/improve it, we might be just as well to
delete it.

- David

>
> -- adrian
>
>>
>>> +; rdar://8950491
>>>
>>> -;CHECK: .long Lset5
>>> -;CHECK-NEXT:        ## DW_AT_decl_file
>>> -;CHECK-NEXT:        ## DW_AT_decl_line
>>> -;CHECK-NEXT:        ## DW_AT_type
>>> -;CHECK-NEXT:        ## DW_AT_location
>>> +;CHECK: DW_TAG_formal_parameter
>>> +;CHECK-NEXT: DW_AT_name {{.*}} "var"
>>> +;CHECK-NEXT: DW_AT_decl_file
>>> +;CHECK-NEXT: DW_AT_decl_line
>>> +;CHECK-NEXT: DW_AT_type
>>> +;CHECK-NEXT: DW_AT_location
>>>
>>> @dfm = external global i32, align 4
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>