[Bug tree-optimization/56094] New: Invalid line number info generated with tree-level ivopts

[Bug tree-optimization/56094] New: Invalid line number info generated with tree-level ivopts

[josh.m.conner at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

             Bug #: 56094
           Summary: Invalid line number info generated with tree-level
                    ivopts
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: josh.m.conner@gmail.com


The attached code has a number of instructions that are associated with the
head of the function.  This was showing up when setting a breakpoint on the
function itself and gdb was setting several - not a problem in general, except
that the statements were more appropriately correlated with intra-loop
calculations instead of anything to do with the prologue.

To reproduce, compile the attached file with -g -O2, and notice the statements
associated with line 83.

These lines are in fact statements that are generated during tree-level
induction variable optimization, but which aren't getting their location data
copied over from the gimple statement and so they default to the start of the
function (sorry for being a bit vague, but it's been a while since I looked
into the mechanics of this and I don't recall the details).

The fix I have implemented in our local tree is in rewrite_use_nonlinear_expr,
where after generating the computation (comp) I verify that it has a location
associated with it - if it doesn't but the use stmt (use->stmt) does have a
location, I copy the location from the use->stmt over to comp.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[josh.m.conner at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #1 from Joshua Conner <josh.m.conner at gmail dot com> 2013-01-24 04:03:44 UTC ---
Created attachment 29263
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29263
Reduced test case

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[josh.m.conner at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #2 from Joshua Conner <josh.m.conner at gmail dot com> 2013-01-24 04:05:09 UTC ---
Sorry, I should have been more specific -- the function I'm describing in the
previous comments is "test_main".

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-24 12:17:14 UTC ---
Statements with no location inherit that of the surrounding statements.  The
issue is that the rewritten statements usually do not exist in literal form
in the original program.  The final assignment of the result of course
should retain use->stmts location.

Now what can happen is that the stmts without location are scheduled into
the prologue ... not sure how / if we want to avoid this.  Jakub?

Note I didn't look at the testcase at all.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-24 14:19:17 UTC ---
On a brief look, this doesn't look like using location of neighbouring
statement, given:
grep 66:1 pr56094.c.115t.cunroll | wc -l
0
grep 66:1 pr56094.c.119t.ivopts | wc -l
39

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-24 15:06:28 UTC ---
So, the reason seems to be:
      mod = build2 (INIT_EXPR, TREE_TYPE (t), t, unshare_expr (val));

      SET_EXPR_LOCATION (mod, EXPR_LOC_OR_HERE (val));
in:
#0  internal_get_tmp_var (val=0x7ffff1aeaaa0, pre_p=0x7fffffffdda8, post_p=0x0,
is_formal=true) at ../../gcc/gimplify.c:636
#1  0x0000000000832a3e in get_formal_tmp_var (val=0x7ffff1aeaaa0,
pre_p=0x7fffffffdda8) at ../../gcc/gimplify.c:657
#2  0x000000000084d3b7 in gimplify_expr (expr_p=0x7fffffffdcf8,
pre_p=0x7fffffffdda8, post_p=0x7fffffffdbb0, 
    gimple_test_f=0x80befc <is_gimple_val(tree_node*)>, fallback=1) at
../../gcc/gimplify.c:8023
#3  0x000000000084f7c8 in force_gimple_operand_1 (expr=0x7ffff1aeaaa0,
stmts=0x7fffffffdda8, gimple_test_f=0x80befc <is_gimple_val(tree_node*)>, 
    var=0x0) at ../../gcc/gimplify.c:8633
#4  0x000000000084f878 in force_gimple_operand_gsi_1 (gsi=0x7fffffffde60,
expr=0x7ffff1aeaaa0, 
    gimple_test_f=0x80befc <is_gimple_val(tree_node*)>, var=0x0, before=true,
m=GSI_SAME_STMT) at ../../gcc/gimplify.c:8669
#5  0x000000000084f923 in force_gimple_operand_gsi (gsi=0x7fffffffde60,
expr=0x7ffff1aeaaa0, simple_p=true, var=0x0, before=true, m=GSI_SAME_STMT)
    at ../../gcc/gimplify.c:8698
