[C++ PATCH] Fix a regression introduced by my PR34094 fix (PR c++/34238)

[C++ PATCH] Fix a regression introduced by my PR34094 fix (PR c++/34238)

[Jakub Jelinek]

Hi!

As shown by the attached testcase (which is probably valid, as
A<>::a is never used and so it shouldn't be necessary to define
it), the DECL_INITIAL (decl) == NULL check doesn't work in cases
where it is actually never needed, only instantiated
(in that case tsubst_decl clears DECL_INITIAL:
        /* We do NOT check for matching decls pushed separately at this
           point, as they may not represent instantiations of this
           template, and in any case are considered separate under the
           discrete model.  */
        r = copy_decl (t);
        DECL_USE_TEMPLATE (r) = 0;
        TREE_TYPE (r) = type;
        /* Clear out the mangled name and RTL for the instantiation.  */
        SET_DECL_ASSEMBLER_NAME (r, NULL_TREE);
        SET_DECL_RTL (r, NULL_RTX);
        DECL_INITIAL (r) = NULL_TREE;
        DECL_CONTEXT (r) = ctx;
).  The following patch fixes that.  For the const static data members 
with initializers in the class definition we won't issue any diagnostics,
but IMHO that's far less severe than making up the vars with default
initializer when there is none.  I've also tried (attached patch)
instead delay this diagnostics after the vars have been output, so we
know for sure if it was emitted or not.  That also fixes this anon9.C
and anon10.C gives diagnostics while it is without any diagnostics ATM,
but anon6.C breaks - there is the A<int>::c var actually emitted into
assembly, even when nobody really needs it :( (with or without the patch),
so with that alternate patch which checks TREE_ASM_WRITTEN we error on
anon6.C.

Is the first patch ok for trunk?  Bootstrapped/regtested on x86_64-linux.

2007-11-26  Jakub Jelinek  <jakub@redhat.com>

	PR c++/34238
	* decl2.c (cp_write_global_declarations): Test
	!DECL_INITIALIZED_IN_CLASS_P instead of checking NULL
	DECL_INITIAL.

	* g++.dg/ext/visibility/anon9.C: New test.

--- gcc/cp/decl2.c.jj	2007-11-22 15:03:54.000000000 +0100
+++ gcc/cp/decl2.c	2007-11-26 21:04:38.000000000 +0100
@@ -3375,7 +3375,7 @@ cp_write_global_declarations (void)
 		 namespace { struct A { static const int i = 4; } };
 		 decl_needed_p won't reliably detect whether it was
 		 really needed.  */
-	      if (DECL_IN_AGGR_P (decl) && DECL_INITIAL (decl) == NULL_TREE)
+	      if (DECL_IN_AGGR_P (decl) && !DECL_INITIALIZED_IN_CLASS_P (decl))
 		error ("%Jstatic data member %qD used, but not defined",
 		       decl, decl);
 	      DECL_EXTERNAL (decl) = 0;
--- gcc/testsuite/g++.dg/ext/visibility/anon9.C.jj	2007-11-26 21:37:34.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon9.C	2007-11-26 21:38:01.000000000 +0100
@@ -0,0 +1,11 @@
+// PR c++/34238
+// { dg-do compile }
+
+namespace
+{
+  template <typename T = int> struct A
+  {
+    static const bool a = true;
+  };
+}
+struct A<> a;

	Jakub

[C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Jakub Jelinek]

Hi!

Over time more problems with PR34094 fix have been reported, they are
summarized in the testcases below.
The following patch (which obsoletes my earlier 34238 patch) fixes all of
this, but letting cgraph_optimize actually emit all the vars into assembly
and only checking afterwards if we emitted something we shouldn't, which
seems to be much more reliable.
We still need to special case static data members initialized in the class,
because at least ATM sometimes such decls are emitted into assembly even if
they e.g. are just used through value (and thus not needed), while their
address wasn't taken etc., and we need to use for that
DECL_INITIALIZED_IN_CLASS_P test rather than check for non-NULL
DECL_INITIAL, because in templates even when initialized in class
DECL_INITIAL is often NULL.

Regtested on x86_64-linux, ok for trunk?

2007-12-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/34238
	PR c++/34340
	* decl2.c (cp_write_global_declarations): Move diagnostics about
	static data member used, but not defined, after cgraph_optimize,
	error only for decls that have been written and weren't
	DECL_INITIALIZED_IN_CLASS_P.

	* g++.dg/ext/visibility/anon9.C: New test.
	* g++.dg/ext/visibility/anon10.C: New test.
	* g++.dg/ext/visibility/repo7.C: New test.

--- gcc/cp/decl2.c.jj	2007-12-04 20:33:17.000000000 +0100
+++ gcc/cp/decl2.c	2007-12-05 13:37:34.000000000 +0100
@@ -3365,21 +3365,7 @@ cp_write_global_declarations (void)
 	  /* If this static data member is needed, provide it to the
 	     back end.  */
 	  if (DECL_NOT_REALLY_EXTERN (decl) && decl_needed_p (decl))
-	    {
-	      /* Error on
-		 namespace { struct A { static int i; }; }
-		 int foo () { return A::i; }
-		 without A::i definition (which can't be defined in
-		 a different CU because of the anonymous namespace).
-		 Don't do this if DECL_INITIAL is set, because for
-		 namespace { struct A { static const int i = 4; } };
-		 decl_needed_p won't reliably detect whether it was
-		 really needed.  */
-	      if (DECL_IN_AGGR_P (decl) && DECL_INITIAL (decl) == NULL_TREE)
-		error ("%Jstatic data member %qD used, but not defined",
-		       decl, decl);
-	      DECL_EXTERNAL (decl) = 0;
-	    }
+	    DECL_EXTERNAL (decl) = 0;
 	}
       if (VEC_length (tree, pending_statics) != 0
 	  && wrapup_global_declarations (VEC_address (tree, pending_statics),
@@ -3450,6 +3436,21 @@ cp_write_global_declarations (void)
 				 VEC_length (tree, pending_statics));
       emit_debug_global_declarations (VEC_address (tree, pending_statics),
 				      VEC_length (tree, pending_statics));
+      /* Error on
+	 namespace { struct A { static int i; }; }
+	 int foo () { return A::i; }
+	 without A::i definition (which can't be defined in
+	 a different CU because of the anonymous namespace).
+	 Don't do this if DECL_INITIALIZED_IN_CLASS_P is set, because for
+	 namespace { struct A { static const int i = 4; } };
+	 we won't reliably detect whether it was really needed
+	 and the definition couldn't provide different value than
+	 the initialized in class anyway.  */
+      for (i = 0; VEC_iterate (tree, pending_statics, i, decl); ++i)
+	if (DECL_IN_AGGR_P (decl)
+	    && TREE_ASM_WRITTEN (decl)
+	    && !DECL_INITIALIZED_IN_CLASS_P (decl))
+	  error ("%Jstatic data member %qD used, but not defined", decl, decl);
     }
 
   /* Generate hidden aliases for Java.  */
