Patchwork Fix debug info for expr and jump stmt

login
register
mail settings
Submitter Dehao Chen
Date Oct. 26, 2012, 10:53 p.m.
Message ID <CAO2gOZU3HafUBWN00OvDqpM-qkjQ41nzXpsBMY_U=0Z51XZvRw@mail.gmail.com>
Download mbox | patch
Permalink /patch/194576/
State New
Headers show

Comments

Dehao Chen - Oct. 26, 2012, 10:53 p.m.
Hi,

I've updated the patch:

1. abandon the changes in cfgexpand.c
2. set the block for trees when lowering gimple stmt.
3. add a unittest.

However, this patch will trigger two lto bug when asserting
LTO_NO_PREVAIL for TREE_CHAIN. After debugging for a while, I found
that the problem was also there even without the patch. This patch
just reveal the problem by moving a decl into cache so that it will be
checked. As I'm not familiar with LTO, not quite sure what the root
problem is. Can anyone help take a look?

Thanks,
Dehao

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

        * tree-eh.c (do_return_redirection): Set location for jump statement.
        (do_goto_redirection): Likewise.
        (frob_into_branch_around): Likewise.
        (lower_try_finally_nofallthru): Likewise.
        (lower_try_finally_copy): Likewise.
        (lower_try_finally_switch): Likewise.
        * gimple-low.c (tree_set_block_r): New callback function.
        (lower_stmt): Set block for tested expr.

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

        * g++.dg/debug/dwarf2/block.C: New testcase.
Michael Matz - Oct. 29, 2012, 1:51 p.m.
Hi,

On Fri, 26 Oct 2012, Dehao Chen wrote:

> 1. abandon the changes in cfgexpand.c

Well, you merely moved the bogus code to gimple-low.c.  It is bogus 
because you unconditionally overwrite TREE_BLOCK of all operands (and all 
operands operands) with the statements block.  I assume the unit-test you 
added shows the problem you were trying to fix?

From the scan-assembler-no directive I'm not really sure what exactly the 
problem is you're seeing.  Can you try to describe it with words for the 
example code?  Which operands has no tree-block where it should have one, 
or which one has the wrong tree-block?


Ciao,
Michael.
Richard Guenther - Oct. 29, 2012, 2:11 p.m.
On Mon, Oct 29, 2012 at 2:51 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Fri, 26 Oct 2012, Dehao Chen wrote:
>
>> 1. abandon the changes in cfgexpand.c
>
> Well, you merely moved the bogus code to gimple-low.c.  It is bogus
> because you unconditionally overwrite TREE_BLOCK of all operands (and all
> operands operands) with the statements block.  I assume the unit-test you
> added shows the problem you were trying to fix?
>
> From the scan-assembler-no directive I'm not really sure what exactly the
> problem is you're seeing.  Can you try to describe it with words for the
> example code?  Which operands has no tree-block where it should have one,
> or which one has the wrong tree-block?

trees without block could be an issue when we extract them to some other
statement (then without block), and move that to some random place.
But the only issue should be that the stmt/expressions effective block
becomes a different one (the currently "active" one during expansion).
I don't see how we could end up generating too many block location
DIEs because of this.

Does the issue vanish when using -fno-tree-ter?

And yes, I agree, the patch looks bogus.

Richard.

>
> Ciao,
> Michael.
Michael Matz - Oct. 29, 2012, 2:17 p.m.
Hi,

On Mon, 29 Oct 2012, Richard Biener wrote:

> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
> > operands operands) with the statements block.  I assume the unit-test you
> > added shows the problem you were trying to fix?
> >
> > From the scan-assembler-no directive I'm not really sure what exactly the
> > problem is you're seeing.  Can you try to describe it with words for the
> > example code?  Which operands has no tree-block where it should have one,
> > or which one has the wrong tree-block?
> 
> trees without block could be an issue when we extract them to some other
> statement (then without block), and move that to some random place.

Even then it would be either the frontends job to set the block, or the 
the job of the pass that introduced the new expression, when there is a 
clearly sane candidate for the block.

> But the only issue should be that the stmt/expressions effective block
> becomes a different one (the currently "active" one during expansion).

Yep.

> I don't see how we could end up generating too many block location
> DIEs because of this.

And even if, I don't see what's the _problem_ with too many block 
locations, except bloat.


Ciao,
Michael.
Dehao Chen - Oct. 29, 2012, 3:25 p.m.
On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 29 Oct 2012, Richard Biener wrote:
>
>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all

Emm, then in gimple-low.c, we should probably not unconditionally
overwrite gimple_block for stmts too?

>> > operands operands) with the statements block.  I assume the unit-test you
>> > added shows the problem you were trying to fix?
>> >
>> > From the scan-assembler-no directive I'm not really sure what exactly the
>> > problem is you're seeing.  Can you try to describe it with words for the
>> > example code?  Which operands has no tree-block where it should have one,
>> > or which one has the wrong tree-block?
>>
>> trees without block could be an issue when we extract them to some other
>> statement (then without block), and move that to some random place.
>
> Even then it would be either the frontends job to set the block, or the
> the job of the pass that introduced the new expression, when there is a
> clearly sane candidate for the block.

Please correct me if I'm wrong: front-end does not set blocks for
either stmt/expr. lower_stmt is the first place that block is set for
stmt, thus I think it should also be the first place to set block for
expr.

