[RFC] Fix for PR58201

[RFC] Fix for PR58201

[Jan Hubicka]

Hi,
this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o
bodies I did not really anticipate.

Here removal of the arguments changes mangling algorithm if
set_decl_assembler_name is invoked late.  This is something I wanted to get rid
of for a long time:  we already compute assembler names for every symbol that
lands symbol table after the early cleanups for every unit that is LTOed and
every unit containing any alias directive.  I think it will make things
smoother if we computed it always early: other persistent source of problems
are same body aliases that are created as a side effect of langhook of
set_decl_assembler_name and that may happen at a time IPA code does not really
expect new functions/variables to appear.

So independently of DECL_ARGUMENTS/DECL_RESULT issues, I would like to propose the
following patch that triggers unconditional computation of DECL_ASSEMBLER_NAME and the
real symbol table construction.  I already benchmarked it few months ago and the
erformance implications seems in wash.

Just to keep things linked, the other two fallouts are
 1) problem with thunks needing DECL_ARGUMENTS when they are output in gimple way
    but these are not streamed, handled by http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00057.html
 2) problem with variable sized arguments and return values
    "fixed" by http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00078.html
    (the fix restored old behaviour, but I do not think it fixes the whole issue
    as discussed in the thread and provided with another testcases that ICEs the
    compiler independently of my changes).

I would like to basically ask if it seems to make sense to go this route and
try to get rid of those declarations.

For LTO it is really nice optimization - instead of streaming 4 decls at
average for a function (function-decl, result-decl and parm-decls), we need
just one. This change was originally motivated by memory analysis of firefox
WPA build where PARM_DECL was the most common type of tree just after
TREE_LIST.  I also think it makes sense from backend point of view to consider
these part of the function body representation, just as DECL_STRUCT_FUNCTOIN
and DECL_INITIAL is.

Even in non-LTO we save a lot of decls for all the external declarations that
are kept in memory.

I would be willing to try to analyze/fix the issues if they appear and I tried
quite curefuly to examine the existing code dealing with arguments. There
are obviously interesting scenarios, like this mangling issue, I missed.

If this does not seem to make sense, I will prepare patch to revert the changes.
If it does, I will commit those fixes and hope things will stabilize quickly.

Honza

	* cgraphunit.c (analyze_functions): Populate assembler names once done
	with early unreachable function removal.

Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 202199)
+++ cgraphunit.c	(working copy)
@@ -1074,6 +1086,7 @@
   bitmap_obstack_release (NULL);
   pointer_set_destroy (reachable_call_targets);
   ggc_collect ();
