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 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.guent...@gmail.com> wrote: > On Mon, Jun 25, 2012 at 3:48 PM, Dehao Chen <de...@google.com> wrote: >> 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? > > 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 <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. */