>
>> But the only issue should be that the stmt/expressions effective block
>> becomes a different one (the currently "active" one during expansion).

I agree. Initially, both stmt and expressions is set to NULL block.
Whenever we update the stmt's block, we should also update the
expression's block, otherwise they will be inconsistent.

>
> Yep.
>
>> I don't see how we could end up generating too many block location
>> DIEs because of this.

For the unittest I added, all the assembly code guarded by "if" are
should be within one lexical block, which is lock:

  LBB1
  {
    asm1
    asm2
    asm3
    asm4
    ....
  }

However, because some expression are with NULL lexical block, these
assembly codes are separated by several discontinual lexical block
which is like:

   LBB1
    {
       asm1
    }
  asm2
  LBB2
    {
       asm3
    }
  asm4
  ....

>
> And even if, I don't see what's the _problem_ with too many block
> locations, except bloat.

Yes, bloat is the major problem.

Thanks,
Dehao

>
>
> Ciao,
> Michael.
Richard Guenther - Oct. 29, 2012, 4:10 p.m.
On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote:
> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
>> Hi,
>>
>> On Mon, 29 Oct 2012, Richard Biener wrote:
>>
>>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
>>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
>
> Emm, then in gimple-low.c, we should probably not unconditionally
> overwrite gimple_block for stmts too?

gimple stmts have no block before gimple-low.

>>> > operands operands) with the statements block.  I assume the unit-test you
>>> > added shows the problem you were trying to fix?
>>> >
>>> > From the scan-assembler-no directive I'm not really sure what exactly the
>>> > problem is you're seeing.  Can you try to describe it with words for the
>>> > example code?  Which operands has no tree-block where it should have one,
>>> > or which one has the wrong tree-block?
>>>
>>> trees without block could be an issue when we extract them to some other
>>> statement (then without block), and move that to some random place.
>>
>> Even then it would be either the frontends job to set the block, or the
>> the job of the pass that introduced the new expression, when there is a
>> clearly sane candidate for the block.
>
> Please correct me if I'm wrong: front-end does not set blocks for
> either stmt/expr. lower_stmt is the first place that block is set for
> stmt, thus I think it should also be the first place to set block for
> expr.

No, only the block on the gimple stmt matters for gimple.  You should not
need to touch the operands block.

>>
>>> But the only issue should be that the stmt/expressions effective block
>>> becomes a different one (the currently "active" one during expansion).
>
> I agree. Initially, both stmt and expressions is set to NULL block.
> Whenever we update the stmt's block, we should also update the
> expression's block, otherwise they will be inconsistent.

I don't think so.

>>
>> Yep.
>>
>>> I don't see how we could end up generating too many block location
>>> DIEs because of this.
>
> For the unittest I added, all the assembly code guarded by "if" are
> should be within one lexical block, which is lock:
>
>   LBB1
>   {
>     asm1
>     asm2
>     asm3
>     asm4
>     ....
>   }
>
> However, because some expression are with NULL lexical block, these
> assembly codes are separated by several discontinual lexical block
> which is like:
>
>    LBB1
>     {
>        asm1
>     }
>   asm2

asm2 should have gone into the currently open scope (similiar for what
we do for line number information).

>   LBB2
>     {
>        asm3
>     }
>   asm4
>   ....
>
>>
>> And even if, I don't see what's the _problem_ with too many block
>> locations, except bloat.
>
> Yes, bloat is the major problem.
>
> Thanks,
> Dehao
>
>>
>>
>> Ciao,
>> Michael.
Dehao Chen - Oct. 29, 2012, 4:49 p.m.
On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote:
>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
>>> Hi,
>>>
>>> On Mon, 29 Oct 2012, Richard Biener wrote:
>>>
>>>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
>>>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
>>
>> Emm, then in gimple-low.c, we should probably not unconditionally
>> overwrite gimple_block for stmts too?
>
> gimple stmts have no block before gimple-low.
>
>>>> > operands operands) with the statements block.  I assume the unit-test you
>>>> > added shows the problem you were trying to fix?
>>>> >
>>>> > From the scan-assembler-no directive I'm not really sure what exactly the
>>>> > problem is you're seeing.  Can you try to describe it with words for the
>>>> > example code?  Which operands has no tree-block where it should have one,
>>>> > or which one has the wrong tree-block?
>>>>
>>>> trees without block could be an issue when we extract them to some other
>>>> statement (then without block), and move that to some random place.
>>>
>>> Even then it would be either the frontends job to set the block, or the
>>> the job of the pass that introduced the new expression, when there is a
>>> clearly sane candidate for the block.
>>
>> Please correct me if I'm wrong: front-end does not set blocks for
>> either stmt/expr. lower_stmt is the first place that block is set for
>> stmt, thus I think it should also be the first place to set block for
>> expr.
>
> No, only the block on the gimple stmt matters for gimple.  You should not
> need to touch the operands block.
>
>>>
>>>> But the only issue should be that the stmt/expressions effective block
>>>> becomes a different one (the currently "active" one during expansion).
>>
>> I agree. Initially, both stmt and expressions is set to NULL block.
>> Whenever we update the stmt's block, we should also update the
>> expression's block, otherwise they will be inconsistent.
>
> I don't think so.

Hi, Richard,

Can you share some insights why you don't think we want to make stmt
and expr's block consistent?

