Hi, Richard,
Thanks for the prompt response.
On Mon, Jun 25, 2012 at 8:02 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Mon, Jun 25, 2012 at 1:43 PM, Dehao Chen <de...@google.com> wrote:
>> During function inlining, a lexical block is added for each cloned
>> callee, and source info is attached to this block for addr2line to
>> derive the inline stack.
>
> Well - the bug is then clearly
>
> /* Set input_location here so we get the right instantiation context
> if we call instantiate_decl from inlinable_function_p. */
> saved_location = input_location;
> if (gimple_has_location (stmt))
> input_location = gimple_location (stmt)
>
> which retails input_location instead of setting it to UNKNOWN_LOCATION.
>
> Not adding a BLOCK will make debug information incorrect, no?
The only case I can think of that gimple_has_location is false for
call stmt is for function split.
If we have function foo, which is split into:
foo
foo.part1
And a callsite foo->foo.part1 is created in foo.
If the ipa-inline decided to inline this callsite, for an instruction
in foo.part1, it will have an inline stack of size 2. In the original
buggy code, the bottom of the inline stack will be random. Using your
proposed approach, the bottom of the inline stack would be
UNKNOW_LOCATION, but still has two levels. For function split, this
inline will not create any lexical block, but resumes the original
lexical block before the split. Thus my change simply not add a new
lexical block. Do you think this makes sense?
Thanks,
Dehao
>
>> However, some callsites do not have source
>> information attached to it. Adding a lexical block would be misleading
>> in this case. E.g. If a function is split, when the split callsite is
>> inlined back, the cloned callee should stay in the same lexical block
>> with its caller. This patch ensures that lexical blocks are only added
>> when the callsite has source location info in it.
>>
>> Bootstrapped and passed gcc regression tests.
>>
>> Is it ok for trunk?
>
> I'd rather see an unconditional set of input_location from gimple_location
> of the statement.
>
> Richard.
>
>> Thanks,
>> Dehao
>>
>> gcc/ChangeLog:
>> 2012-06-25 Dehao Chen <de...@google.com>
>>
>> * tree-profile.c: (expand_call_inline): Make a new lexical block only
>
> ^^^^^
> tree-inline.c
>
>> when the call stmt has source location.
>>
>> Index: gcc/tree-inline.c
>> ===================================================================
>> --- gcc/tree-inline.c (revision 188926)
>> +++ gcc/tree-inline.c (working copy)
>> @@ -3950,10 +3950,17 @@
>> actual inline expansion of the body, and a label for the return
>> statements within the function to jump to. The type of the
>> statement expression is the return type of the function call. */
>> - id->block = make_node (BLOCK);
>> - BLOCK_ABSTRACT_ORIGIN (id->block) = fn;
>> - BLOCK_SOURCE_LOCATION (id->block) = input_location;
>> - prepend_lexical_block (gimple_block (stmt), id->block);
>> + if (gimple_has_location (stmt))
>> + {
>> + id->block = make_node (BLOCK);
>> + BLOCK_ABSTRACT_ORIGIN (id->block) = fn;
>> + BLOCK_SOURCE_LOCATION (id->block) = input_location;
>
> Please use gimple_location (stmt) instead of input_location (yes, I realize
> its set from that).
>
>> + prepend_lexical_block (gimple_block (stmt), id->block);
>> + }
>> + else
>> + {
>> + id->block = gimple_block (stmt);
>> + }
>
>> /* Local declarations will be replaced by their equivalents in this
>> map. */
