PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Ian Lance Taylor]

PR 33094 is about a small C++ test case which mixes anonymous
namespaces, templates, virtual methods, and initialized static const
members to create an ICE.

namespace
{

template <typename T>
class A
{
  virtual T f1() { return c; }
  static const T c = 0;
};

A<int> v;

}

The ICE occurs here in make_rtl_for_nonlocal_decl in cp/decl.c:

      /* An in-class declaration of a static data member should be
	 external; it is only a declaration, and not a definition.  */
      if (init == NULL_TREE)
	gcc_assert (DECL_EXTERNAL (decl));

make_rtl_for_nonlocal_decl is called by cp_finish_decl.
cp_finish_decl is called with a non-NULL INIT parameter.
cp_finish_decl does this:
	  init = check_initializer (decl, init, flags, &cleanup);
check_initializer sees that the type in question (int) does not
require a constructor, and winds up doing this:
	      init_code = store_init_value (decl, init);
and returning NULL.  store_init_value puts the value into DECL_INITIAL
of DECL.

All those steps seem reasonable.  If the anonymous namespace is
removed from the test case, all is well because DECL_EXTERNAL is set.
When using the anonymous namespace, DECL_EXTERNAL is not set.
Specifically, it is cleared in cp_write_global_declarations:
	  /* If this static data member is needed, provide it to the
	     back end.  */
	  if (DECL_NOT_REALLY_EXTERN (decl) && decl_needed_p (decl))
	    DECL_EXTERNAL (decl) = 0;

The DECL_NOT_REALLY_EXTERN flag is only set when using an anonymous
namespace.  It happens in constrain_visibility:

  if (visibility == VISIBILITY_ANON)
    {
      /* extern "C" declarations aren't affected by the anonymous
	 namespace.  */
      if (!DECL_EXTERN_C_P (decl))
	{
	  TREE_PUBLIC (decl) = 0;
	  DECL_INTERFACE_KNOWN (decl) = 1;
	  if (DECL_LANG_SPECIFIC (decl))
	    DECL_NOT_REALLY_EXTERN (decl) = 1;
	}
    }


To sum up: the test in make_rtl_for_nonlocal_decl does not consider
the possibility that the initializer might be in DECL_INITIAL.  This
normally works because static class members normally have
DECL_EXTERNAL set.  However, when an anonymous namespace is used
DECL_EXTERNAL is cleared.


Given the above, this patch fixes the problem.  I ran a bootstrap and
check-g++ on i686-pc-linux-gnu.

Does this look OK?

Ian


2007-08-17  Ian Lance Taylor  <iant@google.com>

	PR c++/33094
	* decl.c (make_rtl_for_nonlocal_decl): Test DECL_INITIAL as well
	as init.


Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 127491)
+++ cp/decl.c	(working copy)
@@ -4963,7 +4963,7 @@ make_rtl_for_nonlocal_decl (tree decl, t
       gcc_assert (TREE_STATIC (decl));
       /* An in-class declaration of a static data member should be
 	 external; it is only a declaration, and not a definition.  */
-      if (init == NULL_TREE)
+      if (init == NULL_TREE && DECL_INITIAL (decl) == NULL_TREE)
 	gcc_assert (DECL_EXTERNAL (decl));
     }
 

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Mark Mitchell]

Ian Lance Taylor wrote:

