Re: [patch] Add a lexical block only when the callsite has source location info

[Richard Guenther <richard dot guenther at gmail dot com>]

On Mon, Jun 25, 2012 at 3:48 PM, Dehao Chen <dehao@google.com> wrote:
> Hi, Richard,
>
> Thanks for the prompt response.
>
> On Mon, Jun 25, 2012 at 8:02 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, Jun 25, 2012 at 1:43 PM, Dehao Chen <dehao@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?

I don't think it behaves sensibly for any other call without a location.
Basically you assume that this only happens for split functions but
I don't see why that should be true.  Why would BLOCKs with
UNKOWN_LOCATION have any effect on addr2line anyways?
That seems to be something to check and fix.

Richard.


> 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 ?<dehao@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. ?*/