>
>>>
>>> Yep.
>>>
>>>> I don't see how we could end up generating too many block location
>>>> DIEs because of this.
>>
>> For the unittest I added, all the assembly code guarded by "if" are
>> should be within one lexical block, which is lock:
>>
>>   LBB1
>>   {
>>     asm1
>>     asm2
>>     asm3
>>     asm4
>>     ....
>>   }
>>
>> However, because some expression are with NULL lexical block, these
>> assembly codes are separated by several discontinual lexical block
>> which is like:
>>
>>    LBB1
>>     {
>>        asm1
>>     }
>>   asm2
>
> asm2 should have gone into the currently open scope (similiar for what
> we do for line number information).

In location_block patch, we changed this not to fall into the
currently open scope, because we want every insn to have correct
location/block with it. This is one of the reasons why we introduced
the location_block patch: it makes it easy to update block for
stmt/expr/phi_arg thus keeping a consistent location/block pair for
all instructions becomes possible.

Yes, let location/block fall into the currently open scope/location
was a workaround before, but if we are able to make the location/block
pair consistent all through compilation, we should do that, right?

Thanks,
Dehao

>
>>   LBB2
>>     {
>>        asm3
>>     }
>>   asm4
>>   ....
>>
>>>
>>> And even if, I don't see what's the _problem_ with too many block
>>> locations, except bloat.
>>
>> Yes, bloat is the major problem.
>>
>> Thanks,
>> Dehao
>>
>>>
>>>
>>> Ciao,
>>> Michael.
Dehao Chen - Oct. 29, 2012, 5:07 p.m.
Hi, Micheal,

Thanks for explaining the design principle of debug info with gimple,
now I can understand your concerns. And thanks for providing the
patch.

However, in some places after gimplification (e.g. tree-inline.c), we
still updates the block/location info. And EXPR_LOCATION is still used
widely after gimplification. Do you mean that in the long run, we'd
want to remove all these?

Thanks,
Dehao

On Mon, Oct 29, 2012 at 9:49 AM, Dehao Chen <dehao@google.com> wrote:
> On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote:
>>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
>>>> Hi,
>>>>
>>>> On Mon, 29 Oct 2012, Richard Biener wrote:
>>>>
>>>>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
>>>>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
>>>
>>> Emm, then in gimple-low.c, we should probably not unconditionally
>>> overwrite gimple_block for stmts too?
>>
>> gimple stmts have no block before gimple-low.
>>
>>>>> > operands operands) with the statements block.  I assume the unit-test you
>>>>> > added shows the problem you were trying to fix?
>>>>> >
>>>>> > From the scan-assembler-no directive I'm not really sure what exactly the
>>>>> > problem is you're seeing.  Can you try to describe it with words for the
>>>>> > example code?  Which operands has no tree-block where it should have one,
>>>>> > or which one has the wrong tree-block?
>>>>>
>>>>> trees without block could be an issue when we extract them to some other
>>>>> statement (then without block), and move that to some random place.
>>>>
>>>> Even then it would be either the frontends job to set the block, or the
>>>> the job of the pass that introduced the new expression, when there is a
>>>> clearly sane candidate for the block.
>>>
>>> Please correct me if I'm wrong: front-end does not set blocks for
>>> either stmt/expr. lower_stmt is the first place that block is set for
>>> stmt, thus I think it should also be the first place to set block for
>>> expr.
>>
>> No, only the block on the gimple stmt matters for gimple.  You should not
>> need to touch the operands block.
>>
>>>>
>>>>> But the only issue should be that the stmt/expressions effective block
>>>>> becomes a different one (the currently "active" one during expansion).
>>>
>>> I agree. Initially, both stmt and expressions is set to NULL block.
>>> Whenever we update the stmt's block, we should also update the
>>> expression's block, otherwise they will be inconsistent.
>>
>> I don't think so.
>
> Hi, Richard,
>
> Can you share some insights why you don't think we want to make stmt
> and expr's block consistent?
>
>>
>>>>
>>>> Yep.
>>>>
>>>>> I don't see how we could end up generating too many block location
>>>>> DIEs because of this.
>>>
>>> For the unittest I added, all the assembly code guarded by "if" are
>>> should be within one lexical block, which is lock:
>>>
>>>   LBB1
>>>   {
>>>     asm1
>>>     asm2
>>>     asm3
>>>     asm4
>>>     ....
>>>   }
>>>
>>> However, because some expression are with NULL lexical block, these
>>> assembly codes are separated by several discontinual lexical block
>>> which is like:
>>>
>>>    LBB1
>>>     {
>>>        asm1
>>>     }
>>>   asm2
>>
>> asm2 should have gone into the currently open scope (similiar for what
>> we do for line number information).
>
> In location_block patch, we changed this not to fall into the
> currently open scope, because we want every insn to have correct
> location/block with it. This is one of the reasons why we introduced
> the location_block patch: it makes it easy to update block for
> stmt/expr/phi_arg thus keeping a consistent location/block pair for
> all instructions becomes possible.
>
> Yes, let location/block fall into the currently open scope/location
> was a workaround before, but if we are able to make the location/block
> pair consistent all through compilation, we should do that, right?
>
> Thanks,
> Dehao
>
>>
>>>   LBB2
>>>     {
>>>        asm3
>>>     }
>>>   asm4
>>>   ....
>>>
>>>>
>>>> And even if, I don't see what's the _problem_ with too many block
>>>> locations, except bloat.
>>>
>>> Yes, bloat is the major problem.
>>>
>>> Thanks,
>>> Dehao
>>>
>>>>
>>>>
>>>> Ciao,
>>>> Michael.
Richard Guenther - Oct. 29, 2012, 6:13 p.m.
On Mon, Oct 29, 2012 at 6:07 PM, Dehao Chen <dehao@google.com> wrote:
> Hi, Micheal,
>
> Thanks for explaining the design principle of debug info with gimple,
> now I can understand your concerns. And thanks for providing the
> patch.
>
> However, in some places after gimplification (e.g. tree-inline.c), we
> still updates the block/location info. And EXPR_LOCATION is still used
> widely after gimplification. Do you mean that in the long run, we'd
> want to remove all these?