> namespace
> {
> 
> template <typename T>
> class A
> {
>   virtual T f1() { return c; }
>   static const T c = 0;
> };

Thanks for the analysis!

> All those steps seem reasonable.  If the anonymous namespace is
> removed from the test case, all is well because DECL_EXTERNAL is set.
> When using the anonymous namespace, DECL_EXTERNAL is not set.

That seems wrong.  This declaration is not a definition.  For example, a
reference to &A<int>::c from within this file should be unresolved at
link-time.  (The standard says that this is an invalid thing to do; I
don't know if it says that a diagnostic is required.)

> 	  /* If this static data member is needed, provide it to the
> 	     back end.  */
> 	  if (DECL_NOT_REALLY_EXTERN (decl) && decl_needed_p (decl))
> 	    DECL_EXTERNAL (decl) = 0;

I think DECL_NOT_REALLY_EXTERN is the wrong test here, now that things
with anonymous namespaces get ELF local linkage.  The confusion here is
that the language says they have external linkage; it's an optimization
that, in generating object files, we now give them ELF local linkage.

There's no such thing as a "static" forward declaration (e.g., "static
int i;" is a definition, not a declaration that "i" exists) -- but now
we've essentially created such a thing.

I think DECL_EXTERNAL has historically only applied to TREE_PUBLIC
things -- but probably that's the best way to represent that.  In other
words, interpret DECL_EXTERNAL as "there is no definition of this entity
in this translation unit (yet)".

> The DECL_NOT_REALLY_EXTERN flag is only set when using an anonymous
> namespace.  It happens in constrain_visibility:

I think that's wrong.  I think we should leave DECL_NOT_REALLY_EXTERN
clear until/unless we actually encounter a definition.  I don't think
constrain_visibility should have any effect on DECL_NOT_REALLY_EXTERN,
given the above.

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

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Andrew Pinski]

On 8/17/07, Mark Mitchell <mark@codesourcery.com> wrote:
> (The standard says that this is an invalid thing to do; I
> don't know if it says that a diagnostic is required.)

The standard say no diagnostic is required but since this is an
anonymous namespace, we can most likely error out here anyways if it
is not declared later on.

Thanks,
Andrew Pinski

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Mark Mitchell]

Andrew Pinski wrote:
> On 8/17/07, Mark Mitchell <mark@codesourcery.com> wrote:
>> (The standard says that this is an invalid thing to do; I
>> don't know if it says that a diagnostic is required.)
> 
> The standard say no diagnostic is required but since this is an
> anonymous namespace, we can most likely error out here anyways if it
> is not declared later on.

OK.

The bottom line is that I don't think putting the code in an anonymous
namespace should have any affect on whether or not it links.  Certainly,
if putting things in an anonymous namespaces increases codesize (by
emitting unnecessary definitions) that's a bug.

Thanks,

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

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Gabriel Dos Reis]

Mark Mitchell <mark@codesourcery.com> writes:

| Andrew Pinski wrote:
| > On 8/17/07, Mark Mitchell <mark@codesourcery.com> wrote:
| >> (The standard says that this is an invalid thing to do; I
| >> don't know if it says that a diagnostic is required.)
| > 
| > The standard say no diagnostic is required but since this is an
| > anonymous namespace, we can most likely error out here anyways if it
| > is not declared later on.
| 
| OK.
| 
| The bottom line is that I don't think putting the code in an anonymous
| namespace should have any affect on whether or not it links.  Certainly,
| if putting things in an anonymous namespaces increases codesize (by
| emitting unnecessary definitions) that's a bug.

I believe Mark is right.

-- Gaby

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Jason Merrill]

Mark Mitchell wrote:
> Ian Lance Taylor wrote:
> 
>> namespace
>> {
>>
>> template <typename T>
>> class A
>> {
>>   virtual T f1() { return c; }
>>   static const T c = 0;
>> };
> 
> Thanks for the analysis!
> 
>> All those steps seem reasonable.  If the anonymous namespace is
>> removed from the test case, all is well because DECL_EXTERNAL is set.
>> When using the anonymous namespace, DECL_EXTERNAL is not set.
> 
> That seems wrong.  This declaration is not a definition.  For example, a
> reference to &A<int>::c from within this file should be unresolved at
> link-time.  (The standard says that this is an invalid thing to do; I
> don't know if it says that a diagnostic is required.)

Yes.  But this happens regardless of whether DECL_EXTERNAL is set.  You 
really can't rely on DECL_EXTERNAL to tell you whether or not something 
is defined; for functions we use DECL_INITAL, for static member 
variables we use DECL_IN_AGGR_P.  constrain_visibility is doing the same 
sort of thing that mark_definable and grokfndecl already did, setting 
DECL_NOT_REALLY_EXTERN not because we have seen a definition, but 
because we've decided that we'd like to write this decl out if we can.

DECL_EXTERNAL doesn't indicate whether or not something is defined, it 
indicates whether or not we need to call assemble_external when we see a 
reference to it.

DECL_NOT_REALLY_EXTERN also doesn't indicate whether or not something is 
defined, it indicates whether or not we should emit it if we have a 
definition and it is needed.

I'm not arguing that this is a sensible design, but that's how the 
compiler already works.  It's simple enough to make the variable in this 
testcase have DECL_EXTERNAL again as you expect, but it's not clear to 
me that this buys us anything.  Do you want to try harder to not set 
DECL_NOT_REALLY_EXTERN in cases where we don't have a definition?

Jason

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Jason Merrill]

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

Since I'm about to go off to the C++ meeting, I've gone ahead and fixed 
the crash by allowing !TREE_PUBLIC as well as DECL_EXTERN.  And added a 
test to make sure that we are emitting static member variables iff they 
have an out-of-class definition.

Tested x86_64-pc-linux-gnu, applied to trunk.  I'd like to apply this to 
4.2 as well; OK, Mark?



[-- Attachment #2: anon-static.patch --]
[-- Type: text/x-patch, Size: 3388 bytes --]

2007-09-28  Jason Merrill  <jason@redhat.com>

	PR c++/33094
	* decl.c (make_rtl_for_nonlocal_decl): It's ok for a member 
	constant to not have DECL_EXTERNAL if it's file-local.

Index: cp/decl.c
===================================================================
*** cp/decl.c	(revision 128841)
--- cp/decl.c	(working copy)
*************** make_rtl_for_nonlocal_decl (tree decl, t
*** 5092,5098 ****
        /* An in-class declaration of a static data member should be
  	 external; it is only a declaration, and not a definition.  */
        if (init == NULL_TREE)
! 	gcc_assert (DECL_EXTERNAL (decl));
      }
  
    /* We don't create any RTL for local variables.  */
--- 5092,5098 ----
        /* An in-class declaration of a static data member should be
  	 external; it is only a declaration, and not a definition.  */
        if (init == NULL_TREE)
! 	gcc_assert (DECL_EXTERNAL (decl) || !TREE_PUBLIC (decl));
      }
  
    /* We don't create any RTL for local variables.  */
Index: cp/cp-tree.h
===================================================================
*** cp/cp-tree.h	(revision 128841)
--- cp/cp-tree.h	(working copy)
*************** more_aggr_init_expr_args_p (const aggr_i
*** 3225,3231 ****
  
  /* DECL_EXTERNAL must be set on a decl until the decl is actually emitted,
     so that assemble_external will work properly.  So we have this flag to
!    tell us whether the decl is really not external.  */
  #define DECL_NOT_REALLY_EXTERN(NODE) \
    (DECL_LANG_SPECIFIC (NODE)->decl_flags.not_really_extern)
  
--- 3225,3235 ----
  
  /* DECL_EXTERNAL must be set on a decl until the decl is actually emitted,
     so that assemble_external will work properly.  So we have this flag to
!    tell us whether the decl is really not external.
! 
!    This flag does not indicate whether or not the decl is defined in the
!    current translation unit; it indicates whether or not we should emit the
!    decl at the end of compilation if it is defined and needed.  */
  #define DECL_NOT_REALLY_EXTERN(NODE) \
    (DECL_LANG_SPECIFIC (NODE)->decl_flags.not_really_extern)
  
Index: cp/decl2.c
===================================================================
*** cp/decl2.c	(revision 128844)
--- cp/decl2.c	(working copy)
*************** coerce_delete_type (tree type)
*** 1314,1319 ****
--- 1314,1322 ----
    return type;
  }
  \f
+ /* DECL is a VAR_DECL for a vtable: walk through the entries in the vtable
+    and mark them as needed.  */
+ 
  static void
  mark_vtable_entries (tree decl)
  {
Index: testsuite/g++.dg/ext/visibility/anon6.C
===================================================================
*** testsuite/g++.dg/ext/visibility/anon6.C	(revision 0)
--- testsuite/g++.dg/ext/visibility/anon6.C	(revision 0)
***************
*** 0 ****
--- 1,28 ----
+ // PR c++/33094
+ // { dg-final { scan-assembler "1BIiE1cE" } }
+ // { dg-final { scan-assembler-not "globl.*1BIiE1cE" } }
+ // { dg-final { scan-assembler-not "1CIiE1cE" } }
+ 
+ // Test that B<int>::c is emitted as an internal symbol, and C<int>::c is
+ // not emitted.
+ 
+ namespace
+ {
+   template <typename T>
+   class A
+   {
+     virtual T f1() { return c; }
+     static const T c = 0;
+   };
+ 
+   template <typename T>
+   class B
+   {
+     static const T c = 0;
+   };
+ 
+   template <typename T> const T B<T>::c;
+ 
+   template class A<int>;
+   template class B<int>;
+ }

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Mark Mitchell]

Jason Merrill wrote:

> DECL_EXTERNAL doesn't indicate whether or not something is defined, it
> indicates whether or not we need to call assemble_external when we see a
> reference to it.

Fair enough.  Indeed, the documentation for DECL_EXTERNAL in tree.h
makes clear that DECL_EXTERNAL may be set at the same time that a
definition is available; DECL_EXTERNAL just means that if you reference
the object in this translation unit, you're generating an external
reference.

> DECL_NOT_REALLY_EXTERN also doesn't indicate whether or not something is
> defined, it indicates whether or not we should emit it if we have a
> definition and it is needed.

Right.  I'm sure we could come up with a better name. :-)  I also wonder
whether, in the post-cgraph world, we could simplify a lot of this
handling in the C++ front end.  We used to play these games to make sure
that the definitions didn't get emitted for COMDAT functions until
needed -- but, now, cgraph will presumably do that for us.

So, could we just get rid of DECL_NOT_REALLY_EXTERN, and, instead of
setting that, just clear DECL_EXTERNAL?

In any case,

> Tested x86_64-pc-linux-gnu, applied to trunk.  I'd like to apply this to 4.2 as well; OK, Mark?

please wait until after the 4.2.2 release and then apply this.

(I'm trying to avoid making the mistake I made with 4.2.2 RC1 where I
kept letting patches in until I felt I had to do RC2...)

Thanks,

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

Re: PATCH RFC: PR 33094: Test DECL_INITIAL in make_rtl_for_nonlocal_decl

[Jason Merrill]

Mark Mitchell wrote:
> Jason Merrill wrote:
> 
>> DECL_NOT_REALLY_EXTERN also doesn't indicate whether or not something is
>> defined, it indicates whether or not we should emit it if we have a
>> definition and it is needed.
> 
> Right.  I'm sure we could come up with a better name. :-)

Yes indeedy.

> I also wonder
> whether, in the post-cgraph world, we could simplify a lot of this
> handling in the C++ front end.  We used to play these games to make sure
> that the definitions didn't get emitted for COMDAT functions until
> needed -- but, now, cgraph will presumably do that for us.
> 
> So, could we just get rid of DECL_NOT_REALLY_EXTERN, and, instead of
> setting that, just clear DECL_EXTERNAL?

It does seem like it might be possible to simplify some of this now that 
we have cgraph, but I think we still need to leave DECL_EXTERNAL set on 
vague linkage entities whether or not we're emitting them.

>> Tested x86_64-pc-linux-gnu, applied to trunk.  I'd like to apply this to 4.2 as well; OK, Mark?
> 
> please wait until after the 4.2.2 release and then apply this.
> 
> (I'm trying to avoid making the mistake I made with 4.2.2 RC1 where I
> kept letting patches in until I felt I had to do RC2...)

Well, this patch seems about as trivial as possible, and is for a 4.2 
regression, but OK.

Jason