#6  0x0000000000b79414 in rewrite_use_nonlinear_expr (data=0x7fffffffdf70,
use=0x1887f90, cand=0x1887f40) at ../../gcc/tree-ssa-loop-ivopts.c:6151
#7  0x0000000000b79df5 in rewrite_use (data=0x7fffffffdf70, use=0x1887f90,
cand=0x1887f40) at ../../gcc/tree-ssa-loop-ivopts.c:6358
#8  0x0000000000b79eb7 in rewrite_uses (data=0x7fffffffdf70) at
../../gcc/tree-ssa-loop-ivopts.c:6391
#9  0x0000000000b7b0a0 in tree_ssa_iv_optimize_loop (data=0x7fffffffdf70,
loop=0x7ffff198bb28) at ../../gcc/tree-ssa-loop-ivopts.c:6716

During original gimplification, I can understand the OR_HERE (aka
input_location) part there, or in passes that maintain input_location.
But generally force_gimple_operand* is often called even from passes that don't
maintain reasonable input_location.  So, either the above should be hack like
      if (gimplify_ctxp->into_ssa)
        SET_EXPR_LOCATION (mod, EXPR_LOCATION (val));
      else
        SET_EXPR_LOCATION (mod, EXPR_LOC_OR_HERE (val));
(or gimplify_ctxp->use_input_location or whatever), or perhaps
force_gimple_operand* should temporarily set input_location to UNKNOWN_LOCATION
and restore it back from a saved copy before returning.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-24 17:26:48 UTC ---
--- gimplify.c.jj    2013-01-11 09:02:55.000000000 +0100
+++ gimplify.c    2013-01-24 18:15:54.246157569 +0100
@@ -8600,6 +8600,7 @@ force_gimple_operand_1 (tree expr, gimpl
 {
   enum gimplify_status ret;
   struct gimplify_ctx gctx;
+  location_t saved_location;

   *stmts = NULL;

@@ -8613,6 +8614,8 @@ force_gimple_operand_1 (tree expr, gimpl
   push_gimplify_context (&gctx);
   gimplify_ctxp->into_ssa = gimple_in_ssa_p (cfun);
   gimplify_ctxp->allow_rhs_cond_expr = true;
+  saved_location = input_location;
+  input_location = UNKNOWN_LOCATION;

   if (var)
     {
@@ -8634,6 +8637,7 @@ force_gimple_operand_1 (tree expr, gimpl
       gcc_assert (ret != GS_ERROR);
     }

+  input_location = saved_location;
   pop_gimplify_context (NULL);

   return expr;

seems to work (there are way too many uses of input_location in gimplify.c),
alternatively we could e.g. grab input_location temporarily just in
force_gimple_operand_gsi_1, depending on gimple_location of the stmt this
should be emitted before resp. after.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #7 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-01-24 18:29:44 UTC ---
Don't we have a EXPR_LOC_OR_UNKNOWN? That seems far nicer than overriding a
global variable and restoring it back afterwards without known what else may be
touching it or relying on it.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> 2013-01-24 18:37:30 UTC ---
"jakub at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:

>
>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094
>
>--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-24
>17:26:48 UTC ---
>--- gimplify.c.jj    2013-01-11 09:02:55.000000000 +0100
>+++ gimplify.c    2013-01-24 18:15:54.246157569 +0100
>@@ -8600,6 +8600,7 @@ force_gimple_operand_1 (tree expr, gimpl
> {
>   enum gimplify_status ret;
>   struct gimplify_ctx gctx;
>+  location_t saved_location;
>
>   *stmts = NULL;
>
>@@ -8613,6 +8614,8 @@ force_gimple_operand_1 (tree expr, gimpl
>   push_gimplify_context (&gctx);
>   gimplify_ctxp->into_ssa = gimple_in_ssa_p (cfun);
>   gimplify_ctxp->allow_rhs_cond_expr = true;
>+  saved_location = input_location;
>+  input_location = UNKNOWN_LOCATION;
>
>   if (var)
>     {
>@@ -8634,6 +8637,7 @@ force_gimple_operand_1 (tree expr, gimpl
>       gcc_assert (ret != GS_ERROR);
>     }
>
>+  input_location = saved_location;
>   pop_gimplify_context (NULL);
>
>   return expr;
>
>seems to work (there are way too many uses of input_location in
>gimplify.c),

Eh, I thought we arrived at this somewhere
Earlier this year...  Ah, I suggested input_location should be unknown_location
Anyway here and we should arrange so and
Assert in force_simple_operand.

>alternatively we could e.g. grab input_location temporarily just in
>force_gimple_operand_gsi_1, depending on gimple_location of the stmt
>this
>should be emitted before resp. after.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #9 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-01-24 18:39:18 UTC ---
> During original gimplification, I can understand the OR_HERE (aka
> input_location) part there, or in passes that maintain input_location.

I thought gimplification happens after each whole function is read, how is
input_location correct in that case? Moreover, how do optimization passes try
to mantain input_location accurate?

> force_gimple_operand* should temporarily set input_location to UNKNOWN_LOCATION
> and restore it back from a saved copy before returning.

My fear is that this is basically delaying the inevitable, which is getting rid
of input_location. At some moment, the saving/restoring of input_location in
random points will start conflicting in unexpected ways and the whole thing
will be unfixable without breaking something else.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> 2013-01-24 19:30:54 UTC ---
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote:

>
>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094
>
>--- Comment #9 from Manuel López-Ibáñez <manu at gcc dot gnu.org>
>2013-01-24 18:39:18 UTC ---
>> During original gimplification, I can understand the OR_HERE (aka
>> input_location) part there, or in passes that maintain
>input_location.
>
>I thought gimplification happens after each whole function is read, how
>is
>input_location correct in that case? Moreover, how do optimization
>passes try
>to mantain input_location accurate?

Input_location should only be used from parsing.  Other places reuse the
variable and those happen to eventually pick up stale values, such as
gimplification.  We should
Arrange for different globals to be used there and privatize input_location. In
the meantime assert that input_location is unknown when
Entering or leaving passes.

>
>> force_gimple_operand* should temporarily set input_location to
>UNKNOWN_LOCATION
>> and restore it back from a saved copy before returning.
>
>My fear is that this is basically delaying the inevitable, which is
>getting rid
>of input_location. At some moment, the saving/restoring of
>input_location in
>random points will start conflicting in unexpected ways and the whole
>thing
>will be unfixable without breaking something else.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #11 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-01-24 20:49:33 UTC ---
(In reply to comment #10)
> 
> Input_location should only be used from parsing.  Other places reuse the
> variable and those happen to eventually pick up stale values, such as
> gimplification.  We should
> Arrange for different globals to be used there and privatize input_location. In
> the meantime assert that input_location is unknown when
> Entering or leaving passes.
> 

Perhaps I didn't make myself clear. What is calling internal_get_tmp_var that
is relying on input_location to be pointing to a sensible location? Is it
really called during parsing?

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-25 11:29:13 UTC ---
Created attachment 29272
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29272
gcc48-pr56094.patch

input_location is used heavily in the gimplifier, gimplify_expr sets it from
the expression being currently gimplified (if it has any), and for temporaries
and their initializers that are created while gimplifying that stmt it is
intentional to use the location of that expression.

I've bootstrapped/regtested this patch on i686-linux (no ada) so far, the lex.c
hunk is to avoid ICEs when e.g. tree-parloops.c calls the make_type langhook
(but there are other callers of that) with input_location of UNKNOWN_LOCATION.
I was surprised there weren't regressions, given that input_location is used
implicitly e.g. by all error/warning calls, unless they supply their own
location.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #13 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2013-01-25 11:43:37 UTC ---
(In reply to comment #12)
> Created attachment 29272 [details]
> gcc48-pr56094.patch
> 
> input_location is used heavily in the gimplifier, gimplify_expr sets it from
> the expression being currently gimplified (if it has any), and for temporaries
> and their initializers that are created while gimplifying that stmt it is
> intentional to use the location of that expression.

You are explaining how it works right now. What I am asking is how we want it
to work, that is, why the gimplifier needs to care about input_location and
cannot use *always* the location of the expression being gimplified (or some
sub-expression) or UNKNOWN_LOCATION for things that are compiler generated.

Moreover, if gimplifying occurs after parsing is completed, why do we need to
use input_location as a communication device between parts of the gimplifier,
why not just use some gimplifier-specific state or pass down specific
locations.

> I've bootstrapped/regtested this patch on i686-linux (no ada) so far, the lex.c
> hunk is to avoid ICEs when e.g. tree-parloops.c calls the make_type langhook
> (but there are other callers of that) with input_location of UNKNOWN_LOCATION.
> I was surprised there weren't regressions, given that input_location is used
> implicitly e.g. by all error/warning calls, unless they supply their own
> location.

Does the diagnostic machinery assert that input_location != UNKNOWN_LOCATION? I
think not. I seem to remember that we sometimes generate:

warning:0:0: something

but I am not sure if this happens for warning_at(UNKNOWN_LOCATION).

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-25 12:04:59 UTC ---
(In reply to comment #13)

> You are explaining how it works right now. What I am asking is how we want it
> to work, that is, why the gimplifier needs to care about input_location and
> cannot use *always* the location of the expression being gimplified (or some
> sub-expression) or UNKNOWN_LOCATION for things that are compiler generated.

Because not all the gimplifier routines see the current expression as whole,
they can work on some subexpression etc., and not all expressions have
location.  So, you generally want to use a location of some expression (if
known) for all the subexpressions that don't have their own location.
Currently that is performed in the gimplifier by saving the current location
in input_location variable and then lots of code use that.  Alternative to that
is use some different variable, gimplifier_location or whatever.  But you'd
really need to change lots of code for that, and I'd be afraid that some code
that needs that could be shared between FEs and the gimplifier.  E.g. make_node
uses input_location, which is desirable in the FEs, but with such a change
would be undesirable in the gimplifier.

> Does the diagnostic machinery assert that input_location != UNKNOWN_LOCATION? I
> think not. I seem to remember that we sometimes generate:
> 
> warning:0:0: something
> 
> but I am not sure if this happens for warning_at(UNKNOWN_LOCATION).

E.g. for warning_at, instead of printing say:
qqq.c: In function ‘foo’:
qqq.c:4:34: warning: argument to ‘sizeof’ in ‘__builtin_memset’ call is the
same expression as the destination; did you mean to provide an explicit length?
[-Wsizeof-pointer-memaccess]
if the first argument is changed to UNKNOWN_LOCATION, it prints:
qqq.c: In function ‘foo’:
cc1: warning: argument to ‘sizeof’ in ‘__builtin_memset’ call is the same
expression as the destination; did you mean to provide an explicit length?
[-Wsizeof-pointer-memaccess]

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> 2013-01-28 08:55:55 UTC ---
(In reply to comment #12)
> Created attachment 29272 [details]
> gcc48-pr56094.patch
> 
> input_location is used heavily in the gimplifier, gimplify_expr sets it from
> the expression being currently gimplified (if it has any), and for temporaries
> and their initializers that are created while gimplifying that stmt it is
> intentional to use the location of that expression.
> 
> I've bootstrapped/regtested this patch on i686-linux (no ada) so far, the lex.c
> hunk is to avoid ICEs when e.g. tree-parloops.c calls the make_type langhook
> (but there are other callers of that) with input_location of UNKNOWN_LOCATION.
> I was surprised there weren't regressions, given that input_location is used
> implicitly e.g. by all error/warning calls, unless they supply their own
> location.

This patch looks like a good way forward to me (moving the input_location
logic to the single caller of expand_all_functions () would be even better
of course, at best up to finalize_compilation_unit - which also should not
need to save the old location but simply arrange it to be UNKNOWN_LOCATION.
That we need input_location during RTL expansion (and RTL opts even?) is
of course bad enough :/)

I'm not sure we want this kind of patch for stage4 though, eventually we
should simply go with setting input_location to UNKNOWN inside
force_gimple_operand and friends.

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-28 14:05:48 UTC ---
Author: jakub
Date: Mon Jan 28 14:05:40 2013
New Revision: 195504

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195504
Log:
    PR tree-optimization/56094
    * gimplify.c (force_gimple_operand_1): Temporarily set input_location
    to UNKNOWN_LOCATION while gimplifying expr.

    * gcc.dg/pr56094.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr56094.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog

[Bug tree-optimization/56094] Invalid line number info generated with tree-level ivopts

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56094

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-28 14:33:18 UTC ---
Should be fixed now on the trunk, but keeping the PR open, so that we don't
forget to revert and do a better fix instead.