Yes.  Unfortunately many of the core inlining routines are also used by
the C++ frontend for template instantiation...

Richard.

> Thanks
> Dehao
>
> On Mon, Oct 29, 2012 at 9:49 AM, Dehao Chen <dehao@google.com> wrote:
>> On Mon, Oct 29, 2012 at 9:10 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote:
>>>> On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, 29 Oct 2012, Richard Biener wrote:
>>>>>
>>>>>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
>>>>>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
>>>>
>>>> Emm, then in gimple-low.c, we should probably not unconditionally
>>>> overwrite gimple_block for stmts too?
>>>
>>> gimple stmts have no block before gimple-low.
>>>
>>>>>> > operands operands) with the statements block.  I assume the unit-test you
>>>>>> > added shows the problem you were trying to fix?
>>>>>> >
>>>>>> > From the scan-assembler-no directive I'm not really sure what exactly the
>>>>>> > problem is you're seeing.  Can you try to describe it with words for the
>>>>>> > example code?  Which operands has no tree-block where it should have one,
>>>>>> > or which one has the wrong tree-block?
>>>>>>
>>>>>> trees without block could be an issue when we extract them to some other
>>>>>> statement (then without block), and move that to some random place.
>>>>>
>>>>> Even then it would be either the frontends job to set the block, or the
>>>>> the job of the pass that introduced the new expression, when there is a
>>>>> clearly sane candidate for the block.
>>>>
>>>> Please correct me if I'm wrong: front-end does not set blocks for
>>>> either stmt/expr. lower_stmt is the first place that block is set for
>>>> stmt, thus I think it should also be the first place to set block for
>>>> expr.
>>>
>>> No, only the block on the gimple stmt matters for gimple.  You should not
>>> need to touch the operands block.
>>>
>>>>>
>>>>>> But the only issue should be that the stmt/expressions effective block
>>>>>> becomes a different one (the currently "active" one during expansion).
>>>>
>>>> I agree. Initially, both stmt and expressions is set to NULL block.
>>>> Whenever we update the stmt's block, we should also update the
>>>> expression's block, otherwise they will be inconsistent.
>>>
>>> I don't think so.
>>
>> Hi, Richard,
>>
>> Can you share some insights why you don't think we want to make stmt
>> and expr's block consistent?
>>
>>>
>>>>>
>>>>> Yep.
>>>>>
>>>>>> I don't see how we could end up generating too many block location
>>>>>> DIEs because of this.
>>>>
>>>> For the unittest I added, all the assembly code guarded by "if" are
>>>> should be within one lexical block, which is lock:
>>>>
>>>>   LBB1
>>>>   {
>>>>     asm1
>>>>     asm2
>>>>     asm3
>>>>     asm4
>>>>     ....
>>>>   }
>>>>
>>>> However, because some expression are with NULL lexical block, these
>>>> assembly codes are separated by several discontinual lexical block
>>>> which is like:
>>>>
>>>>    LBB1
>>>>     {
>>>>        asm1
>>>>     }
>>>>   asm2
>>>
>>> asm2 should have gone into the currently open scope (similiar for what
>>> we do for line number information).
>>
>> In location_block patch, we changed this not to fall into the
>> currently open scope, because we want every insn to have correct
>> location/block with it. This is one of the reasons why we introduced
>> the location_block patch: it makes it easy to update block for
>> stmt/expr/phi_arg thus keeping a consistent location/block pair for
>> all instructions becomes possible.
>>
>> Yes, let location/block fall into the currently open scope/location
>> was a workaround before, but if we are able to make the location/block
>> pair consistent all through compilation, we should do that, right?
>>
>> Thanks,
>> Dehao
>>
>>>
>>>>   LBB2
>>>>     {
>>>>        asm3
>>>>     }
>>>>   asm4
>>>>   ....
>>>>
>>>>>
>>>>> And even if, I don't see what's the _problem_ with too many block
>>>>> locations, except bloat.
>>>>
>>>> Yes, bloat is the major problem.
>>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>>>
>>>>>
>>>>> Ciao,
>>>>> Michael.
Dehao Chen - Oct. 29, 2012, 6:46 p.m.
Ok. I'll test Micheal's patch, and send out the new patch soon.

