[gold patch] PR 16504: Fix internal error caused by conflicting default version definitions

[ccoutant at gmail dot com]

Ian,

This patch fixes a couple of problems with symbol versioning in gold,
but I'd like to get an extra pair eyes on it before I check it in,
since the expected behavior in some of these cases is (to my mind)
quite subtle.

The problem that affects this PR directly is when we have a
default version definition (set by a version script) for a
symbol, foo@@V2, in an LTO object, along with an external reference
to a different version, foo@V1, of that same symbol. We then link
that object against a shared library that provides a default
definition of foo@@V1. When we see the definition of foo@@V1 in the
shared library, we already have foo@@V2 in the symbol table, and we
resolve (incorrectly) foo@@V1 to foo@@V2. We don't actually see the
external reference to foo@V2 until the replacement file is provided,
and then try to resolve it to foo@@V1. At that point, we hit an
internal error in Symbol::override_version() because of
the conflicting version name.

I've fixed this by making sure that we enter foo@@V2 from the
shared library as a new (but not default) symbol, so that when
we later see the external reference to foo@V2, it resolves to
that symbol instead of foo@@V1. I only do this if the new symbol
is coming from a shared library, and if the original symbol
already has a (non-empty) version. If we see a conflicting
default version from a regular object, we give a warning --
or should it be an error? -- Gnu ld simply prints a multiple
definition error for that case.

During my investigation of this bug, I also found that gold is
setting the VERSYM_HIDDEN bit in cases where Gnu ld is not. I've 
changed gold so that it sets the HIDDEN bit only for definitions,
and not for UNDEFs.