--- gcc/testsuite/g++.dg/ext/visibility/anon9.C.jj	2007-12-04 20:44:19.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon9.C	2007-12-04 20:44:19.000000000 +0100
@@ -0,0 +1,11 @@
+// PR c++/34238
+// { dg-do compile }
+
+namespace
+{
+  template <typename T = int> struct A
+  {
+    static const bool a = true;
+  };
+}
+struct A<> a;
--- gcc/testsuite/g++.dg/ext/visibility/anon10.C.jj	2007-12-05 15:07:46.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon10.C	2007-12-05 15:05:16.000000000 +0100
@@ -0,0 +1,44 @@
+// http://bugzilla.redhat.com/411871
+// { dg-do compile }
+
+extern "C" int printf (const char *, ...);
+
+struct E
+{
+  template <typename T> E (const volatile T&);
+  template <typename T> E (T&);
+  char x[64];
+};
+
+template<typename T> struct D
+{
+  static E foo (E, ...);
+  static int foo (T, int);
+};
+
+template<typename T, typename U> struct C
+{
+  static T ca;
+  static const int value = sizeof (D<U>::foo (ca, 0)) == sizeof (int);
+};
+
+struct A
+{
+  int a;
+};
+
+namespace
+{
+  struct B
+  {
+    int a;
+  };
+}
+
+int bar (void)
+{
+  C<A, int> a;
+  C<B, int> b;
+
+  return a.value + b.value;
+}
--- gcc/testsuite/g++.dg/template/repo7.C.jj	2007-12-05 13:46:47.000000000 +0100
+++ gcc/testsuite/g++.dg/template/repo7.C	2007-12-05 13:46:35.000000000 +0100
@@ -0,0 +1,29 @@
+// PR c++/34340
+// { dg-options "-frepo" }
+// { dg-final { cleanup-repo-files } }
+// { dg-require-host-local "" }
+
+struct A
+{
+  int a;
+};
+
+struct B
+{
+  static const A b;
+};
+
+template <typename T> struct D : public T
+{
+  static const A b;
+};
+
+const A B::b = { 1 };
+
+template<typename T> const A D<T>::b = { 2 };
+template class D<B>;
+
+int
+main ()
+{
+}

	Jakub

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Mark Mitchell]

