Patchwork Add a lexical block only when the callsite has source location info

login
register
mail settings
Submitter Dehao Chen
Date June 26, 2012, 7:03 a.m.
Message ID <CAO2gOZW7PMBskpms30Me07Bm6zLc4fJY0QPaE0_5ysGyg7Kqhw@mail.gmail.com>
Download mbox | patch
Permalink /patch/167318/
State New
Headers show

Comments

Dehao Chen - June 26, 2012, 7:03 a.m.
Hi, Richard,

You are right, setting UNKNOWN_LOCATION will not affect addr2line
result. Here is the updated patch:

Passed bootstrap and gcc regression tests.

Is it ok for trunk?

Thanks,
Dehao


On Mon, Jun 25, 2012 at 10:00 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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.  */
Richard Guenther - June 26, 2012, 8:42 a.m.
On Tue, Jun 26, 2012 at 9:03 AM, Dehao Chen <dehao@google.com> wrote:
> Hi, Richard,
>
> You are right, setting UNKNOWN_LOCATION will not affect addr2line
> result. Here is the updated patch:
>
> Passed bootstrap and gcc regression tests.
>
> Is it ok for trunk?

Yes.

Thanks,
Richard.

> Thanks,
> Dehao
>
> Index: tree-inline.c
> ===================================================================
> --- tree-inline.c       (revision 188926)
> +++ tree-inline.c       (working copy)
> @@ -3836,8 +3836,7 @@
>   /* 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);
> +  input_location = gimple_location (stmt);
>
>   /* From here on, we're only interested in CALL_EXPRs.  */
>   if (gimple_code (stmt) != GIMPLE_CALL)
>
> On Mon, Jun 25, 2012 at 10:00 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> 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.  */

Patch

Index: tree-inline.c
===================================================================
--- tree-inline.c	(revision 188926)
+++ tree-inline.c	(working copy)
@@ -3836,8 +3836,7 @@ 
   /* 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);
+  input_location = gimple_location (stmt);

   /* From here on, we're only interested in CALL_EXPRs.  */
   if (gimple_code (stmt) != GIMPLE_CALL)