[PATCH] LowerDbgDeclare - get rid of redundant dbg.values when an alloca survives optimization
Adrian Prantl
aprantl at apple.com
Mon Apr 21 15:27:41 PDT 2014
Ping.
On Apr 3, 2014, at 2:49 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
> On Apr 3, 2014, at 4:09 AM, Eric Christopher <echristo at gmail.com> wrote:
>
>> Looking at this now:
>>
>> Small nit - It'd be nice if the source you list in the comment in the
>> testcase was actually compilable. I think it just needs extern
>> declarations of lookup and verify though :)
>>
>> The actual patch:
>> I think I'm more worried about creating variables in multiple scopes
>> and then coalescing them rather than trying to avoid creating them in
>> the first place.
>
> You mean roughly like this one? (I didn’t attempt this earlier because I was worried about unwanted side effects with SDag, but after implementing patch #2 (from this thread) I’m actually confident enough to say that this should work).
>
> <0001-Debug-info-Let-dbg.values-inserted-by-LowerDbgDeclar.patch>
>
>
>
>> That said will this code work for variables with
>> disjoint locations? I tried to construct a simple testcase based on
>> this:
>>
>> extern int verify(int m);
>> extern void lookup(int *m);
>> extern void bar(int *a, int *b, int *c, int *d, int *e, int *f, int
>> *g, int *h, int *i, int *j, int *k, int *l);
>> extern void baz(int *a, int *b, int *c, int *d, int *e, int *f, int
>> *g, int *h, int *i, int *j, int *k, int *l);
>> int foo(int map, int *map_test)
>> {
>> int a, b, c, d, e, f, g, h, i, j, k, l;
>> lookup(&map);
>> if (!verify(map)) {
>> bar(&a, &b, &c, &d, &e, &f, &g, &h, &i, &j, &k, &l);
>> } else {
>> baz(&a, &b, &c, &d, &e, &f, &g, &h, &i, &j, &k, &l);
>> lookup(&map);
>> }
>> *map_test = map;
>> return a;
>> }
>>
>> ok, maybe it isn't simple, but it's 4am and compiled it for arm64:
>>
>> ;DEBUG_VALUE: foo:map <- X0
>> .loc 1 9 0 ; foo.c:9:0
>> bl _lookup
>> .loc 1 10 0 ; foo.c:10:0
>> Ltmp7:
>> ldur w0, [fp, #-20]
>> Ltmp8:
>> bl _verify
>> ;DEBUG_VALUE: foo:map_test <- X19
>> add x8, sp, #44
>> add x9, sp, #48
>> add x10, sp, #52
>> add x11, sp, #56
>> cbz w0, LBB0_2
>> Ltmp9:
>> ; BB#1: ; %if.else
>> sub x0, fp, #24
>> Ltmp10:
>> ;DEBUG_VALUE: foo:a <- X0
>>
>> but after the patch your code appears to have foo:map in a single location:
>>
>> 0x00000044: DW_TAG_formal_parameter [3]
>> DW_AT_name [DW_FORM_strp] (
>> .debug_str[0x00000065] = "map")
>> DW_AT_decl_file [DW_FORM_data1] (0x01)
>> DW_AT_decl_line [DW_FORM_data1] (0x06)
>> DW_AT_type [DW_FORM_ref4] (cu + 0x0134 => {0x00000134})
>> DW_AT_location [DW_FORM_exprloc] (<0x2> 91 6c )
>>
>> where I'd have expected a location range since we're not in X0 the entire time?
>
> What you were seeing there _is_ a separate bug and - behold! - it’s fixed by patch #2 in this thread. You can’t compare the dwarf output with the _comments_ in the asm output.
>
>>
>> (This could also just be a separate bug of not tracking the locations
>> better, but I haven't looked much more.)
>>
>
> thanks for the review!
> Adrian
>
>> -eric
>>
>>
>> On Thu, Mar 27, 2014 at 1:54 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>> Hi Eric, David,
>>>
>>> time to revive age-old threads! I looked at this again, and here is the first in a series of patches that aim at improving debugging for optimized code.
>>>
>>> This one eliminates redundant variables being emitted when LowerDbgDeclare is hoisting a dbg.declare into a different lexical block as it happens in the included testcase.
>>>
>>> Next up: dealing with variables that are located at a FrameIndex and described DBG_VALUEs in different parts of the same function.
>>>
>>> -- adrian
>>>
>>>
>>>
>>>
>>>
>>> On Oct 29, 2013, at 1:23 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Tue, Oct 29, 2013 at 1:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Oct 29, 2013 at 1:06 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>
>>>> On Oct 29, 2013, at 11:51, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Oct 29, 2013 at 11:41 AM, Adrian Prantl <aprantl at apple.com> wrote:
>>>>> Hi Eric and David,
>>>>>
>>>>> On Oct 21, 2013, at 17:07, Eric Christopher <echristo at gmail.com> wrote:
>>>>> Cool deal. I've got some ideas on how to refactor/rewrite some of our
>>>>>> loc handling anyhow, but it's going to be a few days before it's
>>>>>> ready.
>>>>>
>>>>>
>>>>> I had some time to look into this some more and I learned a couple of interesting things:
>>>>> - The ##DEBUG_VALUE: comments in the asm output are really just comments do not necessarily show up in the DWARF. This is maybe not surprising, but what was surprising to me are the circumstances under which a DBG_VALUE makes it into the location list of a local variable:
>>>>> - dbg.declare and dbg.value intrinsics are mutually exclusive; the presence of a dbg.declare will shadow any dbg.values that refer to the same variable. As soon as there is an entry in the MMI map (FunctionLowering::set() in FunctionLowering.cpp:134), the dbg.values will be ignored by DwarfDebug::collectVariableInfo (DwarfDebug.cpp::1366).
>>>>> - Simply disabling that check will result in two separate DIEs for the same variable.
>>>>>
>>>>> This means that while my patch was correct based on how DwarfDebug currently behaves, that behavior is broken and I should instead allow for dbg.values and dbg.declares to be coalesced into a single location list.
>>>>>
>>>>> Before I dive into this I'd be curious to hear more about your ideas for location handling that you mentioned in your last mail, so we don't accidentally evolve the code into two different directions.
>>>>>
>>>>>
>>>>> I also found a second, related issue that I'd like to tackle.
>>>>>
>>>>> This example:
>>>>>
>>>>>> foo(int map)
>>>>>> {
>>>>>> lookup(&map);
>>>>>> if (!verify(map)) { }
>>>>>> }
>>>>>>
>>>>>
>>>>> results in the following DWARF:
>>>>>
>>>>>> 0x00000027: TAG_subprogram [2] *
>>>>>> AT_name( "foo" )
>>>>>> AT_low_pc( 0x0000000000000000 )
>>>>>> AT_high_pc( 0x0000000000000026 )
>>>>>> ...
>>>>>> 0x00000046: TAG_formal_parameter [3]
>>>>>> AT_name( "map" )
>>>>>> AT_type( {0x00000075} ( int ) )
>>>>>> AT_location( fbreg -4 )
>>>>>>
>>>>>> 0x00000054: TAG_lexical_block [4] *
>>>>>> AT_low_pc( 0x0000000000000016 )
>>>>>> AT_high_pc( 0x0000000000000020 )
>>>>>>
>>>>>> 0x00000065: TAG_formal_parameter [3]
>>>>>> AT_name( "map" )
>>>>>> AT_type( {0x00000075} ( int ) )
>>>>>> AT_location( fbreg -4 )
>>>>>
>>>>> I would like teach DWARFDebug::constructScopeDIE() to recognize if a variable DIE describes a "subset" of a DIE in one of its parent lexical blocks.
>>>>>
>>>>> To be clear, this just looks buggy - we shouldn't have two formal parameters here... there is only one formal parameter to 'foo'. If we create two DIEs that's a bug, we shouldn't do that.
>>>>>
>>>>
>>>> That would be a fairly trivial fix in DWARFDebug::createScopeChildrenDIE(). I'm wondering: Should there ever be more than one DIE describing a variable in different lexical scopes or should there be only ever a single DIE with multiple entries in the DW_AT_location list?
>>>>
>>>> The latter, I'm fairly sure.
>>>>
>>>> Yes.
>>>>
>>>> -eric
>>>
>>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list