On Wed, Jul 18, 2012 at 1:04 AM, Dehao Chen <de...@google.com> wrote:
> Hi, Dodji,
>
> Thanks for the comments.
>
> On Tue, Jul 17, 2012 at 11:32 PM, Dodji Seketeli <do...@redhat.com> wrote:
>>
>> Dehao Chen <de...@google.com> writes:
>>
>> > Thanks Richard,
>> >
>> > Hi, Dodji,
>>
>> Hi Dehao,
>>
>> > In gcc, we've met many cases where location_t and block are inconsistent
>> > in
>> > the debug info. This generates very confusing addr2line results. This is
>> > mainly due to the fact that some data structures does not updates its
>> > block
>> > info as expected (e.g. phi_arg_t). I'm planning to clean this up by
>> > encoding block into the location_t. In this way, we do not need to
>> > update
>> > block whenever we do transformation. And we may even be able to
>> > eliminate
>> > the "block" field in gimple (as soon as we can derive that from
>> > location_t). To do that, we need to add a field "void *block" in
>> > expanded_location in libcpp/include/line-map.h. Do you think this is OK?
>> > Or
>> > any comments will be welcome.
>>
>> Richard, Jan, Jakub and myself discussed this on the public IRC of GCC
>> and it seems like we reached a sort of modus vivendi for this. They are
>> in CC of this message.
>>
>> I don't think you can just add a pointer to a block in the
>> expanded_location structure like that. That would imply that you'd need
>> to be able to simply associate an instance of location_t handed out
>> (encoded) by a line map, with a block. The problem is that the
>> location_t integer itself already encodes a column number and a line
>> number. And a given set of instances of location_t (those handed out by
>> a given line map) share the same line map. And a line map struct
>> carries things like the file name, whether the file is a system file,
>> etc. To decode an instance of location_t, it takes looking up which
>> line map it was encoded from. Once we have the map we can get the file
>> path from it, and we can decode the line/columns numbers directly from
>> the value of the location_t instance itself. These values are then put
>> into a newly allocated expanded_location.
>>
>> All this to say that "just" adding a pointer to the expanded_location
>> wouldn't work. You need to way to e.g. associate the location_t to the
>> block.
>
>
> Sorry that I may mis-addressed the issue. What I was saying is that adding a
> pointer to expanded_location is just the first step. We difinitely need to
> modify the linemap to associate the pointer with the location_t.
>
>>
>>
>> From our discussion, it looks like a good way to go would be to do
>> something like what the RTL infrastructure does with insn locators (e.g
>> in emit-rtl.c, starting from bloc_locators_locs and going to
>> locator_eq).
>>
>> Basically create a new "block_locator" integer that would associated
>> with a {location_t, block} pair. There would be a vector of
>> 'block_locator's. The indexes of the 'block_locator's would index a
>> vector of {location_t, block} pair. That way, looking up the
>> {location_t, block} that corresponds to a given block_location would be
>> possible. This is or a variation along these lines.
>>
>
> I got your point. But one more thing: we need to change location_t in all
> data structure into this index, right? E.g., gimple_location would return
> this index instead of original location_t? And we also want to change
> expand_location(location_t) into expand_location(index)?
The question is what to do with fold () and friends - on GENERIC we use
EXPR_LOCATION on tcc_expression nodes and a block pointer in
struct tree_exp. Now, currently fold () only has _loc variants (as you noticed)
and for correctness we'd need to pass block there, too.
So - the question is whether to avoid all this and change tree_exp.locus/block
to block_location, too (which obviously would affect frontends and all of fold).
If we don't do that all of the middle-end that uses fold () and then
force_gimple_operand () and friends would lose the block part of the location.
CC'in the ml again, this is of general interest and others may have comments.
Richard.
> An alternative approach would be simply add "block" as part of the hash
> function in the linemap. In this way, whenever the block is changed, we can
> simply add a new location_t number. The pros of this approach is that the
> changes is kept minimum. The cons of this approach is that location_t is
> globally numbered. What do you think?
>
> Thanks,
> Dehao
>
>
>>
>> There would be one such association for each current function where we
>> need it. That association would be then be saved in the function data
>> structure of the current function so we don't loose it whenever we
>> switch functions.
>>
>> The mid-term goal would then be to unify this with the insns locators
>> used by the RTL infrastructure.
>>
>> I hope this helps.
>>
>> Dodji
>> >
>> > Thanks,
>> > Dehao
>> >
>> >
>> > On Tue, Jul 17, 2012 at 5:27 PM, Richard Guenther <
>> > richard.guent...@gmail.com> wrote:
>> >
>> >> On Tue, Jul 17, 2012 at 7:23 AM, Dehao Chen <de...@google.com> wrote:
>> >> > Hi, Richard,
>> >> >
>> >> > I'm planning to add a field (tree block;) to the expanded_location
>> >> > (in
>> >> > libcpp/include/line-map.h). And when the block info is modified
>> >> > (during
>> >> > function inlining, etc.), we invoke linemap_add to create a new
>> >> location_t
>> >> > for it. Do you think this is gonna work?
>> >>
>> >> Adding a field of type 'tree' is of course not good (a void * would
>> >> work I guess).
>> >> I briefly talked with Dodji about this at the GNU Cauldron - CCing him
>> >> here
>> >> because he knows libcpp a lot better than me.
>> >>
>> >> Richard.
>> >>
>> >> >
>> >> > Thanks,
>> >> > Dehao
>> >> >
>> >> >
>> >> > On Tue, Jul 10, 2012 at 4:59 PM, Richard Guenther
>> >> > <richard.guent...@gmail.com> wrote:
>> >> >>
>> >> >> On Tue, Jul 10, 2012 at 9:20 AM, Dehao Chen <de...@google.com>
>> >> >> wrote:
>> >> >> > Hi, Richard,
>> >> >> >
>> >> >> > I found that I also need to modify the following interface:
>> >> >> >
>> >> >> > tree fold_build2_stat_loc (location_t, enum tree_code, tree, tree,
>> >> >> > tree MEM_STAT_DECL);
>> >> >> >
>> >> >> > to
>> >> >> >
>> >> >> > tree fold_build2_stat_loc (tree block, location_t, enum tree_code,
>> >> tree,
>> >> >> > tree,
>> >> >> > tree MEM_STAT_DECL);
>> >> >> >
>> >> >> > This will affect some files, but not as many as this patch.
>> >> >> >
>> >> >> > Do you agree for change like this? If yes, I'll send out the patch
>> >> soon.
>> >> >>
>> >> >> No, this is surely not wanted. It would make the fold_build2_loc
>> >> >> prototype non-canonical with all the rest. Rather than make every
>> >> >> place taking a location_t also take a block argument please work on
>> >> >> providing a location similar to the RTL infrastructure which
>> >> >> combines
>> >> >> location_t and block.
>> >> >>
>> >> >> Richard.
>> >> >>
>> >> >>
>> >> >>
>> >> >> > Thanks,
>> >> >> > Dehao
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > On Tue, Jul 10, 2012 at 3:17 PM, Dehao Chen <de...@google.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Tue, Jul 10, 2012 at 1:57 PM, Xinliang David Li <
>> >> davi...@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 <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. 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 <de...@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. */
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >>
>
>
