Patchwork [RFC] Fix for PR58201

login
register
mail settings
Submitter Jan Hubicka
Date Sept. 7, 2013, 8:28 a.m.
Message ID <20130907082846.GC4841@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/273376/
State New
Headers show

Comments

Jan Hubicka - Sept. 7, 2013, 8:28 a.m.
> > >+ 2013-09-04  Jan Hubicka  <jh@suse.cz>
> > >+
> > >+ 	PR middle-end/58201
> > >+ 	* cgraphunit.c (analyze_functions): Clear AUX fields
> > >+ 	after processing; initialize assembler name has.
> > >+
> > I checked and double checked and with this commit a C++ test regressed:
> > 
> > FAIL: g++.dg/template/cond2.C -std=gnu++98 (test for warnings, line 9)
> > FAIL: g++.dg/template/cond2.C -std=gnu++11 (test for warnings, line 9)
> > 
> > In practice we emit a message off by one line, line 10 instead of
> > the correct line 9. Is it possible that you are clearing too much,
> > so to speak?
> 
> Amazing, this testcase was triggering an ICE and I had to disable initialization
> of assembler name hash when error arrived.  That fixed the problem, but I did not
> notice it is still having wrong line info.
> 
> Adding an alias into the unit will trigger wrong line info before my change, since
> assembler name hash is populated too.
> 
> The error is output from DECL_ASSEMBLER_NAME hook that by itself is strange.
> 
> #0  0x0000000001041490 in error(char const*, ...) ()
> #1  0x0000000000735039 in write_expression(tree_node*) () at ../../gcc/cp/mangle.c:3008
> #2  0x000000000073a70f in write_template_arg(tree_node*) () at ../../gcc/cp/mangle.c:3179
> #3  0x000000000073ae69 in write_template_args(tree_node*) () at ../../gcc/cp/mangle.c:2551
> #4  0x00000000007332a4 in write_name(tree_node*, int) () at ../../gcc/cp/mangle.c:821
> #5  0x000000000073700c in write_type(tree_node*) () at ../../gcc/cp/mangle.c:2522
> #6  0x0000000000737279 in write_type(tree_node*) () at ../../gcc/cp/mangle.c:2017
> #7  0x0000000000738a18 in write_method_parms(tree_node*, int, tree_node*) () at ../../gcc/cp/mangle.c:2509
> #8  0x0000000000738e2f in write_bare_function_type(tree_node*, int, tree_node*) () at ../../gcc/cp/mangle.c:2451
> #9  0x0000000000733b9e in write_mangled_name(tree_node*, bool) () at ../../gcc/cp/mangle.c:689
> #10 0x000000000073c5b6 in mangle_decl_string(tree_node*) () at ../../gcc/cp/mangle.c:3446
> #11 0x000000000073c7e9 in mangle_decl(tree_node*) () at ../../gcc/cp/mangle.c:3468
> #12 0x0000000000d1df01 in decl_assembler_name(tree_node*) () at ../../gcc/tree.c:546
> #13 0x000000000083b1f5 in insert_to_assembler_name_hash(symtab_node_def*, bool) ()
> #14 0x000000000083b352 in symtab_initialize_asm_name_hash() [clone .part.3] ()

In the tree prior my change:
jh@gcc10:~/trunk/build4/gcc$ ./xgcc -B ./ -O2 ../../gcc/testsuite/g++.dg/template/cond2.C 
../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int test(c<(X ?  :  Y)>&) [with int X = 0; int Y = 2]':
../../gcc/testsuite/g++.dg/template/cond2.C:9:17:   required from here
../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle operand to '?:' operand cannot be mangled
 template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" }
                            ^
jh@gcc10:~/trunk/build4/gcc$ ./xgcc -B ./ -O2 ../../gcc/testsuite/g++.dg/template/cond2.C  -flto
../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int test(c<(X ?  :  Y)>&) [with int X = 0; int Y = 2]':
../../gcc/testsuite/g++.dg/template/cond2.C:6:28:   required from here
../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle operand to '?:' operand cannot be mangled
 template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" }

