This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix debug info for expr and jump stmt
Hi,
On Mon, 29 Oct 2012, Dehao Chen 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?
That's okay, because the gimple statements were just generated during
gimplification.
> >> 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.
Yes, sorry, you're right. It's the lowering of bind expressions that
tacks blocks to statements.
> 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.
The point is, there shouldn't be any expressions left after gimplification
that would receive locations/blocks. Every side-effect of computing an
expression should be a separate gimple statement. The only exception
should be loads for arguments of call and asm statements statements and
there setting the location of those expressions to the one of the call
statement itself is superfluous.
> >> 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.
No, we should not. Basically the thing to keep in mind is that the
location/block of expressions after gimplification is supposed to be
meaningless. The gimple statements are the anchors for locations.
Everything should be made to work in this model, not by tacking locations
to expression. But see below...
> >> 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
> ....
Hmm, I asked about specifics for this testcase. Let me find out myself...
You're talking about this situation:
.LBB4:
....
# code for line 20
....A
.LBE4:
.loc 1 22 0
movq -16(%rbp), %rax # key, tmp82
.LBB5:
....
# code for line 22/23
....
.LBE5:
movq -16(%rbp), %rax # key, tmp83
.LBB6:
....
.LBE6
where the reload of 'key' is splitting the lexical block? This comes from
_10 = key_3->rulesToParseHdl;
Where the MEM_REF of *key_3 does have a location (hence locus), but no
block associated:
(gdb) p expand_location(((tree)0x7ffff62252d0)->exp.locus)
$12 = {file = 0x7fffffffe38e "blabla.C", line = 22, column = 29, data =
0x0, sysp = false}
whereas the statement has the same location, but is associated with a
block:
(gdb) p expand_location(curr_location )
$14 = {file = 0x7fffffffe38e "blabla.C", line = 22, column = 29,
data = 0x7ffff60cf410, sysp = false}
This inconsistency is indeed generated by lower_stmt, which inserts
the block into this locator for the statements, but not for operands.
The issue is that tree->RTL expand listens to what's stored in the
EXPR_LOCATION, instead of taking the location info from curr_location
(which itself comes from the currently expanded gimple statement).
The patch/hack below solves this, and probably also your problem. It
might introduce others, but I think we should work towards making this
patch possible. I.e. follow the above layed out model, that after
gimplification locators are _only_ in gimple statements, nowhere else
(though I'm not sure what to do about constructs that don't generate
statements, like simple global data constructors).
I'm not sure how realistic reaching this goal for 4.8 is. If it isn't
then I fear something along the lines of your hack is necessary.
Ciao,
Michael.
---------------------
Index: expr.c
===================================================================
--- expr.c (revision 192809)
+++ expr.c (working copy)
@@ -5030,7 +5030,7 @@ store_expr (tree exp, rtx target, int ca
{
rtx temp;
rtx alt_rtl = NULL_RTX;
- location_t loc = EXPR_LOCATION (exp);
+ location_t loc = curr_insn_location ();
if (VOID_TYPE_P (TREE_TYPE (exp)))
{
@@ -7881,7 +7881,7 @@ expand_expr_real (tree exp, rtx target,
{
location_t saved_location = input_location;
location_t saved_curr_loc = curr_insn_location ();
- input_location = EXPR_LOCATION (exp);
+ input_location = curr_insn_location ();
set_curr_insn_location (input_location);
ret = expand_expr_real_1 (exp, target, tmode, modifier, alt_rtl);
@@ -9120,7 +9120,7 @@ expand_expr_real_1 (tree exp, rtx target
int ignore;
tree context;
bool reduce_bit_field;
- location_t loc = EXPR_LOCATION (exp);
+ location_t loc = curr_insn_location ();
struct separate_ops ops;
tree treeop0, treeop1, treeop2;
tree ssa_name = NULL_TREE;
@@ -9481,7 +9481,7 @@ expand_expr_real_1 (tree exp, rtx target
with non-BLKmode values. */
gcc_assert (GET_MODE (ret) != BLKmode);
- val = build_decl (EXPR_LOCATION (exp),
+ val = build_decl (curr_insn_location (),
VAR_DECL, NULL, TREE_TYPE (exp));
DECL_ARTIFICIAL (val) = 1;
DECL_IGNORED_P (val) = 1;