Jakub Jelinek wrote:

> The following patch (which obsoletes my earlier 34238 patch) fixes all of
> this, but letting cgraph_optimize actually emit all the vars into assembly
> and only checking afterwards if we emitted something we shouldn't, which
> seems to be much more reliable.

The diagnostic is not about what the compiler happens to end up spitting
out; it's about the user's program.  If you do:

  namespace { struct A { static int i; }; }
  static int foo() { return A::i; }

the program is still erroneous, even though the compiler may likely
optimize away foo, and the reference to A::i.  So, I don't think we
should be doing this check after cgraph_optimize.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Jakub Jelinek]

On Thu, Dec 06, 2007 at 08:31:17PM -0800, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> > The following patch (which obsoletes my earlier 34238 patch) fixes all of
> > this, but letting cgraph_optimize actually emit all the vars into assembly
> > and only checking afterwards if we emitted something we shouldn't, which
> > seems to be much more reliable.
> 
> The diagnostic is not about what the compiler happens to end up spitting
> out; it's about the user's program.  If you do:
> 
>   namespace { struct A { static int i; }; }
>   static int foo() { return A::i; }
> 
> the program is still erroneous, even though the compiler may likely
> optimize away foo, and the reference to A::i.

That's true, but the standard explicitly says the diagnostic is not
required.  With
struct A { static int i; }; static int foo() { return A::i; } int main () {}
as whole program we wouldn't diagnose this either.  
To me it is more important that we don't error on valid programs (especially
a lot of boost using code is broken), and when all references are optimized
out, no real harm is done, compiler doesn't make up something unexpected.
When compiler puts it into bss, it can e.g. do so even for objects that need
constructing, without running any constructors on it etc.  That's something
that should be warned about.  If we have some bit which we can really rely
on to be set reliably for these purposes, then the warning can be precise,
but TREE_USED is unfortunately not it.

BTW, is the boost testcase valid?
template<typename T, typename U> struct C
{
  static T ca;
  static const int value = sizeof (D<U>::foo (ca, 0)) == sizeof (int);
};
is instantiated with T in anon namespace (so ca is also limited to current
CU), but it is (intentionally) never defined and only used in sizeof
expression.  [class.static.data]/5 says
"There shall be exactly one definition of a static data member that is used in a
program"
but it is not clear (at least to me) what exactly counts as use.
Boost certainly wouldn't like to pollute bss of programs with many
(sometimes big) variables that nobody ever uses.

I'm afraid I don't know the FE enough to further improve this warning
without breaking valid programs, so the other possibility for me is just to
back out the original patch and leave this can of worms to you/Jason.
Though, waiting for that in a state which at least errors out when real
harm is done rather than being silent even for those cases is IMHO
preferrable.

	Jakub

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Mark Mitchell]

Jakub Jelinek wrote:

>>   namespace { struct A { static int i; }; }
>>   static int foo() { return A::i; }
>>
>> the program is still erroneous, even though the compiler may likely
>> optimize away foo, and the reference to A::i.
> 
> That's true, but the standard explicitly says the diagnostic is not
> required.

It does?  Where?  (I'm not disagreeing, I'm just surprised, and I
couldn't find a pointer.)

> To me it is more important that we don't error on valid programs (especially
> a lot of boost using code is broken), and when all references are optimized
> out, no real harm is done, compiler doesn't make up something unexpected.

We don't want to issue errors about valid programs -- but we do want to
issue diagnostics about invalid programs.  Those can be warnings,
pedwarns, etc.; they don't necessarily have to stop compilation.

> that should be warned about.  If we have some bit which we can really rely
> on to be set reliably for these purposes, then the warning can be precise,
> but TREE_USED is unfortunately not it.

TREE_USED should be accurate.  It should tell us whether or not a thing
was used by the input.  If TREE_USED is not set when it should be, or
set when it shouldn't be, we have a problem.

> template<typename T, typename U> struct C
> {
>   static T ca;
>   static const int value = sizeof (D<U>::foo (ca, 0)) == sizeof (int);
> };

