[C++ PATCH] Fix -frepo (PR c++/36364)

[C++ PATCH] Fix -frepo (PR c++/36364)

[Jakub Jelinek]

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

Hi!

The following testcase distilled from:
#include <string>
#include <ext/mt_allocator.h>

//workaround:
//template class std::basic_string <char, std::char_traits <char>, __gnu_cxx::__mt_alloc <char> >;

int main ()
{
  std::basic_string <char, std::char_traits <char>, __gnu_cxx::__mt_alloc <char> > s;
  s.push_back ('a');
}
doesn't link with -frepo.  The problem is a static const data member with
initializer, which repo_emit_p returns 2 for (to make sure it is properly
optimized out when possible), but then import_export_decl sees the
-frepo implied -fno-implicit-templates and doesn't emit it at all.

Saying this is invalid and requires user to explicitly instantiate it is
difficult, as that IMHO doesn't play well with the -frepo model.

Attached are two alternative patches.  One stops implying
-fno-implicit-templates for -frepo in c-opts.c, only implies that
in import_export_class and in import_export_decl for VTABLE/VTT, the other in
the second spot in import_export_decl basically implies -fimplicit-templates
for the cases where repo_emit_p returned 2.
The first one will require explicit instantiation for terminal
if -frepo -fno-implicit-templates, the second one will not, otherwise
they are the same.