The test case provided with the PR leaves me with a nervous
feeling that neither IFUNCs nor .symver directives are a good
idea when using LTO. For .symver at least, the compiler doesn't
see the real symbol name (it's just an asm pass-thru), so it
could be missing some important linkage information. It could
theoretically figure out how the IFUNCs work by looking at the
resolve function, but I'm not sure it does.

-cary


gold/
	PR gold/16504
	* dynobj.cc (Versions::symbol_section_contents): Don't set
	VERSYM_HIDDEN flag for undefined symbols.
	* symtab.cc (Symbol_table::add_from_object): Don't override default
	version definition with a different default version.
	* symtab.h (Symbol::from_dyn): New method.
	* testsuite/plugin_test.c (struct sym_info): Add ver field.
	(claim_file_hook): Pass symbol version to plugin API.
	(parse_readelf_line): Parse symbol version.
	* testsuite/Makefile.am (ver_test_pr16504): New test case.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/ver_test_pr16504.sh: New test script.
	* testsuite/ver_test_pr16504_a.c: New source file.
	* testsuite/ver_test_pr16504_a.script: New version script.
	* testsuite/ver_test_pr16504_b.c: New source file.
	* testsuite/ver_test_pr16504_b.script: New version script.


diff --git a/gold/dynobj.cc b/gold/dynobj.cc
index d85adbcbe1..7012802d4f 100644
--- a/gold/dynobj.cc
+++ b/gold/dynobj.cc
@@ -1776,7 +1776,10 @@ Versions::symbol_section_contents(const Symbol_table* symtab,
 	version_index = this->version_index(symtab, dynpool, *p);
       // If the symbol was defined as foo@V1 instead of foo@@V1, add
       // the hidden bit.
-      if ((*p)->version() != NULL && !(*p)->is_default())
+      if ((*p)->version() != NULL
+	  && (*p)->is_defined()
+	  && !(*p)->is_default()
+	  && !(*p)->from_dyn())
         version_index |= elfcpp::VERSYM_HIDDEN;
       elfcpp::Swap<16, big_endian>::writeval(pbuf + (*p)->dynsym_index() * 2,
                                              version_index);
diff --git a/gold/symtab.cc b/gold/symtab.cc
index 34551ace7d..238834dce8 100644
--- a/gold/symtab.cc
+++ b/gold/symtab.cc
@@ -989,7 +989,7 @@ Symbol_table::add_from_object(Object* object,
   // ins.first->second: the value (Symbol*).
   // ins.second: true if new entry was inserted, false if not.
 
-  Sized_symbol<size>* ret;
+  Sized_symbol<size>* ret = NULL;
   bool was_undefined_in_reg;
   bool was_common;
   if (!ins.second)
@@ -1049,17 +1049,42 @@ Symbol_table::add_from_object(Object* object,
 	  // it, then change it to NAME/VERSION.
 	  ret = this->get_sized_symbol<size>(insdefault.first->second);
 
-	  was_undefined_in_reg = ret->is_undefined() && ret->in_reg();
-	  // Commons from plugins are just placeholders.
-	  was_common = ret->is_common() && ret->object()->pluginobj() == NULL;
-
-	  this->resolve(ret, sym, st_shndx, is_ordinary, orig_st_shndx, object,
-			version, is_default_version);
-          if (parameters->options().gc_sections())
-            this->gc_mark_dyn_syms(ret);
-	  ins.first->second = ret;
+	  // If the existing symbol already has a version,
+	  // don't override it with the new symbol.
+	  // This should only happen when the new symbol
+	  // is from a shared library.
+	  if (ret->version() != NULL)
+	    {
+	      if (!object->is_dynamic())
+	        {
+		  gold_warning(_("%s: conflicting default version definition"
+				 " for %s@@%s"),
+			       object->name().c_str(), name, version);
+		  if (ret->source() == Symbol::FROM_OBJECT)
+		    gold_info(_("%s: %s: previous definition of %s@@%s here"),
+			      program_name,
+			      ret->object()->name().c_str(),
+			      name, ret->version());
+	        }
+	      ret = NULL;
+	      is_default_version = false;
+	    }
+	  else
+	    {
+	      was_undefined_in_reg = ret->is_undefined() && ret->in_reg();
+	      // Commons from plugins are just placeholders.
+	      was_common = (ret->is_common()
+			    && ret->object()->pluginobj() == NULL);
+
+	      this->resolve(ret, sym, st_shndx, is_ordinary, orig_st_shndx,
+			    object, version, is_default_version);
+	      if (parameters->options().gc_sections())
+		this->gc_mark_dyn_syms(ret);
+	      ins.first->second = ret;
+	    }
 	}
-      else
+
+      if (ret == NULL)
 	{
 	  was_undefined_in_reg = false;
 	  was_common = false;
diff --git a/gold/symtab.h b/gold/symtab.h
index fdb75114ac..089e156b45 100644
--- a/gold/symtab.h
+++ b/gold/symtab.h
@@ -344,6 +344,11 @@ class Symbol
   set_in_dyn()
   { this->in_dyn_ = true; }
 
+  // Return whether this symbol is defined in a dynamic object.
+  bool
+  from_dyn() const
+  { return this->source_ == FROM_OBJECT && this->object()->is_dynamic(); }
+
   // Return whether this symbol has been seen in a real ELF object.
   // (IN_REG will return TRUE if the symbol has been seen in either
   // a real ELF object or an object claimed by a plugin.)
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 7140df64d0..c926f8ba8f 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -2516,6 +2516,23 @@ plugin_pr22868_b_ir.o: plugin_pr22868_b.c
 plugin_pr22868_b.o: plugin_pr22868_b.c
 	$(COMPILE) -c -fpic -o $@ $<
 
+check_SCRIPTS += ver_test_pr16504.sh
+check_DATA += ver_test_pr16504.stdout
+ver_test_pr16504.stdout: ver_test_pr16504.so
+	$(TEST_READELF) -W --dyn-syms $< >$@ 2>/dev/null
+ver_test_pr16504.so: ver_test_pr16504_a.so ver_test_pr16504_b.o.syms ver_test_pr16504_b.script gcctestdir/ld
+	gcctestdir/ld -shared -o $@ --plugin ./plugin_test.so --version-script $(srcdir)/ver_test_pr16504_b.script ver_test_pr16504_b.o.syms ver_test_pr16504_a.so
+ver_test_pr16504_a.so: ver_test_pr16504_a.o ver_test_pr16504_a.script gcctestdir/ld
+	gcctestdir/ld -shared -o $@ --version-script $(srcdir)/ver_test_pr16504_a.script ver_test_pr16504_a.o
+ver_test_pr16504_a.o: ver_test_pr16504_a.c
+	$(COMPILE) -c -fpic -o $@ $<
+# Filter out the UNDEFs from the symbols file to simulate GCC behavior,
+# which does not pass the UNDEF from a .symver directive.
+ver_test_pr16504_b.o.syms: ver_test_pr16504_b.o
+	$(TEST_READELF) -sW $< 2>/dev/null | grep -v "UND" >$@
+ver_test_pr16504_b.o: ver_test_pr16504_b.c
+	$(COMPILE) -c -fpic -o $@ $<
+
 endif PLUGINS
 
 check_PROGRAMS += exclude_libs_test
diff --git a/gold/testsuite/plugin_test.c b/gold/testsuite/plugin_test.c
index e80f92626f..a21abcad4b 100644
--- a/gold/testsuite/plugin_test.c
+++ b/gold/testsuite/plugin_test.c
@@ -46,6 +46,7 @@ struct sym_info
   char* vis;
   char* sect;
   char* name;
+  char* ver;
 };
 
 static struct claimed_file* first_claimed_file = NULL;
@@ -397,7 +398,14 @@ claim_file_hook (const struct ld_plugin_input_file* file, int* claimed)
           syms[nsyms].name = malloc(len + 1);
           strncpy(syms[nsyms].name, info.name, len + 1);
         }
-      syms[nsyms].version = NULL;
+      if (info.ver == NULL)
+        syms[nsyms].version = NULL;
+      else
+        {
+          len = strlen(info.ver);
+          syms[nsyms].version = malloc(len + 1);
+          strncpy(syms[nsyms].version, info.ver, len + 1);
+        }
       syms[nsyms].def = def;
       syms[nsyms].visibility = vis;
       syms[nsyms].size = info.size;
@@ -676,11 +684,26 @@ parse_readelf_line(char* p, struct sym_info* info)
   p += strspn(p, " ");
 
   /* Name field.  */
-  /* FIXME:  Look for version.  */
-  len = strlen(p);
-  if (len == 0)
-    p = NULL;
-  else if (p[len-1] == '\n')
-    p[--len] = '\0';
-  info->name = p;
+  len = strcspn(p, "@\n");
+  if (len > 0 && p[len] == '@')
+    {
+      /* Get the symbol version.  */
+      char* vp = p + len;
+      int vlen;
+
+      vp += strspn(vp, "@");
+      vlen = strcspn(vp, "\n");
+      vp[vlen] = '\0';
+      if (vlen > 0)
+	info->ver = vp;
+      else
+	info->ver = NULL;
+    }
+  else
+    info->ver = NULL;
+  p[len] = '\0';
+  if (len > 0)
+    info->name = p;
+  else
+    info->name = NULL;
 }
diff --git a/gold/testsuite/ver_test_pr16504.sh b/gold/testsuite/ver_test_pr16504.sh
new file mode 100755
index 0000000000..f4e3b8b028
--- /dev/null
+++ b/gold/testsuite/ver_test_pr16504.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+# ver_test_pr16504.sh -- test that undef gets correct version with LTO.
+
+# Copyright (C) 2018 Free Software Foundation, Inc.
+# Written by Cary Coutant <ccoutant@gmail.com>.
+
+# This file is part of gold.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected symbol in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check ver_test_pr16504.stdout " FUNC .* UND  *foo@VER2"
+check ver_test_pr16504.stdout " IFUNC .* foo@@VER1"
+
+exit 0
diff --git a/gold/testsuite/ver_test_pr16504_a.c b/gold/testsuite/ver_test_pr16504_a.c
new file mode 100644
index 0000000000..76280228fa
--- /dev/null
+++ b/gold/testsuite/ver_test_pr16504_a.c
@@ -0,0 +1,5 @@
+const char* foo(void);
+
+const char*
+foo(void)
+{ return "x"; }
diff --git a/gold/testsuite/ver_test_pr16504_a.script b/gold/testsuite/ver_test_pr16504_a.script
new file mode 100644
index 0000000000..86bb0a0355
--- /dev/null
+++ b/gold/testsuite/ver_test_pr16504_a.script
@@ -0,0 +1,4 @@
+VER2 {
+global:
+  foo;
+};
diff --git a/gold/testsuite/ver_test_pr16504_b.c b/gold/testsuite/ver_test_pr16504_b.c
new file mode 100644
index 0000000000..e6d82b6d37
--- /dev/null
+++ b/gold/testsuite/ver_test_pr16504_b.c
@@ -0,0 +1,10 @@
+void
+new_foo(void);
+__asm__(".symver new_foo,foo@VER2");
+
+static void (*resolve_foo(void)) (void)
+{
+  return new_foo;
+}
+
+void foo(void) __attribute__((ifunc("resolve_foo")));
diff --git a/gold/testsuite/ver_test_pr16504_b.script b/gold/testsuite/ver_test_pr16504_b.script
new file mode 100644
index 0000000000..94b373dd65
--- /dev/null
+++ b/gold/testsuite/ver_test_pr16504_b.script
@@ -0,0 +1,4 @@
+VER1 {
+global:
+  foo;
+};