Patchwork fix PR51910, take 2

login
register
mail settings
Submitter Jason Merrill
Date Feb. 8, 2012, 9:41 a.m.
Message ID <4F32432F.4040503@redhat.com>
Download mbox | patch
Permalink /patch/140098/
State New
Headers show

Comments

Jason Merrill - Feb. 8, 2012, 9:41 a.m.
On 02/02/2012 11:27 PM, Jakub Jelinek wrote:
> Anyway, here is the tweaking all the symbols that demangle the same
> equally patch (untested so far) as an alternative.  On the example it
> unfortunately causes also the D2 dtor into the link which wasn't there
> otherwise (and with -Wl,--no-demangle).  While D2 dtors are generally small,
> perhaps it will sometimes force into the link also ctor or dtor variants
> that couldn't be aliased together.

Hmm, I wrote up something quite similar on the plane.  One difference is 
that mine synchronizes .rpo files that start with some variants chosen 
and others not.  Does this make sense to you?

> I wonder if it wouldn't be better to just use a linker plugin for -frepo
> if the linker supports plugins.

Probably.  But I don't feel motivated to write it.

Jason
Jakub Jelinek - Feb. 8, 2012, 1:01 p.m.
On Tue, Feb 07, 2012 at 11:41:03PM -1000, Jason Merrill wrote:
> Hmm, I wrote up something quite similar on the plane.  One
> difference is that mine synchronizes .rpo files that start with some
> variants chosen and others not.  Does this make sense to you?

You mean the maybe_tweak change?  Not sure I understand it.

Anyway, using VEC is probably cleaner than my patch, on the other
side might be more expensive for the common case where most
symbols don't demangle the same like any other symbol,
because then you allocate a VEC (I think 4 pointers) for each of the
hash table entries, i.e. one extra allocation for each.
> 
> >I wonder if it wouldn't be better to just use a linker plugin for -frepo
> >if the linker supports plugins.
> 
> Probably.  But I don't feel motivated to write it.

Me neither ;).
Anyway, the question is if the increases in object sizes with -frepo
(due to bringing in unused ctor or dtor variants from time to time)
will be acceptable to -frepo users (if there are any).

	Jakub
Jason Merrill - Feb. 11, 2012, 8:41 a.m.
On 02/08/2012 03:01 AM, Jakub Jelinek wrote:
> On Tue, Feb 07, 2012 at 11:41:03PM -1000, Jason Merrill wrote:
>> Hmm, I wrote up something quite similar on the plane.  One
>> difference is that mine synchronizes .rpo files that start with some
>> variants chosen and others not.  Does this make sense to you?
>
> You mean the maybe_tweak change?  Not sure I understand it.

That and some of the changes in scan_linker_output as well.

> Anyway, using VEC is probably cleaner than my patch, on the other
> side might be more expensive for the common case where most
> symbols don't demangle the same like any other symbol,
> because then you allocate a VEC (I think 4 pointers) for each of the
> hash table entries, i.e. one extra allocation for each.

It looks like a VEC adds two unsigned ints, whereas your patch adds one 
pointer; not a big difference.

> Anyway, the question is if the increases in object sizes with -frepo
> (due to bringing in unused ctor or dtor variants from time to time)
> will be acceptable to -frepo users (if there are any).

I suppose we would run into this with classes with virtual bases that 
are not themselves derived from; we wouldn't need the base variant in 
that case.  So this changes from a link-failure regression to, I 
suppose, missed-optimization.  I'm OK with that.

Jason

Patch

commit 1f4b0d273cfd81948762d3922cdafac40a06a2cf
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Feb 2 00:07:44 2012 -0500

    	PR c++/51910
    	* tlink.c (demangled_hash_entry): Change mangled to a VEC.
    	(demangle_new_symbols): Fill it.
    	(scan_linker_output): Walk it.
    	(start_tweaking): Split out from scan_linker_output.
    	(maybe_tweak): Update sym->chosen.
    	* Makefile.in (COLLECT2_OBJS): Add vec.o and gcc-none.o

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index c9ecc4b..486538d 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1946,7 +1946,7 @@  gcc-ranlib.c: gcc-ar.c
 gcc-nm.c: gcc-ar.c
 	cp $^ $@
 
-COLLECT2_OBJS = collect2.o collect2-aix.o tlink.o
+COLLECT2_OBJS = collect2.o collect2-aix.o tlink.o vec.o ggc-none.o
 COLLECT2_LIBS = @COLLECT2_LIBS@
 collect2$(exeext): $(COLLECT2_OBJS) $(LIBDEPS)
 # Don't try modifying collect2 (aka ld) in place--it might be linking this.
diff --git a/gcc/testsuite/g++.dg/template/repo10.C b/gcc/testsuite/g++.dg/template/repo10.C
new file mode 100644
index 0000000..c92f7a5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/repo10.C
@@ -0,0 +1,16 @@ 
+// PR c++/51910
+// { dg-options -frepo }
+// { dg-require-host-local "" }
+// { dg-skip-if "dkms are not final links" { vxworks_kernel } }
+// { dg-final cleanup-repo-files }
+
+template<typename T>
+struct Foo
+{
+  virtual ~Foo() { }
+};
+
+int main( int, char*[] )
+{
+  Foo<int> test;
+}
diff --git a/gcc/tlink.c b/gcc/tlink.c
index f054047..67c7086 100644
--- a/gcc/tlink.c
+++ b/gcc/tlink.c
@@ -32,6 +32,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "collect2.h"
 #include "filenames.h"
 #include "diagnostic-core.h"