+  symtab_initialize_asm_name_hash ();
 }
 
 /* Translate the ugly representation of aliases as alias pairs into nice

Re: [RFC] Fix for PR58201

[Jakub Jelinek]

On Wed, Sep 04, 2013 at 06:04:09PM +0200, Jan Hubicka wrote:
> 	* cgraphunit.c (analyze_functions): Populate assembler names once done
> 	with early unreachable function removal.

Please add some of the testcases from the PRs and mention the PRs in the
ChangeLog entry.

> --- cgraphunit.c	(revision 202199)
> +++ cgraphunit.c	(working copy)
> @@ -1074,6 +1086,7 @@
>    bitmap_obstack_release (NULL);
>    pointer_set_destroy (reachable_call_targets);
>    ggc_collect ();
> +  symtab_initialize_asm_name_hash ();
>  }
>  
>  /* Translate the ugly representation of aliases as alias pairs into nice

	Jakub

Re: [RFC] Fix for PR58201

[Bernd Schmidt]

On 09/04/2013 06:04 PM, Jan Hubicka wrote:
> this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o
> bodies I did not really anticipate.
[...]
> I would like to basically ask if it seems to make sense to go this route and
> try to get rid of those declarations.

I'm currently working on a new target, ptx, which uses a
pseudo-assembler where functions (even extern ones) need to be declared
with their arguments and return types. With my current code I have to
look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite
sure yet whether the change to delete them will break the backend.


Bernd

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> On 09/04/2013 06:04 PM, Jan Hubicka wrote:
> > this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o
> > bodies I did not really anticipate.
> [...]
> > I would like to basically ask if it seems to make sense to go this route and
> > try to get rid of those declarations.
> 
> I'm currently working on a new target, ptx, which uses a
> pseudo-assembler where functions (even extern ones) need to be declared
> with their arguments and return types. With my current code I have to
> look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite
> sure yet whether the change to delete them will break the backend.

How do you support K&R functions here?  My basic idea was that TYPE_ARG_TYPES
should give enough information about external function calling convention
anyone will ever need. I would hope that this will be sufficient for your
use, too, despite the fact you no longer have parameter names at hand
and you also lose info about external inline K&R-style delcared functions
that has been optimized out.

If not that indeed, you will not see DECL_ARGUMENTS for external function
anytime after cgraph_remove_unreachable_functions is called.

Honza

Re: [RFC] Fix for PR58201

[Bernd Schmidt]

On 09/04/2013 07:09 PM, Jan Hubicka wrote:
> How do you support K&R functions here?  My basic idea was that TYPE_ARG_TYPES
> should give enough information about external function calling convention
> anyone will ever need. I would hope that this will be sufficient for your
> use, too, despite the fact you no longer have parameter names at hand
> and you also lose info about external inline K&R-style delcared functions
> that has been optimized out.

TYPE_ARG_TYPES doesn't exist for all functions, so right now the backend
is using whichever of the two is available. It seems that TYPE_ARG_TYPES
is actually NULL for K&R functions (gcc.c-torture/compile/20000403-1.c
is the first one that fails if I try to use only TYPE_ARG_TYPES).


Bernd

Re: [RFC] Fix for PR58201

[Jeff Law]

On 09/04/2013 10:49 AM, Bernd Schmidt wrote:
> On 09/04/2013 06:04 PM, Jan Hubicka wrote:
>> this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o
>> bodies I did not really anticipate.
> [...]
>> I would like to basically ask if it seems to make sense to go this route and
>> try to get rid of those declarations.
>
> I'm currently working on a new target, ptx, which uses a
> pseudo-assembler where functions (even extern ones) need to be declared
> with their arguments and return types. With my current code I have to
> look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite
> sure yet whether the change to delete them will break the backend.
IIRC the PA had similar requirements as well -- in 
ASM_DECLARE_FUNCTION_NAME we have to peek at DECL_ARGUMENTS so we can 
pass to the assembler & linker which registers hold arguments.

Jeff

Re: [RFC] Fix for PR58201

[Richard Biener]

Jan Hubicka <hubicka@ucw.cz> wrote:
>> On 09/04/2013 06:04 PM, Jan Hubicka wrote:
>> > this is third fallout of my change to remove
>DECL_ARGUMENTS/DECL_RESULT for functions w/o
>> > bodies I did not really anticipate.
>> [...]
>> > I would like to basically ask if it seems to make sense to go this
>route and
>> > try to get rid of those declarations.
>> 
>> I'm currently working on a new target, ptx, which uses a
>> pseudo-assembler where functions (even extern ones) need to be
>declared
>> with their arguments and return types. With my current code I have to
>> look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite
>> sure yet whether the change to delete them will break the backend.
>
>How do you support K&R functions here?  My basic idea was that
>TYPE_ARG_TYPES
>should give enough information about external function calling
>convention
>anyone will ever need. I would hope that this will be sufficient for
>your
>use, too, despite the fact you no longer have parameter names at hand
>and you also lose info about external inline K&R-style delcared
>functions
>that has been optimized out.
>
>If not that indeed, you will not see DECL_ARGUMENTS for external
>function
>anytime after cgraph_remove_unreachable_functions is called.

In fact it has to work because of indirect calls and how we now handle gimple call abi via gimple_call_fntype.

Richard.

>Honza

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> On 09/04/2013 10:49 AM, Bernd Schmidt wrote:
> >On 09/04/2013 06:04 PM, Jan Hubicka wrote:
> >>this is third fallout of my change to remove DECL_ARGUMENTS/DECL_RESULT for functions w/o
> >>bodies I did not really anticipate.
> >[...]
> >>I would like to basically ask if it seems to make sense to go this route and
> >>try to get rid of those declarations.
> >
> >I'm currently working on a new target, ptx, which uses a
> >pseudo-assembler where functions (even extern ones) need to be declared
> >with their arguments and return types. With my current code I have to
> >look at DECL_ARGUMENTS fairly late in the compilation. I'm not quite
> >sure yet whether the change to delete them will break the backend.
> IIRC the PA had similar requirements as well -- in
> ASM_DECLARE_FUNCTION_NAME we have to peek at DECL_ARGUMENTS so we
> can pass to the assembler & linker which registers hold arguments.

This use should be safe, too. ASM_DECLARE_FUNCTION_NAME is called on function whose
body is being output and there we do have DECL_ARGUMENTS (as we consider them
part of the body)

Honza

Re: [RFC] Fix for PR58201

[Jan Hubicka]

Hi,
this is the patch I commited after testing on x86_64-linux.

Honza

Index: ChangeLog
===================================================================
*** ChangeLog	(revision 202271)
--- ChangeLog	(working copy)
***************
*** 1,3 ****
--- 1,9 ----
+ 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.
+ 
  2013-09-04  Dodji Seketeli  <dodji@redhat.com>
  
  	* tree.h (DECL_BUILT_IN): Fix typo in comment.
Index: cgraphunit.c
===================================================================
*** cgraphunit.c	(revision 202271)
--- cgraphunit.c	(working copy)
*************** analyze_functions (void)
*** 1064,1069 ****
--- 1064,1071 ----
  	}
        node->symbol.aux = NULL;
      }
+   for (;node; node = node->symbol.next)
+     node->symbol.aux = NULL;
    first_analyzed = cgraph_first_function ();
    first_analyzed_var = varpool_first_variable ();
    if (cgraph_dump_file)
*************** analyze_functions (void)
*** 1074,1079 ****
--- 1076,1086 ----
    bitmap_obstack_release (NULL);
    pointer_set_destroy (reachable_call_targets);
    ggc_collect ();
+   /* Initialize assembler name hash, in particular we want to trigger C++
+      mangling and same body alias creation before we free DECL_ARGUMENTS
+      used by it.  */
+   if (!seen_error ())
+   symtab_initialize_asm_name_hash ();
  }
  
  /* Translate the ugly representation of aliases as alias pairs into nice
Index: testsuite/ChangeLog
===================================================================
*** testsuite/ChangeLog	(revision 202297)
--- testsuite/ChangeLog	(working copy)
***************
*** 1,3 ****
--- 1,10 ----
+ 2013-09-04  Jan Hubicka  <jh@suse.cz>
+ 
+ 	PR middle-end/58201
+ 	* g++.dg/torture/pr58201_0.C: New testcase.
+ 	* g++.dg/torture/pr58201_1.C: New testcase.
+ 	* g++.dg/torture/pr58201.h: New testcase.
+ 
  2013-09-05  Jan Hubicka  <jh@suse.cz>
  
  	* gcc.dg/autopar/pr49960.c: Disable partial inlining
Index: testsuite/g++.dg/torture/pr58201_0.C
===================================================================
*** testsuite/g++.dg/torture/pr58201_0.C	(revision 0)
--- testsuite/g++.dg/torture/pr58201_0.C	(revision 0)
***************
*** 0 ****
--- 1,9 ----
+ #include "pr58201.h"    
+                     
+ C::C2::C2(){ }      
+ C::C2::~C2() { }    
+                     
+ int main ()         
+ {                   
+     return 0;       
+ }  
Index: testsuite/g++.dg/torture/pr58201_1.C
===================================================================
*** testsuite/g++.dg/torture/pr58201_1.C	(revision 0)
--- testsuite/g++.dg/torture/pr58201_1.C	(revision 0)
***************
*** 0 ****
--- 1,10 ----
+ /* { dg-do link } */
+ /* { dg-options "-O2" } */
+ /* { dg-additional-sources "pr58201_0.C" } */
+ #include "pr58201.h"
+ 
+ A::A() { }
+ A::~A() { }
+ B::B() { }
+ B::~B() { }
+ 
Index: testsuite/g++.dg/torture/pr58201.h
===================================================================
*** testsuite/g++.dg/torture/pr58201.h	(revision 0)
--- testsuite/g++.dg/torture/pr58201.h	(revision 0)
***************
*** 0 ****
--- 1,24 ----
+ class A
+ {
+  protected:
+   A();
+   virtual ~A();
+ };
+ 
+ class B : virtual public A
+ {
+  public:
+   B();
+   virtual ~B();
+ };
+ 
+ class C
+ {
+  private:
+   class C2 : public B
+    {
+    public:
+      C2();
+      virtual ~C2();
+    };
+ };