Thanks,
Dehao
Jakub Jelinek - Oct. 30, 2012, 9:52 a.m.
On Mon, Oct 29, 2012 at 05:10:10PM +0100, Richard Biener wrote:
> On Mon, Oct 29, 2012 at 4:25 PM, Dehao Chen <dehao@google.com> wrote:
> > On Mon, Oct 29, 2012 at 7:17 AM, Michael Matz <matz@suse.de> wrote:
> >> Hi,
> >>
> >> On Mon, 29 Oct 2012, Richard Biener wrote:
> >>
> >>> > Well, you merely moved the bogus code to gimple-low.c.  It is bogus
> >>> > because you unconditionally overwrite TREE_BLOCK of all operands (and all
> >
> > Emm, then in gimple-low.c, we should probably not unconditionally
> > overwrite gimple_block for stmts too?
> 
> gimple stmts have no block before gimple-low.

And tree expressions don't have TREE_BLOCK before gimple-low either.
So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple
stmts as well as all expression in the operands.  It is not overwriting
anything, no frontend sets TREE_BLOCK for any expression, the way frontends
associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND
after gimplification, and it is gimple-low responsibility to set it.

In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK
initially.  Before the location_t changes, again it was gimple-low that
was the first setter of TREE_BLOCK, which was valid for all
IS_EXPR_CODE_CLASS.  So, IMNSHO gimple-low should merge location_t
with block for all gimple stmts and all tree expressions used in its
operands.  It shouldn't be set on trees that can be shared, so
say decls etc. should keep using just location_t's without associated block.
So perhaps the right test for gimple-low setting of block is
CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp).

	Jakub
Dehao Chen - Oct. 30, 2012, 2:17 p.m.
> And tree expressions don't have TREE_BLOCK before gimple-low either.
> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple
> stmts as well as all expression in the operands.  It is not overwriting
> anything, no frontend sets TREE_BLOCK for any expression, the way frontends
> associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND
> after gimplification, and it is gimple-low responsibility to set it.
>
> In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK
> initially.  Before the location_t changes, again it was gimple-low that
> was the first setter of TREE_BLOCK, which was valid for all
> IS_EXPR_CODE_CLASS.  So, IMNSHO gimple-low should merge location_t
> with block for all gimple stmts and all tree expressions used in its
> operands.  It shouldn't be set on trees that can be shared, so
> say decls etc. should keep using just location_t's without associated block.
> So perhaps the right test for gimple-low setting of block is
> CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp).
>
>         Jakub

I Kind of like this idea. What do you guys think?

Thanks,
Dehao
Richard Guenther - Oct. 30, 2012, 2:38 p.m.
On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote:
>> And tree expressions don't have TREE_BLOCK before gimple-low either.
>> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple
>> stmts as well as all expression in the operands.  It is not overwriting
>> anything, no frontend sets TREE_BLOCK for any expression, the way frontends
>> associate IL with BLOCKs is by putting them inside of BIND_EXPR/GIMPLE_BIND
>> after gimplification, and it is gimple-low responsibility to set it.
>>
>> In 4.3 before tuples, it was solely gimple-low that set TREE_BLOCK
>> initially.  Before the location_t changes, again it was gimple-low that
>> was the first setter of TREE_BLOCK, which was valid for all
>> IS_EXPR_CODE_CLASS.  So, IMNSHO gimple-low should merge location_t
>> with block for all gimple stmts and all tree expressions used in its
>> operands.  It shouldn't be set on trees that can be shared, so
>> say decls etc. should keep using just location_t's without associated block.
>> So perhaps the right test for gimple-low setting of block is
>> CAN_HAVE_LOCATION_P (exp) && !tree_node_can_be_shared (exp).
>>
>>         Jakub
>
> I Kind of like this idea. What do you guys think?

I question the need of BLOCK info on expression trees.  If BLOCKs are
relevant then the tree ends up referencing a declaration with a BLOCK as
context, no?  Thus, the case

  int tem, a;
  {
    int a;
    ...
    tem = a;
  }
  int b = tem + 5;

where we may end up with gimple like

  b = a + 5;

thus mixing two BLOCKs inside a stmt (and no expression trees to
attach different BLOCKs) is no different from the case where we end
up with expression trees.

Thus my original question - why isn't a NULL BLOCK treated the same
as UNKNOWN_LOCATION?  Or rather, _why_ does Michas patch not work?
Did you analyze the guality fails?

Thanks,
Richard.

> Thanks,
> Dehao
Michael Matz - Oct. 30, 2012, 2:49 p.m.
Hi,

On Tue, 30 Oct 2012, Richard Biener wrote:

> On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote:
> >> And tree expressions don't have TREE_BLOCK before gimple-low either.
> >> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple
> >> stmts as well as all expression in the operands.

That would be a step away from the ideal situation, so we rather should 
first analyze why the testcase fails with my patch.  I expected some 
fallout and am actually surprised it's only one testcase :)

What we should end up with in the ideal world is that we simply have no 
expressions in gimple (and hence no place to store any locations for 
them), except via gimple statements.

> I question the need of BLOCK info on expression trees.  If BLOCKs are 
> relevant then the tree ends up referencing a declaration with a BLOCK as 
> context, no?  Thus, the case
> 
>   int tem, a;
>   {
>     int a;
>     ...
>     tem = a;
>   }
>   int b = tem + 5;
> 
> where we may end up with gimple like
> 
>   b = a + 5;
> 
> thus mixing two BLOCKs inside a stmt (and no expression trees to
> attach different BLOCKs) is no different from the case where we end
> up with expression trees.
> 
> Thus my original question - why isn't a NULL BLOCK treated the same
> as UNKNOWN_LOCATION?