+#include "vec.h"
 
 /* TARGET_64BIT may be defined to use driver specific functionality. */
 #undef TARGET_64BIT
@@ -67,10 +68,14 @@  typedef struct file_hash_entry
   int tweaking;
 } file;
 
+typedef const char *str;
+DEF_VEC_P(str);
+DEF_VEC_ALLOC_P(str,heap);
+
 typedef struct demangled_hash_entry
 {
   const char *key;
-  const char *mangled;
+  VEC(str,heap) *mangled;
 } demangled;
 
 /* Hash and comparison functions for these hash tables.  */
@@ -435,9 +440,15 @@  maybe_tweak (char *line, file *f)
       sym->tweaked = 1;
 
       if (line[0] == 'O')
-	line[0] = 'C';
+	{
+	  line[0] = 'C';
+	  sym->chosen = 1;
+	}
       else
-	line[0] = 'O';
+	{
+	  line[0] = 'O';
+	  sym->chosen = 0;
+	}
     }
 }
 
@@ -598,10 +609,34 @@  demangle_new_symbols (void)
 	continue;
 
       dem = demangled_hash_lookup (p, true);
-      dem->mangled = sym->key;
+      VEC_safe_push (str, heap, dem->mangled, sym->key);
     }
 }
 
+/* We want to tweak symbol SYM.  Return true if all is well, false on
+   error.  */
+
+static bool
+start_tweaking (symbol *sym)
+{
+  if (sym && sym->tweaked)
+    {
+      error ("'%s' was assigned to '%s', but was not defined "
+	     "during recompilation, or vice versa",
+	     sym->key, sym->file->key);
+      return 0;
+    }
+  if (sym && !sym->tweaking)
+    {
+      if (tlink_verbose >= 2)
+	fprintf (stderr, _("collect: tweaking %s in %s\n"),
+		 sym->key, sym->file->key);
+      sym->tweaking = 1;
+      file_push (sym->file);
+    }
+  return true;
+}
+
 /* Step through the output of the linker, in the file named FNAME, and
    adjust the settings for each symbol encountered.  */
 
@@ -616,8 +651,11 @@  scan_linker_output (const char *fname)
     {
       char *p = line, *q;
       symbol *sym;
+      demangled *dem = 0;
       int end;
       int ok = 0;
+      unsigned ix;
+      str s;
 
       /* On darwin9, we might have to skip " in " lines as well.  */
       if (skip_next_in_line
@@ -662,7 +700,6 @@  scan_linker_output (const char *fname)
 	/* Try a mangled name in quotes.  */
 	{
 	  char *oldq = q + 1;
-	  demangled *dem = 0;
 	  q = 0;
 
 	  /* On darwin9, we look for "foo" referenced from:\n\(.* in .*\n\)*  */
@@ -718,9 +755,7 @@  scan_linker_output (const char *fname)
 	    {
 	      *q = 0;
 	      dem = demangled_hash_lookup (p, false);
-	      if (dem)
-		sym = symbol_hash_lookup (dem->mangled, false);
-	      else
+	      if (!dem)
 		{
 		  if (!strncmp (p, USER_LABEL_PREFIX,
 				strlen (USER_LABEL_PREFIX)))
@@ -730,24 +765,43 @@  scan_linker_output (const char *fname)
 	    }
 	}
 
-      if (sym && sym->tweaked)
+      if (dem)
+	{
+	  /* We found a demangled name.  If this is the name of a
+	     constructor or destructor, there can be several mangled names
+	     that match it, so choose or unchoose all of them.  If some are
+	     chosen and some not, leave the later ones that don't match
+	     alone for now; either this will cause the link to suceed, or
+	     on the next attempt we will switch all of them the other way
+	     and that will cause it to succeed.  */
+	  int chosen = 0;
+	  int len = VEC_length (str, dem->mangled);
+	  ok = true;
+	  FOR_EACH_VEC_ELT (str, dem->mangled, ix, s)
+	    {
+	      sym = symbol_hash_lookup (s, false);
+	      if (ix == 0)
+		chosen = sym->chosen;
+	      else if (sym->chosen != chosen)
+		/* Mismatch.  */
+		continue;
+	      /* Avoid an error about re-tweaking when we guess wrong in
+		 the case of mismatch.  */
+	      if (len > 1)
+		sym->tweaked = false;
+	      ok = start_tweaking (sym);
+	    }
+	}
+      else
+	ok = start_tweaking (sym);
+
+      obstack_free (&temporary_obstack, temporary_firstobj);
+
+      if (!ok)
 	{
-	  error ("'%s' was assigned to '%s', but was not defined "
-		 "during recompilation, or vice versa",
-		 sym->key, sym->file->key);
 	  fclose (stream);
 	  return 0;
 	}
-      if (sym && !sym->tweaking)
-	{
-	  if (tlink_verbose >= 2)
-	    fprintf (stderr, _("collect: tweaking %s in %s\n"),
-		     sym->key, sym->file->key);
-	  sym->tweaking = 1;
-	  file_push (sym->file);
-	}
-
-      obstack_free (&temporary_obstack, temporary_firstobj);
     }
 
   fclose (stream);