The second line in
      if ((flag_implicit_templates
          && !flag_use_repository)
I've added in December last year doesn't make sense, in current
SVN flag_use_repository != 0 implies flag_implicit_templates = 0.

Is this ok for 4.4/4.3, or do you prefer doing something else?

	Jakub

[-- Attachment #2: Z197 --]
[-- Type: text/plain, Size: 3246 bytes --]

2008-06-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36364
	* c-opts.c (c_common_handle_option): Don't clear
	flag_implicit_templates for -frepo.

	* decl2.c (import_export_class): Adjust for -frepo
	not implying flag_implicit_templates = 0.
	(import_export_decl): Likewise.

	* g++.dg/template/repo9.C: New test.

--- gcc/c-opts.c.jj	2008-06-19 12:22:29.000000000 +0200
+++ gcc/c-opts.c	2008-06-23 16:44:10.000000000 +0200
@@ -755,8 +755,6 @@ c_common_handle_option (size_t scode, co
 
     case OPT_frepo:
       flag_use_repository = value;
-      if (value)
-	flag_implicit_templates = 0;
       break;
 
     case OPT_frtti:
--- gcc/cp/decl2.c.jj	2008-06-23 08:40:29.000000000 +0200
+++ gcc/cp/decl2.c	2008-06-23 16:45:41.000000000 +0200
@@ -1589,7 +1589,7 @@ import_export_class (tree ctype)
   else if (lookup_attribute ("dllexport", TYPE_ATTRIBUTES (ctype)))
     import_export = 1;
   else if (CLASSTYPE_IMPLICIT_INSTANTIATION (ctype)
-	   && !flag_implicit_templates)
+	   && (!flag_implicit_templates || flag_use_repository))
     /* For a template class, without -fimplicit-templates, check the
        repository.  If the virtual table is assigned to this
        translation unit, then export the class; otherwise, import
@@ -2282,7 +2282,7 @@ import_export_decl (tree decl)
 		}
 	    }
 	}
-      else if (!flag_implicit_templates
+      else if ((!flag_implicit_templates || flag_use_repository)
 	       && CLASSTYPE_IMPLICIT_INSTANTIATION (class_type))
 	import_p = true;
       else
@@ -2331,8 +2331,7 @@ import_export_decl (tree decl)
     {
       /* DECL is an implicit instantiation of a function or static
 	 data member.  */
-      if ((flag_implicit_templates
-	   && !flag_use_repository)
+      if (flag_implicit_templates
 	  || (flag_implicit_inline_templates
 	      && TREE_CODE (decl) == FUNCTION_DECL
 	      && DECL_DECLARED_INLINE_P (decl)))
--- gcc/testsuite/g++.dg/template/repo9.C.jj	2008-06-23 16:54:25.000000000 +0200
+++ gcc/testsuite/g++.dg/template/repo9.C	2008-06-23 16:54:14.000000000 +0200
@@ -0,0 +1,48 @@
+// PR c++/36364
+// { dg-options "-frepo" }
+// { dg-final { cleanup-repo-files } }
+// { dg-require-host-local "" }
+
+template <typename C> struct A
+{
+  static void assign (C &c1, const C &c2) { c1 = c2; }
+};
+
+template <typename C, typename T> struct B
+{
+  struct D
+  {
+    static const C terminal;
+    static unsigned long stor[];
+    static D &empty_rep ()
+    {
+      void *p = reinterpret_cast <void *>(&stor);
+      return *reinterpret_cast <D *>(p);
+    }
+    void test (unsigned long n)
+    {
+      T::assign (this->refdata ()[n], terminal);
+    }
+    C *refdata () throw ()
+    {
+      return reinterpret_cast <C *>(this + 1);
+    }
+  };
+  C *dataplus;
+  C *data () const { return dataplus; }
+  D *rep () const { return &((reinterpret_cast < D * >(data ()))[-1]); }
+  static D & empty_rep () { return D::empty_rep (); }
+  B () : dataplus (empty_rep ().refdata ()) { }
+  ~B () { }
+  void push_back (C c) { rep ()->test (10); }
+};
+
+template <typename C, typename T> const C B <C, T>::D::terminal = C ();
+template <typename C, typename T> unsigned long B <C, T>::D::stor[64];
+
+int
+main ()
+{
+  B <char, A <char> > s;
+  s.push_back ('a');
+}

[-- Attachment #3: Z197a --]
[-- Type: text/plain, Size: 2045 bytes --]

2008-06-23  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36364
	* decl2.c (import_export_decl): Set comdat_p whenever -frepo.

	* g++.dg/template/repo9.C: New test.

--- gcc/cp/decl2.c.jj	2008-06-23 08:40:29.000000000 +0200
+++ gcc/cp/decl2.c	2008-06-23 17:33:53.000000000 +0200
@@ -2331,8 +2331,8 @@ import_export_decl (tree decl)
     {
       /* DECL is an implicit instantiation of a function or static
 	 data member.  */
-      if ((flag_implicit_templates
-	   && !flag_use_repository)
+      if (flag_implicit_templates
+	  || flag_use_repository
 	  || (flag_implicit_inline_templates
 	      && TREE_CODE (decl) == FUNCTION_DECL
 	      && DECL_DECLARED_INLINE_P (decl)))
--- gcc/testsuite/g++.dg/template/repo9.C.jj	2008-06-23 16:54:25.000000000 +0200
+++ gcc/testsuite/g++.dg/template/repo9.C	2008-06-23 16:54:14.000000000 +0200
@@ -0,0 +1,48 @@
+// PR c++/36364
+// { dg-options "-frepo" }
+// { dg-final { cleanup-repo-files } }
+// { dg-require-host-local "" }
+
+template <typename C> struct A
+{
+  static void assign (C &c1, const C &c2) { c1 = c2; }
+};
+
+template <typename C, typename T> struct B
+{
+  struct D
+  {
+    static const C terminal;
+    static unsigned long stor[];
+    static D &empty_rep ()
+    {
+      void *p = reinterpret_cast <void *>(&stor);
+      return *reinterpret_cast <D *>(p);
+    }
+    void test (unsigned long n)
+    {
+      T::assign (this->refdata ()[n], terminal);
+    }
+    C *refdata () throw ()
+    {
+      return reinterpret_cast <C *>(this + 1);
+    }
+  };
+  C *dataplus;
+  C *data () const { return dataplus; }
+  D *rep () const { return &((reinterpret_cast < D * >(data ()))[-1]); }
+  static D & empty_rep () { return D::empty_rep (); }
+  B () : dataplus (empty_rep ().refdata ()) { }
+  ~B () { }
+  void push_back (C c) { rep ()->test (10); }
+};
+
+template <typename C, typename T> const C B <C, T>::D::terminal = C ();
+template <typename C, typename T> unsigned long B <C, T>::D::stor[64];
+
+int
+main ()
+{
+  B <char, A <char> > s;
+  s.push_back ('a');
+}

Re: [C++ PATCH] Fix -frepo (PR c++/36364)

[Mark Mitchell]

Jakub Jelinek wrote:

>   std::basic_string <char, std::char_traits <char>, __gnu_cxx::__mt_alloc <char> > s;

> doesn't link with -frepo.  The problem is a static const data member with
> initializer, which repo_emit_p returns 2 for (to make sure it is properly
> optimized out when possible), but then import_export_decl sees the
> -frepo implied -fno-implicit-templates and doesn't emit it at all.

If I understand correctly, the problem is that we have a static data 
member of a template which is marked as required by collect2, but then 
when we recompile the file, is not actually emitted.  Is that right?

If so, I think the fix is to honor IDENTIFIER_REPO_CHOSEN for the 
VAR_DECL case of a static data member initialized by an integral 
constant expression.  Change the code in this test:

       if (DECL_INTEGRAL_CONSTANT_VAR_P (decl)
           && DECL_CLASS_SCOPE_P (decl))

to (1) add the variable to pending_repo, and (2) if 
IDENTIFIER_REPO_CHOSEN return 1, otherwise return 2.  Probably the:

   if (!DECL_REPO_AVAILABLE...)

block should be factored into a helper function.

Does that work?

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

Re: [C++ PATCH] Fix -frepo (PR c++/36364)

[Jakub Jelinek]

On Tue, Jun 24, 2008 at 02:03:37PM -0700, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> >  std::basic_string <char, std::char_traits <char>, __gnu_cxx::__mt_alloc 
> >  <char> > s;
> 
> >doesn't link with -frepo.  The problem is a static const data member with
> >initializer, which repo_emit_p returns 2 for (to make sure it is properly
> >optimized out when possible), but then import_export_decl sees the
> >-frepo implied -fno-implicit-templates and doesn't emit it at all.
> 
> If I understand correctly, the problem is that we have a static data 
> member of a template which is marked as required by collect2, but then 
> when we recompile the file, is not actually emitted.  Is that right?

No.  The problem is const static data member, for which:
      /* Const static data members initialized by constant expressions must
         be processed where needed so that their definitions are
         available.  */
      if (DECL_INTEGRAL_CONSTANT_VAR_P (decl)
          && DECL_CLASS_SCOPE_P (decl))
        return 2;
Returning 2 from repo_emit_p means thatg it is handled as in -fno-repo
compilation - it never makes it into the *.rpo file, neither
as O _Z...terminal, nor as C _Z...terminal.  But as -frepo implies
-fno-implicit-templates, in this case it works as in -fno-implicit-templates
compilation - which means explicit instantiation is required.

	Jakub

Re: [C++ PATCH] Fix -frepo (PR c++/36364)

[Mark Mitchell]

Jakub Jelinek wrote:

> No.  The problem is const static data member, for which:
>       /* Const static data members initialized by constant expressions must
>          be processed where needed so that their definitions are
>          available.  */
>       if (DECL_INTEGRAL_CONSTANT_VAR_P (decl)
>           && DECL_CLASS_SCOPE_P (decl))
>         return 2;
> Returning 2 from repo_emit_p means thatg it is handled as in -fno-repo
> compilation - it never makes it into the *.rpo file, neither
> as O _Z...terminal, nor as C _Z...terminal.  But as -frepo implies
> -fno-implicit-templates, in this case it works as in -fno-implicit-templates
> compilation - which means explicit instantiation is required.

Right, that is why I suggested using both the DECL_REPO_AVAILABLE and 
the IDENTIFIER_REPO_CHOSEN logic.  Of course, you have to use 
DECL_REPO_AVAILABLE to let collect2 know about it.  But, once you do 
that, it should assign it to an object file, and then 
IDENTIFIER_REPO_CHOSEN will tell you to write it out by returning 1 from 
that function.

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

[C++ PATCH] Fix -frepo (PR c++/36364, take 2)

[Jakub Jelinek]

On Tue, Jun 24, 2008 at 03:53:46PM -0700, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> >No.  The problem is const static data member, for which:
> >      /* Const static data members initialized by constant expressions must
> >         be processed where needed so that their definitions are
> >         available.  */
> >      if (DECL_INTEGRAL_CONSTANT_VAR_P (decl)
> >          && DECL_CLASS_SCOPE_P (decl))
> >        return 2;
> >Returning 2 from repo_emit_p means thatg it is handled as in -fno-repo
> >compilation - it never makes it into the *.rpo file, neither
> >as O _Z...terminal, nor as C _Z...terminal.  But as -frepo implies
> >-fno-implicit-templates, in this case it works as in 
> >-fno-implicit-templates
> >compilation - which means explicit instantiation is required.
> 
> Right, that is why I suggested using both the DECL_REPO_AVAILABLE and 
> the IDENTIFIER_REPO_CHOSEN logic.  Of course, you have to use 
> DECL_REPO_AVAILABLE to let collect2 know about it.  But, once you do 
> that, it should assign it to an object file, and then 
> IDENTIFIER_REPO_CHOSEN will tell you to write it out by returning 1 from 
> that function.

Did you mean something like this, or have I completely misunderstood?

Works on RUNTESTFLAGS=dg.exp=repo*.C, will run a full x86_64-linux
bootstrap/regtest tonight.

Ok for trunk/4.3?

2008-06-27  Jakub Jelinek  <jakub@redhat.com>

	PR c++/36364
	* repo.c (repo_emit_p): Put const static data members initialized
	by const expr into *.rpo file, just return 2 if IDENTIFIER_REPO_CHOSEN
	for it is 0.

	* g++.dg/template/repo9.C: New test.

--- gcc/cp/repo.c.jj	2008-02-11 14:45:57.000000000 +0100
+++ gcc/cp/repo.c	2008-06-27 17:16:23.000000000 +0200
@@ -280,6 +280,7 @@ finish_repo (void)
 int
 repo_emit_p (tree decl)
 {
+  int ret = 0;
   gcc_assert (TREE_PUBLIC (decl));
   gcc_assert (TREE_CODE (decl) == FUNCTION_DECL
 	      || TREE_CODE (decl) == VAR_DECL);
@@ -306,10 +307,12 @@ repo_emit_p (tree decl)
 	return 2;
       /* Const static data members initialized by constant expressions must
 	 be processed where needed so that their definitions are
-	 available.  */
+	 available.  Still record them into *.rpo files, so if they
+	 weren't actually emitted and collect2 requests them, they can
+	 be provided.  */
       if (DECL_INTEGRAL_CONSTANT_VAR_P (decl)
 	  && DECL_CLASS_SCOPE_P (decl))
-	return 2;
+	ret = 2;
     }
   else if (!DECL_TEMPLATE_INSTANTIATION (decl))
     return 2;
@@ -343,7 +346,7 @@ repo_emit_p (tree decl)
       pending_repo = tree_cons (NULL_TREE, decl, pending_repo);
     }
 
-  return IDENTIFIER_REPO_CHOSEN (DECL_ASSEMBLER_NAME (decl));
+  return IDENTIFIER_REPO_CHOSEN (DECL_ASSEMBLER_NAME (decl)) ? 1 : ret;
 }
 
 /* Returns true iff the prelinker has explicitly marked CLASS_TYPE for
--- gcc/testsuite/g++.dg/template/repo9.C.jj	2008-06-27 16:56:42.000000000 +0200
+++ gcc/testsuite/g++.dg/template/repo9.C	2008-06-27 16:56:42.000000000 +0200
@@ -0,0 +1,48 @@
+// PR c++/36364
+// { dg-options "-frepo" }
+// { dg-final { cleanup-repo-files } }
+// { dg-require-host-local "" }
+
+template <typename C> struct A
+{
+  static void assign (C &c1, const C &c2) { c1 = c2; }
+};
+
+template <typename C, typename T> struct B
+{
+  struct D
+  {
+    static const C terminal;
+    static unsigned long stor[];
+    static D &empty_rep ()
+    {
+      void *p = reinterpret_cast <void *>(&stor);
+      return *reinterpret_cast <D *>(p);
+    }
+    void test (unsigned long n)
+    {
+      T::assign (this->refdata ()[n], terminal);
+    }
+    C *refdata () throw ()
+    {
+      return reinterpret_cast <C *>(this + 1);
+    }
+  };
+  C *dataplus;
+  C *data () const { return dataplus; }
+  D *rep () const { return &((reinterpret_cast < D * >(data ()))[-1]); }
+  static D & empty_rep () { return D::empty_rep (); }
+  B () : dataplus (empty_rep ().refdata ()) { }
+  ~B () { }
+  void push_back (C c) { rep ()->test (10); }
+};
+
+template <typename C, typename T> const C B <C, T>::D::terminal = C ();
+template <typename C, typename T> unsigned long B <C, T>::D::stor[64];
+
+int
+main ()
+{
+  B <char, A <char> > s;
+  s.push_back ('a');
+}


	Jakub

Re: [C++ PATCH] Fix -frepo (PR c++/36364, take 2)

[Mark Mitchell]

Jakub Jelinek wrote:

> Did you mean something like this, or have I completely misunderstood?

No, that's exactly what I meant.

> Works on RUNTESTFLAGS=dg.exp=repo*.C, will run a full x86_64-linux
> bootstrap/regtest tonight.
> 
> Ok for trunk/4.3?

OK, assuming testing shows no new failures.

> 2008-06-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/36364
> 	* repo.c (repo_emit_p): Put const static data members initialized
> 	by const expr into *.rpo file, just return 2 if IDENTIFIER_REPO_CHOSEN
> 	for it is 0.
> 
> 	* g++.dg/template/repo9.C: New test.

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