Since merging location and block a null BLOCK doesn't imply no location.  
It can very well have a location without a block.  What we might want to 
imply is that a null BLOCK implies the BLOCK from the statement.  But as 
you say, first we should find out why my patch breaks the one testcase.

> Or rather, _why_ does Michas patch not work? Did you analyze the guality 
> fails?


Ciao,
Michael.
Richard Guenther - Oct. 30, 2012, 2:53 p.m.
On Tue, Oct 30, 2012 at 3:49 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 30 Oct 2012, Richard Biener wrote:
>
>> On Tue, Oct 30, 2012 at 3:17 PM, Dehao Chen <dehao@google.com> wrote:
>> >> And tree expressions don't have TREE_BLOCK before gimple-low either.
>> >> So IMNSHO it is gimple-low.c that should set TREE_BLOCK of all the gimple
>> >> stmts as well as all expression in the operands.
>
> That would be a step away from the ideal situation, so we rather should
> first analyze why the testcase fails with my patch.  I expected some
> fallout and am actually surprised it's only one testcase :)
>
> What we should end up with in the ideal world is that we simply have no
> expressions in gimple (and hence no place to store any locations for
> them), except via gimple statements.
>
>> I question the need of BLOCK info on expression trees.  If BLOCKs are
>> relevant then the tree ends up referencing a declaration with a BLOCK as
>> context, no?  Thus, the case
>>
>>   int tem, a;
>>   {
>>     int a;
>>     ...
>>     tem = a;
>>   }
>>   int b = tem + 5;
>>
>> where we may end up with gimple like
>>
>>   b = a + 5;
>>
>> thus mixing two BLOCKs inside a stmt (and no expression trees to
>> attach different BLOCKs) is no different from the case where we end
>> up with expression trees.
>>
>> Thus my original question - why isn't a NULL BLOCK treated the same
>> as UNKNOWN_LOCATION?
>
> Since merging location and block a null BLOCK doesn't imply no location.
> It can very well have a location without a block.  What we might want to
> imply is that a null BLOCK implies the BLOCK from the statement.  But as
> you say, first we should find out why my patch breaks the one testcase.

Yes, I mean we happily leave the stmt line location the same if we have
a stmt with UNKNOWN_LOCATION (thus it inherits that of the previous stmt),
we should do the same with BLOCKs - if a stmt has a location with NULL
BLOCK then it should inherit the block info from the previous stmt.

Richard.

>> Or rather, _why_ does Michas patch not work? Did you analyze the guality
>> fails?
>
>
> Ciao,
> Michael.
Jakub Jelinek - Oct. 30, 2012, 3:03 p.m.
On Tue, Oct 30, 2012 at 03:38:11PM +0100, Richard Biener wrote:
> I question the need of BLOCK info on expression trees.  If BLOCKs are
> relevant then the tree ends up referencing a declaration with a BLOCK as
> context, no?  Thus, the case
> 
>   int tem, a;
>   {
>     int a;
>     ...
>     tem = a;
>   }
>   int b = tem + 5;
> 
> where we may end up with gimple like
> 
>   b = a + 5;
> 
> thus mixing two BLOCKs inside a stmt (and no expression trees to
> attach different BLOCKs) is no different from the case where we end
> up with expression trees.

IMHO either we don't use locations at all on tree expressions (thus
no source location nor block), or both.  A source location has always
an associated block it is present in.  Of course for shared trees we
can't put there any block, as it can appear anywhere.

> Thus my original question - why isn't a NULL BLOCK treated the same
> as UNKNOWN_LOCATION?  Or rather, _why_ does Michas patch not work?
> Did you analyze the guality fails?

Micha's patch is degrading debug info quality.  Whenever some expression
has different source location from the source location of the gimple stmt,
then assuming that other source location is from the same block is wrong,
it could very well be from a different one.  On the testcase that fails
with Micha's patch, we have:
  [pr43479.c : 8:4] l_2 = l_1(D) + 1;
  [pr43479.c : 8:4] # DEBUG l => l_2
  [pr43479.c : 10:9] # DEBUG h => n_3(D)
  [pr43479.c : 12:11] # DEBUG i => k_4(D)
  [pr43479.c : 13:8] k_5 = k_4(D) + 1;
  [pr43479.c : 13:8] # DEBUG k => k_5
  [pr43479.c : 17:11] # DEBUG j => m_6(D)
  [pr43479.c : 18:8] m_7 = m_6(D) + 1;
  [pr43479.c : 18:8] # DEBUG m => m_7
  [pr43479.c : 22:3] __asm__ __volatile__("" :  : "r" k_5, "r" l_2);
  [pr43479.c : 23:3] __asm__ __volatile__("" :  : "r" m_7, "r" n_3(D));
where line 8 is from the outer block in the source, line 10 from the middle
block, line 12/13 from first innermost block, line 17/18 from second
innermost block.  But all of the l_2, k_5 and m_7 setters are TERed,
so everything is emitted when expanding the two asm
statements and with Micha's patch the block used is the block of the asm
statement, while previously each TERed statement got its own block.

I'd say either we should do the TREE_BLOCK setting on all non-shareable
trees during gimple-low and clear the block (but then likely whole
location?; it doesn't make sense to say in the debugger that something
has certain source location when you can't print variables declared in that
location) if copying expressions for use elsewhere, outside of the
containing function.  Or say during gimplification or gimple-low.c
simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION
to make it clear we don't use it (wonder if that could affect debug info
quality, perhaps not that much), but during expansion if creating trees
from TERed stmts they need to be set back, or the current location/block
adjusted accordingly.

	Jakub
