gold patch: Patch resolution of NAME/NULL and NAME/VERSION

[Ian Lance Taylor <iant at google dot com>]

It turns out that gold failed to support symbol overriding in the case
where the main executable defines SYM, it is linked against a shared
library which refers to SYM/VERSION, and a subsequent shared library
which defines SYM/VERSION as the default version.  In this case, gold
would not put SYM in the dynamic symbol table, so the shared libraries
would continue to use the version from the shared library rather than
letting the executable override.

In particular, this happened when a C++ program provided a definition
of malloc, and was linked against libstdc++ which has an undefined
reference to malloc/GLIBC_2.0, and then against libc which has a
defined symbol malloc/GLIBC_2.0.  In this case gold failed to put
malloc into the dynamic symbol table, and thus did not override the
version of malloc used by libstdc++.

This is a case of seeing NAME/NULL and NAME/VERSION as separate
symbols, and then having to resolve them together.  I expanded the
handling of this, working through a few different cases with the help
of the testsuite.  I added a test case for the original problem.  This
is committed to CVS.

Ian


2008-07-18  Ian Lance Taylor  <iant@google.com>

	* symtab.cc (Symbol_table::add_from_object): Rewrite the case
	where we see NAME/NULL and NAME/VERSION  as separate symbols.
	* testsuite/ver_test_main.cc (main): Call t4.
	(t4, t4_2a): Define.
	* testsuite/ver_test_2.cc (t4_2): Define.
	* testsuite/ver_test_2.script: Put t4_2a in VER2.
	* testsuite/ver_test_4.cc (t4_2a): Define.
	* testsuite/ver_test_4.script: Put t4_2a in VER2.
	* testsuite/ver_test.h (t4, t4_2, t4_2a): Declare.


Index: symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.100
diff -p -u -r1.100 symtab.cc
--- symtab.cc	10 Jul 2008 23:01:20 -0000	1.100
+++ symtab.cc	18 Jul 2008 06:51:14 -0000
@@ -687,23 +687,58 @@ Symbol_table::add_from_object(Object* ob
 	      // NAME/NULL point to NAME/VERSION.
 	      insdef.first->second = ret;
 	    }
-	  else if (insdef.first->second != ret
-	           && insdef.first->second->is_undefined())
+	  else if (insdef.first->second != ret)
 	    {
 	      // This is the unfortunate case where we already have
-	      // entries for both NAME/VERSION and NAME/NULL.  Note
-	      // that we don't want to combine them if the existing
-	      // symbol is going to override the new one.  FIXME: We
-	      // currently just test is_undefined, but this may not do
-	      // the right thing if the existing symbol is from a
-	      // shared library and the new one is from a regular
-	      // object.
-
-	      const Sized_symbol<size>* sym2;
-	      sym2 = this->get_sized_symbol<size>(insdef.first->second);
-	      Symbol_table::resolve<size, big_endian>(ret, sym2, version);
-	      this->make_forwarder(insdef.first->second, ret);
-	      insdef.first->second = ret;
+	      // entries for both NAME/VERSION and NAME/NULL.  We now
+	      // see a symbol NAME/VERSION where VERSION is the
+	      // default version.  We have already resolved this new
+	      // symbol with the existing NAME/VERSION symbol.
+
+	      // It's possible that NAME/NULL and NAME/VERSION are
+	      // both defined in regular objects.  This can only
+	      // happen if one object file defines foo and another
+	      // defines foo@@ver.  This is somewhat obscure, but we
+	      // call it a multiple definition error.
+
+	      // It's possible that NAME/NULL actually has a version,
+	      // in which case it won't be the same as VERSION.  This
+	      // happens with ver_test_7.so in the testsuite for the
+	      // symbol t2_2.  We see t2_2@@VER2, so we define both
+	      // t2_2/VER2 and t2_2/NULL.  We then see an unadorned
+	      // t2_2 in an object file and give it version VER1 from
+	      // the version script.  This looks like a default
+	      // definition for VER1, so it looks like we should merge
+	      // t2_2/NULL with t2_2/VER1.  That doesn't make sense,
+	      // but it's not obvious that this is an error, either.
+	      // So we just punt.
+
+	      // If one of the symbols has non-default visibility, and
+	      // the other is defined in a shared object, then they
+	      // are different symbols.
+
+	      // Otherwise, we just resolve the symbols as though they
+	      // were the same.
+
+	      if (insdef.first->second->version() != NULL)
+		{
+		  gold_assert(insdef.first->second->version() != version);
+		  def = false;
+		}
+	      else if (ret->visibility() != elfcpp::STV_DEFAULT
+		  && insdef.first->second->is_from_dynobj())
+		def = false;
+	      else if (insdef.first->second->visibility() != elfcpp::STV_DEFAULT
+		       && ret->is_from_dynobj())
+		def = false;
+	      else
+		{
+		  const Sized_symbol<size>* sym2;
+		  sym2 = this->get_sized_symbol<size>(insdef.first->second);
+		  Symbol_table::resolve<size, big_endian>(ret, sym2, version);
+		  this->make_forwarder(insdef.first->second, ret);
+		  insdef.first->second = ret;
+		}
 	    }
 	  else
 	    def = false;