I don't think the standard is not entirely clear.  It says this code is
not evaluated, but I don't see anything that says that this means that
"ca" is not used.  Of course, might well not require that, independent
of what the standard says; I agree that QoI suggests not issuing an
error in this case.

> I'm afraid I don't know the FE enough to further improve this warning
> without breaking valid programs, so the other possibility for me is just to
> back out the original patch and leave this can of worms to you/Jason.

All right; let's revert your original patch, then.  AIUI, that patch
fixed an accepts-invalid problem; that's better than rejects-valid problems.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

On Fri, Dec 07, 2007 at 12:52:31PM -0800, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> >>   namespace { struct A { static int i; }; }
> >>   static int foo() { return A::i; }
> >>
> >> the program is still erroneous, even though the compiler may likely
> >> optimize away foo, and the reference to A::i.
> > 
> > That's true, but the standard explicitly says the diagnostic is not
> > required.
> 
> It does?  Where?  (I'm not disagreeing, I'm just surprised, and I
> couldn't find a pointer.)

[class.static.data]/5:
"There shall be exactly one definition of a static data member that is used in a program; no diagnostic is required; see 3.2."

The patch I posted last Wednesday diagnosed the errors basically for the
same stuff that ld would diagnose it, if anon namespace was not involved.
When all uses are optimized out, etc., ld will not error nor warn either.

> > that should be warned about.  If we have some bit which we can really rely
> > on to be set reliably for these purposes, then the warning can be precise,
> > but TREE_USED is unfortunately not it.
> 
> TREE_USED should be accurate.  It should tell us whether or not a thing
> was used by the input.  If TREE_USED is not set when it should be, or
> set when it shouldn't be, we have a problem.

But it is set e.g. when seen in sizeof as shown on that boost snippet.

> > I'm afraid I don't know the FE enough to further improve this warning
> > without breaking valid programs, so the other possibility for me is just to
> > back out the original patch and leave this can of worms to you/Jason.
> 
> All right; let's revert your original patch, then.  AIUI, that patch
> fixed an accepts-invalid problem; that's better than rejects-valid problems.

If you prefer that, here is a patch I've bootstrapped/regtested on
x86_64-linux.

	Jakub

[-- Attachment #2: Z93 --]
[-- Type: text/plain, Size: 2939 bytes --]

2007-12-10  Jakub Jelinek  <jakub@redhat.com>

	PR c++/34238
	* decl2.c (cp_write_global_declarations): Revert 2007-11-22 change.

	* g++.dg/ext/visibility/anon7.C: Add xfail.
	* g++.dg/ext/visibility/anon9.C: New test.
	* g++.dg/ext/visibility/anon10.C: New test.

--- gcc/cp/decl2.c.jj	2007-12-09 22:25:40.000000000 +0100
+++ gcc/cp/decl2.c	2007-12-09 22:29:59.000000000 +0100
@@ -3366,21 +3366,7 @@ cp_write_global_declarations (void)
 	  /* If this static data member is needed, provide it to the
 	     back end.  */
 	  if (DECL_NOT_REALLY_EXTERN (decl) && decl_needed_p (decl))
-	    {
-	      /* Error on
-		 namespace { struct A { static int i; }; }
-		 int foo () { return A::i; }
-		 without A::i definition (which can't be defined in
-		 a different CU because of the anonymous namespace).
-		 Don't do this if DECL_INITIAL is set, because for
-		 namespace { struct A { static const int i = 4; } };
-		 decl_needed_p won't reliably detect whether it was
-		 really needed.  */
-	      if (DECL_IN_AGGR_P (decl) && DECL_INITIAL (decl) == NULL_TREE)
-		error ("%Jstatic data member %qD used, but not defined",
-		       decl, decl);
-	      DECL_EXTERNAL (decl) = 0;
-	    }
+	    DECL_EXTERNAL (decl) = 0;
 	}
       if (VEC_length (tree, pending_statics) != 0
 	  && wrapup_global_declarations (VEC_address (tree, pending_statics),
--- gcc/testsuite/g++.dg/ext/visibility/anon7.C.jj	2007-11-20 19:12:40.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon7.C	2007-12-09 20:54:27.000000000 +0100
@@ -5,7 +5,7 @@ namespace
 {
   struct A {
     static int bar ();
-    static int i;		// { dg-error "used, but not defined" }
+    static int i;		// { dg-error "used, but not defined" "" { xfail *-*-* } }
     static int j;
     static int k;
     static int l;
--- gcc/testsuite/g++.dg/ext/visibility/anon9.C.jj	2007-12-04 20:44:19.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon9.C	2007-12-04 20:44:19.000000000 +0100
@@ -0,0 +1,11 @@
+// PR c++/34238
+// { dg-do compile }
+
+namespace
+{
+  template <typename T = int> struct A
+  {
+    static const bool a = true;
+  };
+}
+struct A<> a;
--- gcc/testsuite/g++.dg/ext/visibility/anon10.C.jj	2007-12-05 15:07:46.000000000 +0100
+++ gcc/testsuite/g++.dg/ext/visibility/anon10.C	2007-12-05 15:05:16.000000000 +0100
@@ -0,0 +1,44 @@
+// http://bugzilla.redhat.com/411871
+// { dg-do compile }
+
+extern "C" int printf (const char *, ...);
+
+struct E
+{
+  template <typename T> E (const volatile T&);
+  template <typename T> E (T&);
+  char x[64];
+};
+
+template<typename T> struct D
+{
+  static E foo (E, ...);
+  static int foo (T, int);
+};
+
+template<typename T, typename U> struct C
+{
+  static T ca;
+  static const int value = sizeof (D<U>::foo (ca, 0)) == sizeof (int);
+};
+
+struct A
+{
+  int a;
+};
+
+namespace
+{
+  struct B
+  {
+    int a;
+  };
+}
+
+int bar (void)
+{
+  C<A, int> a;
+  C<B, int> b;
+
+  return a.value + b.value;
+}

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Mark Mitchell]

Jakub Jelinek wrote:
> On Fri, Dec 07, 2007 at 12:52:31PM -0800, Mark Mitchell wrote:
>> Jakub Jelinek wrote:
>>
>>>>   namespace { struct A { static int i; }; }
>>>>   static int foo() { return A::i; }
>>>>
>>>> the program is still erroneous, even though the compiler may likely
>>>> optimize away foo, and the reference to A::i.
>>> That's true, but the standard explicitly says the diagnostic is not
>>> required.
>> It does?  Where?  (I'm not disagreeing, I'm just surprised, and I
>> couldn't find a pointer.)
> 
> [class.static.data]/5:
> "There shall be exactly one definition of a static data member that is used in a program; no diagnostic is required; see 3.2."

Thanks.

However, I would suspect that there is also something that says that if
you use the value of a variable (whether a static data member or not),
the variable must be defined in the program.  I'm sure that you're
right, though, thinking a bit more about it; that probably says that no
diagnostic is required too.

> The patch I posted last Wednesday diagnosed the errors basically for the
> same stuff that ld would diagnose it, if anon namespace was not involved.
> When all uses are optimized out, etc., ld will not error nor warn either.
> 
>>> that should be warned about.  If we have some bit which we can really rely
>>> on to be set reliably for these purposes, then the warning can be precise,
>>> but TREE_USED is unfortunately not it.
>> TREE_USED should be accurate.  It should tell us whether or not a thing
>> was used by the input.  If TREE_USED is not set when it should be, or
>> set when it shouldn't be, we have a problem.
> 
> But it is set e.g. when seen in sizeof as shown on that boost snippet.

Hmph, I see.  I suspect that the standard itself isn't consistent on
this point.  For example, there is language that says that if you use
the name of a member in a class definition, the member name must refer
to the same thing after the definition.  For example:

  const int i = 7;
  struct S {
     int a[i];
     static const int i = 3;
  };

is erroneous.  But, surely:

  int i;
  struct S {
     int a[sizeof(i)];
     double i;
  };

is also erroneous.  So, in this context "use" must mean what TREE_USED
is meaning.

Frankly, I think the Boost code is trying too hard to be clever.
Defining the variable is not a big deal, and if the compiler can pull
out all references to the variable, it can pull out the variable too!
But, what I think doesn't necessarily matter...

>>> I'm afraid I don't know the FE enough to further improve this warning
>>> without breaking valid programs, so the other possibility for me is just to
>>> back out the original patch and leave this can of worms to you/Jason.
>> All right; let's revert your original patch, then.  AIUI, that patch
>> fixed an accepts-invalid problem; that's better than rejects-valid problems.
> 
> If you prefer that, here is a patch I've bootstrapped/regtested on
> x86_64-linux.

OK, please apply that.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [C++ PATCH] Fix regressions introduced by my PR34094 fix (PR c++/34238, c++/34340)

[Jason Merrill]

It seems safe to give the error if DECL_SYMBOL_REFERENCED 
(DECL_ASSEMBLER_NAME) is set, just decl_needed_p is too strong a test.

Jason