Richard Guenther - Oct. 30, 2012, 3:15 p.m.
On Tue, Oct 30, 2012 at 4:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 30, 2012 at 03:38:11PM +0100, Richard Biener wrote:
>> I question the need of BLOCK info on expression trees.  If BLOCKs are
>> relevant then the tree ends up referencing a declaration with a BLOCK as
>> context, no?  Thus, the case
>>
>>   int tem, a;
>>   {
>>     int a;
>>     ...
>>     tem = a;
>>   }
>>   int b = tem + 5;
>>
>> where we may end up with gimple like
>>
>>   b = a + 5;
>>
>> thus mixing two BLOCKs inside a stmt (and no expression trees to
>> attach different BLOCKs) is no different from the case where we end
>> up with expression trees.
>
> IMHO either we don't use locations at all on tree expressions (thus
> no source location nor block), or both.  A source location has always
> an associated block it is present in.  Of course for shared trees we
> can't put there any block, as it can appear anywhere.
>
>> Thus my original question - why isn't a NULL BLOCK treated the same
>> as UNKNOWN_LOCATION?  Or rather, _why_ does Michas patch not work?
>> Did you analyze the guality fails?
>
> Micha's patch is degrading debug info quality.  Whenever some expression
> has different source location from the source location of the gimple stmt,
> then assuming that other source location is from the same block is wrong,
> it could very well be from a different one.  On the testcase that fails
> with Micha's patch, we have:
>   [pr43479.c : 8:4] l_2 = l_1(D) + 1;
>   [pr43479.c : 8:4] # DEBUG l => l_2
>   [pr43479.c : 10:9] # DEBUG h => n_3(D)
>   [pr43479.c : 12:11] # DEBUG i => k_4(D)
>   [pr43479.c : 13:8] k_5 = k_4(D) + 1;
>   [pr43479.c : 13:8] # DEBUG k => k_5
>   [pr43479.c : 17:11] # DEBUG j => m_6(D)
>   [pr43479.c : 18:8] m_7 = m_6(D) + 1;
>   [pr43479.c : 18:8] # DEBUG m => m_7
>   [pr43479.c : 22:3] __asm__ __volatile__("" :  : "r" k_5, "r" l_2);
>   [pr43479.c : 23:3] __asm__ __volatile__("" :  : "r" m_7, "r" n_3(D));
> where line 8 is from the outer block in the source, line 10 from the middle
> block, line 12/13 from first innermost block, line 17/18 from second
> innermost block.  But all of the l_2, k_5 and m_7 setters are TERed,
> so everything is emitted when expanding the two asm
> statements and with Micha's patch the block used is the block of the asm
> statement, while previously each TERed statement got its own block.
>
> I'd say either we should do the TREE_BLOCK setting on all non-shareable
> trees during gimple-low and clear the block (but then likely whole
> location?; it doesn't make sense to say in the debugger that something
> has certain source location when you can't print variables declared in that
> location) if copying expressions for use elsewhere, outside of the
> containing function.  Or say during gimplification or gimple-low.c
> simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION
> to make it clear we don't use it (wonder if that could affect debug info
> quality, perhaps not that much), but during expansion if creating trees
> from TERed stmts they need to be set back, or the current location/block
> adjusted accordingly.

So maybe TER (well, those looking up the stmt) should pick the location
from the TERed statement properly then?

Richard.

>         Jakub
Jakub Jelinek - Oct. 30, 2012, 3:21 p.m.
On Tue, Oct 30, 2012 at 04:15:38PM +0100, Richard Biener wrote:
> So maybe TER (well, those looking up the stmt) should pick the location
> from the TERed statement properly then?

Perhaps, but Micha's patch doesn't do that.
But in that case IMHO it still would help to set all expr locations to
UNKNOWN_LOCATION during gimple lowering, to make it clear we ignore
the locations.

	Jakub
Richard Guenther - Oct. 30, 2012, 3:29 p.m.
On Tue, Oct 30, 2012 at 4:21 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 30, 2012 at 04:15:38PM +0100, Richard Biener wrote:
>> So maybe TER (well, those looking up the stmt) should pick the location
>> from the TERed statement properly then?
>
> Perhaps, but Micha's patch doesn't do that.
> But in that case IMHO it still would help to set all expr locations to
> UNKNOWN_LOCATION during gimple lowering, to make it clear we ignore
> the locations.

Yes indeed.

Richard.

>         Jakub
Eric Botcazou - Oct. 30, 2012, 3:53 p.m.
> I'd say either we should do the TREE_BLOCK setting on all non-shareable
> trees during gimple-low and clear the block (but then likely whole
> location?; it doesn't make sense to say in the debugger that something
> has certain source location when you can't print variables declared in that
> location) if copying expressions for use elsewhere, outside of the
> containing function.  Or say during gimplification or gimple-low.c
> simply set t->exp.locus of all non-shareable expressions to UNKNOWN_LOCATION
> to make it clear we don't use it (wonder if that could affect debug info
> quality, perhaps not that much), but during expansion if creating trees
> from TERed stmts they need to be set back, or the current location/block
> adjusted accordingly.

