| Message ID | CAO2gOZUGYop71A25c33wUNbjmJ9idzp7iy5Vk6C5hAD0v1ZONw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. */
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. */
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. */
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. */
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. */
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. */