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

login
register
mail settings
Submitter Dehao Chen
Date June 25, 2012, 11:43 a.m.
Message ID <CAO2gOZUGYop71A25c33wUNbjmJ9idzp7iy5Vk6C5hAD0v1ZONw@mail.gmail.com>
Download mbox | patch
Permalink /patch/167040/
State New
Headers show

Comments

Dehao Chen - June 25, 2012, 11:43 a.m.
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. 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?

Thanks,
Dehao

gcc/ChangeLog:
2012-06-25  Dehao Chen  <dehao@google.com>

	* tree-profile.c: (expand_call_inline): Make a new lexical block only
	when the call stmt has source location.
Richard Guenther - June 25, 2012, 12:02 p.m.
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?

> 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.  */
Dehao Chen - June 25, 2012, 1:48 p.m.
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?

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 25, 2012, 2 p.m.
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.  */
Xinliang David Li - July 10, 2012, 5:57 a.m.
Is this related to the problem described in
http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01511.html ?

David

On Mon, Jun 25, 2012 at 4:43 AM, 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. 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?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2012-06-25  Dehao Chen  <dehao@google.com>
>
>         * tree-profile.c: (expand_call_inline): Make a new lexical block only
>         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;
> +      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.  */
Dehao Chen - July 10, 2012, 7:17 a.m.
On Tue, Jul 10, 2012 at 1:57 PM, Xinliang David Li <davidxl@google.com> wrote:
> Is this related to the problem described in
> http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01511.html ?

This does not sounds related to me. This patch only fix the block info
for phi_arg_t.

The following patch is related to function split, but only for debug info.

http://gcc.gnu.org/ml/gcc-patches/2012-06/msg01579.html

Thanks,
Dehao

>
> David
>
> On Mon, Jun 25, 2012 at 4:43 AM, 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. 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?
>>
>> Thanks,
>> Dehao
>>
>> gcc/ChangeLog:
>> 2012-06-25  Dehao Chen  <dehao@google.com>
>>
>>         * tree-profile.c: (expand_call_inline): Make a new lexical block only
>>         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;
>> +      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: 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;
+      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.  */