Index: testsuite/ver_test.h
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test.h,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test.h
--- testsuite/ver_test.h	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test.h	18 Jul 2008 06:51:14 -0000
@@ -30,11 +30,14 @@
 extern bool t1();
 extern bool t2();
 extern bool t3();
+extern bool t4();
 
 extern "C" {
 
 extern int t1_2();
 extern int t2_2();
 extern int t3_2();
+extern int t4_2();
+extern int t4_2a();
 
 }
Index: testsuite/ver_test_2.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test_2.cc,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test_2.cc
--- testsuite/ver_test_2.cc	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test_2.cc	18 Jul 2008 06:51:14 -0000
@@ -28,3 +28,13 @@ t3_2()
   TRACE
   return t1_2();
 }
+
+// Calls a versioned function in ver_test_4.cc which should be
+// overridden by an unversioned function in the main program.
+
+int
+t4_2()
+{
+  TRACE
+  return t4_2a();
+}
Index: testsuite/ver_test_2.script
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test_2.script,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test_2.script
--- testsuite/ver_test_2.script	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test_2.script	18 Jul 2008 06:51:14 -0000
@@ -27,5 +27,5 @@ VER2 {
   global:
     t1_2;
     t3_2;
+    t4_2a;
 } VER1;
-
Index: testsuite/ver_test_4.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test_4.cc,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test_4.cc
--- testsuite/ver_test_4.cc	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test_4.cc	18 Jul 2008 06:51:14 -0000
@@ -51,3 +51,14 @@ t2_2_b()
   TRACE
   return 22;
 }
+
+
+// This function is given a version by the version script, and should
+// be overridden by the main program.
+
+int
+t4_2a()
+{
+  TRACE
+  return -42;
+}
Index: testsuite/ver_test_4.script
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test_4.script,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test_4.script
--- testsuite/ver_test_4.script	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test_4.script	18 Jul 2008 06:51:14 -0000
@@ -31,5 +31,6 @@ VER2 {
   global:
     t1_2;
     t2_2;
+    t4_2a;
 } VER1;
 
Index: testsuite/ver_test_main.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/ver_test_main.cc,v
retrieving revision 1.3
diff -p -u -r1.3 ver_test_main.cc
--- testsuite/ver_test_main.cc	13 Mar 2008 21:04:21 -0000	1.3
+++ testsuite/ver_test_main.cc	18 Jul 2008 06:51:14 -0000
@@ -30,6 +30,7 @@ main()
   assert(t1());
   assert(t2());
   assert(t3());
+  assert(t4());
   return 0;
 }
 
@@ -52,3 +53,22 @@ t3()
   TRACE
   return t3_2() == 12;
 }
+
+// Call a function in a shared library that calls a function which is
+// defined in the main program and defined with a default version in
+// the shared library.  The symbol in the main program should override
+// even though it doesn't have a version.
+
+bool
+t4()
+{
+  TRACE
+  return t4_2() == 42;
+}
+
+int
+t4_2a()
+{
+  TRACE
+  return 42;
+}