The debugger isn't the only consumer of debug info, and other tools might need 
a finer granularity than a GIMPLE location, so clearing EXPR_LOCATION to work 
around a debug info size issue seems very short-sighted (to say the least).
Dehao Chen - Oct. 30, 2012, 4:34 p.m.
> The debugger isn't the only consumer of debug info, and other tools might need
> a finer granularity than a GIMPLE location, so clearing EXPR_LOCATION to work
> around a debug info size issue seems very short-sighted (to say the least).

Hi, Eric,

There might be some misunderstanding here. Clearing EXPR_LOCATION is
not a work around to reduce debug size. It is aiming at making gcc
implementation cleaner. And before we resetting it, we need to truely
make sure nothing after gimple-low is dependent on it. Please let me
know if you have other concerns.

Thanks,
Dehao

>
> --
> Eric Botcazou

Patch

Index: gcc/tree-eh.c
===================================================================
--- gcc/tree-eh.c	(revision 192809)
+++ gcc/tree-eh.c	(working copy)
@@ -739,6 +739,7 @@  do_return_redirection (struct goto_queue_node *q,
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -758,6 +759,7 @@  do_goto_redirection (struct goto_queue_node *q, tr
     gimple_seq_add_seq (&q->repl_stmt, mod);
 
   x = gimple_build_goto (finlab);
+  gimple_set_location (x, q->location);
   gimple_seq_add_stmt (&q->repl_stmt, x);
 }
 
@@ -857,6 +859,7 @@  frob_into_branch_around (gimple tp, eh_region regi
       if (!over)
 	over = create_artificial_label (loc);
       x = gimple_build_goto (over);
+      gimple_set_location (x, loc);
       gimple_seq_add_stmt (&cleanup, x);
     }
   gimple_seq_add_seq (&eh_seq, cleanup);
@@ -1085,6 +1088,7 @@  lower_try_finally_nofallthru (struct leh_state *st
 	  emit_post_landing_pad (&eh_seq, tf->region);
 
 	  x = gimple_build_goto (lab);
+	  gimple_set_location (x, gimple_location (tf->try_finally_expr));
 	  gimple_seq_add_stmt (&eh_seq, x);
 	}
     }
@@ -1223,6 +1227,7 @@  lower_try_finally_copy (struct leh_state *state, s
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&new_stmt, x);
     }
 
@@ -1395,6 +1400,7 @@  lower_try_finally_switch (struct leh_state *state,
 
       tmp = lower_try_finally_fallthru_label (tf);
       x = gimple_build_goto (tmp);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&switch_body, x);
     }
 
@@ -1423,6 +1429,7 @@  lower_try_finally_switch (struct leh_state *state,
       gimple_seq_add_stmt (&eh_seq, x);
 
       x = gimple_build_goto (finally_label);
+      gimple_set_location (x, tf_loc);
       gimple_seq_add_stmt (&eh_seq, x);
 
       tmp = build_int_cst (integer_type_node, eh_index);
Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c	(revision 192809)
+++ gcc/gimple-low.c	(working copy)
@@ -331,7 +331,18 @@  lower_omp_directive (gimple_stmt_iterator *gsi, st
   gsi_next (gsi);
 }
 
+/* Call back function to set the block for expr.  */
 
+static tree
+tree_set_block_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED,
+                     void *data)
+{
+  tree block = (tree) data;
+  if (CAN_HAVE_LOCATION_P (*tp))
+    TREE_SET_BLOCK (*tp, block);
+  return NULL_TREE;
+}
+
 /* Lower statement GSI.  DATA is passed through the recursion.  We try to
    track the fallthruness of statements and get rid of unreachable return
    statements in order to prevent the EH lowering pass from adding useless
@@ -343,8 +354,11 @@  static void
 lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
 {
   gimple stmt = gsi_stmt (*gsi);
+  unsigned i;
 
   gimple_set_block (stmt, data->block);
+  for (i = 0; i < gimple_num_ops (stmt); i++)
+    walk_tree (gimple_op_ptr (stmt, i), tree_set_block_r, data->block, NULL);
 
   switch (gimple_code (stmt))
     {
Index: gcc/testsuite/g++.dg/debug/dwarf2/block.C
===================================================================
--- gcc/testsuite/g++.dg/debug/dwarf2/block.C	(revision 0)
+++ gcc/testsuite/g++.dg/debug/dwarf2/block.C	(revision 0)
@@ -0,0 +1,29 @@ 
+// Compiler should not generate too many lexical blocks for this function.
+// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
+// { dg-options "-O0 -fno-exceptions -g -dA" }
+
+union UElement {
+    void* pointer;
+    int integer;
+};
+struct UColToken {
+  unsigned source;
+  unsigned char **rulesToParseHdl;
+};
+
+int uhash_hashTokens(const union UElement k)
+{
+  int hash = 0;
+  struct UColToken *key = (struct UColToken *)k.pointer;
+  if (key != 0) {
+    int len = (key->source & 0xFF000000)>>24;
+    int inc = ((len - 32) / 32) + 1;
+    const unsigned char *p = (key->source & 0x00FFFFFF)
+			     + *(key->rulesToParseHdl);
+    const unsigned char *limit = p + len;
+    hash = *p + *limit;
+  }
+  return hash;
+}
+
+// { dg-final { scan-assembler-not "LBB10" } }