So it is just an accident that the line info is output sanely (if line 9 is
sane, I don't exactly know)

What I can think of is to hide the stale source location when it no longer have
defined meaning:

but of course this just leads to:
jh@gcc10:~/trunk/build7/gcc$ ./xgcc -B ./ -O2 ../../gcc/testsuite/g++.dg/template/cond2.C
../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int test(c<(X ?  :  Y)>&) [with int X = 0; int Y = 2]':
:0:0:   required from here
../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle operand to '?:' operand cannot be mangled
 template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" }

So i think it is up to C++ FE to correctly set and restore location info if it
wants to have error output correctly.  DECL_SOURCE_LOCATION of the decl being
mangled is 6 that leads to useless mesage:

jh@gcc10:~/trunk/build7/gcc$ ./xgcc -B ./ -O2 ../../gcc/testsuite/g++.dg/template/cond2.C
../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int test(c<(X ?  :  Y)>&) [with int X = 0; int Y = 2]':
../../gcc/testsuite/g++.dg/template/cond2.C:6:28:   required from here
../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle operand to '?:' operand cannot be mangled
 template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" }

Honza
Paolo Carlini - Sept. 7, 2013, 9 a.m.
Hi Honza,

and thanks for the analysis, now I understand the issue a little more.

On 09/07/2013 10:28 AM, Jan Hubicka wrote:
> So it is just an accident that the line info is output sanely (if line 
> 9 is sane, I don't exactly know)
I would say that in general it's definitely sane, because that is the 
instantiation point. And 10 is wrong, too late, it points to the closing 
bracket.

However, I notice that clang doesn't even try to output a message having 
to do with line 9: if I understand correctly, if that operator cannot be 
mangled, that happens unconditionally, isn't something that may succeed. 
Thus I wonder if, instead of outputting garbage line numbers we could at 
least suppress in such cases the whole "In instantiation of..." part of 
the diagnostic, it would be better than garbage. Do we have a mechanism 
for that?

Now I also understand that this should be a very uncommon error message, 
but, I'm wondering, is it possible that other errors, for other issues, 
are also affected? That is, other diagnostic happening very late and 
sensitive to the recent rework?

Paolo.
Jan Hubicka - Sept. 7, 2013, 9:09 a.m.
> Hi Honza,
> 
> and thanks for the analysis, now I understand the issue a little more.
> 
> On 09/07/2013 10:28 AM, Jan Hubicka wrote:
> >So it is just an accident that the line info is output sanely (if
> >line 9 is sane, I don't exactly know)
> I would say that in general it's definitely sane, because that is
> the instantiation point. And 10 is wrong, too late, it points to the
> closing bracket.
> 
> However, I notice that clang doesn't even try to output a message
> having to do with line 9: if I understand correctly, if that
> operator cannot be mangled, that happens unconditionally, isn't
> something that may succeed. Thus I wonder if, instead of outputting
> garbage line numbers we could at least suppress in such cases the
> whole "In instantiation of..." part of the diagnostic, it would be
> better than garbage. Do we have a mechanism for that?

It seems to be what older GCCs are doing, too.
> 
> Now I also understand that this should be a very uncommon error
> message, but, I'm wondering, is it possible that other errors, for
> other issues, are also affected? That is, other diagnostic happening
> very late and sensitive to the recent rework?

Any use of source_location from DECL_ASSEMBLER_NAME hanghook is wrong, since
source_location may point anywhere when the lazy mangling is triggered.  It
surprised me that DECL_ASSEMBLER_NAME triggers error/warning messages.  It is
up to backend when on at what symbols it will call it, so it is definitely
source of inconsistency.  So from design point of view I would preffer C++
FE to output these errors somewhere else.  But practicaly we can perhaps just
modify the message to not expect thelocation?

I am just leaving for flight to Pisa, will be back online at the hotel.

Honza
Paolo Carlini - Sept. 7, 2013, 10:13 a.m.
Hi

>What I can think of is to hide the stale source location when it no
>longer have
>defined meaning:
>Index: cgraphunit.c
>===================================================================
>--- cgraphunit.c        (revision 202352)
>+++ cgraphunit.c        (working copy)
>@@ -913,6 +913,7 @@ analyze_functions (void)
>
>   bitmap_obstack_initialize (NULL);
>   cgraph_state = CGRAPH_STATE_CONSTRUCTION;
>+  input_location = UNKNOWN_LOCATION;
>
>/* Ugly, but the fixup can not happen at a time same body alias is
>created;
>      C++ FE is confused about the COMDAT groups being right.  */
>
>but of course this just leads to:
>jh@gcc10:~/trunk/build7/gcc$ ./xgcc -B ./ -O2
>../../gcc/testsuite/g++.dg/template/cond2.C
>../../gcc/testsuite/g++.dg/template/cond2.C: In instantiation of 'int
>test(c<(X ?  :  Y)>&) [with int X = 0; int Y = 2]':
>:0:0:   required from here
>../../gcc/testsuite/g++.dg/template/cond2.C:6:28: error: omitted middle
>operand to '?:' operand cannot be mangled
>template<int X, int Y> int test(c<X ? : Y>&); // { dg-error "omitted" }

Actually this kind of change makes a lot of sense to me (cmp clang too): since at that point we do *not* really know the location of the "required from" bit, just plainly admit it. Would it be possible in such cases to have a conditional in the diagnostic machinery and avoid the output of that bit completely when the location is UNKNOWN? I don't think there are existing cases where it's sane to say :0:0 as "required from"!?!

Short term at least, it would seem a good enough solution to me + a comment in testcase too.

Then indeed as you explained in your last message before leaving (enjoy Pisa! All in all I spent there ~10 years!) we should audit, avoid as much as possible such late diagnostic, etc.

What do you think?

Paolo
Jakub Jelinek - Sept. 7, 2013, 11:16 a.m.
On Sat, Sep 07, 2013 at 11:00:22AM +0200, Paolo Carlini wrote:
> and thanks for the analysis, now I understand the issue a little more.
> 
> On 09/07/2013 10:28 AM, Jan Hubicka wrote:
> >So it is just an accident that the line info is output sanely (if
> >line 9 is sane, I don't exactly know)
> I would say that in general it's definitely sane, because that is
> the instantiation point. And 10 is wrong, too late, it points to the
> closing bracket.
> 
> However, I notice that clang doesn't even try to output a message
> having to do with line 9: if I understand correctly, if that
> operator cannot be mangled, that happens unconditionally, isn't
> something that may succeed. Thus I wonder if, instead of outputting
> garbage line numbers we could at least suppress in such cases the
> whole "In instantiation of..." part of the diagnostic, it would be
> better than garbage. Do we have a mechanism for that?
> 
> Now I also understand that this should be a very uncommon error
> message, but, I'm wondering, is it possible that other errors, for
> other issues, are also affected? That is, other diagnostic happening
> very late and sensitive to the recent rework?

As I wrote in the PR, IMHO mangle_decl should
  location_t save_location = input_location;
  input_location = DECL_SOURCE_LOCATION (decl);
...
  input_location = save_location;
around the call, and perhaps also all the error calls inside of
mangle.c should be actually error_at (EXPR_LOC_OR_HERE (expr), ...),
so that if expr has locus, it will print it at that point, otherwise
will use locus of the decl.

	Jakub
Jason Merrill - Sept. 7, 2013, 4:40 p.m.
On 09/07/2013 06:13 AM, Paolo Carlini wrote:
> Actually this kind of change makes a lot of sense to me (cmp clang too): since at that point we do *not* really know the location of the "required from" bit, just plainly admit it. Would it be possible in such cases to have a conditional in the diagnostic machinery and avoid the output of that bit completely when the location is UNKNOWN? I don't think there are existing cases where it's sane to say :0:0 as "required from"!?!

That does make sense.  We should disable "required from" for 
UNKNOWN_LOCATION.

Jason
Paolo Carlini - Sept. 7, 2013, 8:07 p.m.
Hi all, Jakub,

On 09/07/2013 01:16 PM, Jakub Jelinek wrote:
> As I wrote in the PR, IMHO mangle_decl should
>    location_t save_location = input_location;
>    input_location = DECL_SOURCE_LOCATION (decl);
> ...
>    input_location = save_location;
> around the call,
I had a look and I'm afraid this is already happening and isn't enough: 
in mangle_decl_string, the call of write_mangled_name is wrapped in 
exactly what you are suggesting. Or you mean something else?

Otherwise, I'm afraid we have to resort to what Jason too appears to 
find acceptable, thus what Honza proposed about UNKNOWN_LOCATION + the 
tweak to print_instantiation_partial_context_line (essentially just 
return immediately if loc == UNKNOWN_LOCATION, I think)

Paolo.
Jan Hubicka - Sept. 7, 2013, 9:33 p.m.
> Hi all, Jakub,
> 
> On 09/07/2013 01:16 PM, Jakub Jelinek wrote:
> >As I wrote in the PR, IMHO mangle_decl should
> >   location_t save_location = input_location;
> >   input_location = DECL_SOURCE_LOCATION (decl);
> >...
> >   input_location = save_location;
> >around the call,
> I had a look and I'm afraid this is already happening and isn't
> enough: in mangle_decl_string, the call of write_mangled_name is
> wrapped in exactly what you are suggesting. Or you mean something
> else?
> 
> Otherwise, I'm afraid we have to resort to what Jason too appears to
> find acceptable, thus what Honza proposed about UNKNOWN_LOCATION +
> the tweak to print_instantiation_partial_context_line (essentially
> just return immediately if loc == UNKNOWN_LOCATION, I think)

Yes, I think those two changes makes perfect case.  Middle end probably should
be more consistent about not setting input_locations beyhond of this point (it
does not really make much sense).  It is not how things works unforutnately -
RTL expansion still lives in statement at a time and uses input location to 
propagate things down and there are many other random setters of this variable.

I will try to audit things incrementally.  With the change to cgraphunit I made
this week all mangling ought to happen early (this is also not 100% true: in
some very special cases where we late access virtual tables through BINFOs, I
have independent fix for that).

Honza
> 
> Paolo.
Jan Hubicka - Sept. 7, 2013, 9:37 p.m.
> > Hi all, Jakub,
> > 
> > On 09/07/2013 01:16 PM, Jakub Jelinek wrote:
> > >As I wrote in the PR, IMHO mangle_decl should
> > >   location_t save_location = input_location;
> > >   input_location = DECL_SOURCE_LOCATION (decl);
> > >...
> > >   input_location = save_location;
> > >around the call,
> > I had a look and I'm afraid this is already happening and isn't
> > enough: in mangle_decl_string, the call of write_mangled_name is
> > wrapped in exactly what you are suggesting. Or you mean something
> > else?
> > 
> > Otherwise, I'm afraid we have to resort to what Jason too appears to
> > find acceptable, thus what Honza proposed about UNKNOWN_LOCATION +
> > the tweak to print_instantiation_partial_context_line (essentially
> > just return immediately if loc == UNKNOWN_LOCATION, I think)
> 
> Yes, I think those two changes makes perfect case.  Middle end probably should
> be more consistent about not setting input_locations beyhond of this point (it
> does not really make much sense).  It is not how things works unforutnately -
> RTL expansion still lives in statement at a time and uses input location to 
> propagate things down and there are many other random setters of this variable.
> 
> I will try to audit things incrementally.  With the change to cgraphunit I made
> this week all mangling ought to happen early (this is also not 100% true: in
> some very special cases where we late access virtual tables through BINFOs, I
> have independent fix for that).

Also I think with David's changes to get rid of global contextes, we probably should
have universum (w/o any location), function (with start/end locations as we have in cfun
now) and expression (with its own location) contextes now.

It would map to our global state, cfun+current_function_decl and
input_location+rtl_profile_for_bb in our current state of things.

Diagnostic than should know if it binds to specific function or specific statement
and can pick the porper location.

Honza
> 
> Honza
> > 
> > Paolo.

Patch

Index: cgraphunit.c
===================================================================
--- cgraphunit.c        (revision 202352)
+++ cgraphunit.c        (working copy)
@@ -913,6 +913,7 @@  analyze_functions (void)
 
   bitmap_obstack_initialize (NULL);
   cgraph_state = CGRAPH_STATE_CONSTRUCTION;
+  input_location = UNKNOWN_LOCATION;
 
   /* Ugly, but the fixup can not happen at a time same body alias is created;
      C++ FE is confused about the COMDAT groups being right.  */