Re: [RFC] Fix for PR58201

[Paolo Carlini]

Hi Honza,

On 09/06/2013 01:05 AM, Jan Hubicka wrote:
> Hi,
> this is the patch I commited after testing on x86_64-linux.
>
> Honza
>
> Index: ChangeLog
> ===================================================================
> *** ChangeLog	(revision 202271)
> --- ChangeLog	(working copy)
> ***************
> *** 1,3 ****
> --- 1,9 ----
> + 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?

After the recent breakages, up to date testresults are arriving only now 
on gcc-testresults confirming my finding, for example Andreas Schwab 
already posted a couple.

Thanks!
Paolo.

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> >+ 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] ()

It seems to be latent bug in C++ FE to not set location correctly to error - no one
promise that the current locus will correspond to the declaration being mangled
during the lazy call of assembler name.  So I suppose error needs to be updated
to get correct locus?

Honza

> 
> After the recent breakages, up to date testresults are arriving only
> now on gcc-testresults confirming my finding, for example Andreas
> Schwab already posted a couple.
> 
> Thanks!
> Paolo.

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> > >+ 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:
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" }

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

Re: [RFC] Fix for PR58201

[Paolo Carlini]

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.

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> 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

Re: [RFC] Fix for PR58201

[Paolo Carlini]

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

Re: [RFC] Fix for PR58201

[Jakub Jelinek]

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

Re: [RFC] Fix for PR58201

[Jason Merrill]

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

Re: [RFC] Fix for PR58201

[Paolo Carlini]

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.

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> 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.

Re: [RFC] Fix for PR58201

[Jan Hubicka]

> > 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.