[patch] PR15662: reordered function parameters when inlining

[David Blaikie]

On Tue, Jun 4, 2013 at 4:46 PM, Eric Christopher <echristo at gmail.com> wrote:
> I think this is fine in general, couple of comments on the patch itself:
>
> a) comment it with, basically, your commit message :)

Done (rephrased to be comment appropriate, let me know if there's
other wordsmithing you think would work better)

> b) please use 'I' for your iterator variable, having a lower case j as
> an index and the lower case i as the iterator makes it more difficult
> to see what's what IMO

& done.

Committed in r183297.

> Still a shame about DW_TAG_arg_variable, but it's to be put up with at
> the moment.

Yeah, I think I experimented with a change to just use
formal_parameter for the tag name at least (to remove one more of our
pseudo tag names) but had some trouble with that for some reason.
That'd be a start - the underlying issue (of representing
parameters/function types as variables) is perhaps harder to deal with
- really they are variables, but we might want to track them in some
more absolute fashion & have the actual dbg_variables refer to those
more formal definitions somehow.

>
> Thanks!
>
> -eric
>
> On Tue, Jun 4, 2013 at 4:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> This fixes PR15662 as follows:
>>
>>     PR15662: Optimized debug info produces out of order function parameters
>>
>>     When a function is inlined we lazily construct the variables
>>     representing the function's parameters. After that, we add any remaining
>>     unused parameters.
>>
>>     If the function doesn't use all the parameters, or uses them out of
>>     order, then the DWARF would produce them in that order, producing a
>>     parameter order that doesn't match the source.
>>
>>     This fix causes us to always keep the arg variables at the start of the
>>     variable list & in the original order from the source.
>>
>> The only issue is that this slows down slightly the non-opt case (we'd
>> bail out when we find the variables list attached to the subprogram is
>> empty) & moreso in the opt case (since we have to do the
>> search/ordering work). We could do a bit more to make the loop at
>> DwarfDebug.cpp:1679 be more efficient (rather than calling
>> addScopeVariable each time causing a new search for the insertion
>> point, we could factor it out to find/use the insertion point
>> incrementally). I also didn't actually try to find a test case where
>> variables were used in the inline function, but out of order. If such
>> a case doesn't exist, then there would be no need to handle insertion
>> order in the lazy/inlining case - just at the "put the rest in" at the
>> end (the aforementioned loop). As it is, we could still make the lazy
>> case better by continuing to be lazy, then re-ordering them just once
>> at the end.
>>
>> Open to suggestions/preferences/ideas.