gold patch committed: Fix symbol visibility (PR 9836)

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

PR 9836 is about a bug in gold's symbol visibility handling: it was not
forcing symbols with hidden visibility to be local.  I committed this
patch to fix the bug, along with a test.

Ian


2009-02-27  Ian Lance Taylor  <iant@google.com>

	PR 9836
	* symtab.cc (Symbol_table::add_from_object): If the visibility is
	hidden or internal, force the symbol to be local.
	* resolve.cc (Symbol::override_visibility): Define.
	(Symbol::override_base): Use override_visibility.
	(Symbol_table::resolve): Likewise.
	(Symbol::override_base_with_special): Likewise.
	(Symbol_table::override_with_special): If the visibility is hidden
	or internal, force the symbol to be local.
	* symtab.h (class Symbol): Add set_visibility and
	override_visibility.
	* testsuite/ver_test_1.sh: New file.
	* testsuite/Makefile.am (check_SCRIPTS): Add ver_test_1.sh.
	(check_DATA): Add ver_test_1.syms.
	(ver_test_1.syms): New target.
	* testsuite/Makefile.in: Rebuild.


Index: resolve.cc
===================================================================
RCS file: /cvs/src/src/gold/resolve.cc,v
retrieving revision 1.38
diff -p -u -r1.38 resolve.cc
--- resolve.cc	19 Sep 2008 22:54:57 -0000	1.38
+++ resolve.cc	27 Feb 2009 19:50:48 -0000
@@ -1,6 +1,6 @@
 // resolve.cc -- symbol resolution for gold
 
-// Copyright 2006, 2007, 2008 Free Software Foundation, Inc.
+// Copyright 2006, 2007, 2008, 2009 Free Software Foundation, Inc.
 // Written by Ian Lance Taylor <iant@google.com>.
 
 // This file is part of gold.
@@ -63,6 +63,26 @@ Symbol::override_version(const char* ver
     }
 }
 
+// This symbol is being overidden by another symbol whose visibility
+// is VISIBILITY.  Updated the VISIBILITY_ field accordingly.
+
+inline void
+Symbol::override_visibility(elfcpp::STV visibility)
+{
+  // The rule for combining visibility is that we always choose the
+  // most constrained visibility.  In order of increasing constraint,
+  // visibility goes PROTECTED, HIDDEN, INTERNAL.  This is the reverse
+  // of the numeric values, so the effect is that we always want the
+  // smallest non-zero value.
+  if (visibility != elfcpp::STV_DEFAULT)
+    {
+      if (this->visibility_ == elfcpp::STV_DEFAULT)
+	this->visibility_ = visibility;
+      else if (this->visibility_ > visibility)
+	this->visibility_ = visibility;
+    }
+}
+
 // Override the fields in Symbol.
 
 template<int size, bool big_endian>
@@ -78,7 +98,7 @@ Symbol::override_base(const elfcpp::Sym<
   this->is_ordinary_shndx_ = is_ordinary;
   this->type_ = sym.get_st_type();
   this->binding_ = sym.get_st_bind();
-  this->visibility_ = sym.get_st_visibility();
+  this->override_visibility(sym.get_st_visibility());
   this->nonvis_ = sym.get_st_nonvis();
   if (object->is_dynamic())
     this->in_dyn_ = true;
@@ -279,6 +299,9 @@ Symbol_table::resolve(Sized_symbol<size>
     {
       if (adjust_common_sizes && sym.get_st_size() > to->symsize())
         to->set_symsize(sym.get_st_size());
+      // The ELF ABI says that even for a reference to a symbol we
+      // merge the visibility.
+      to->override_visibility(sym.get_st_visibility());
     }
 
   // A new weak undefined reference, merging with an old weak
@@ -721,7 +744,7 @@ Symbol::override_base_with_special(const
   this->override_version(from->version_);
   this->type_ = from->type_;
   this->binding_ = from->binding_;
-  this->visibility_ = from->visibility_;
+  this->override_visibility(from->visibility_);
   this->nonvis_ = from->nonvis_;
 
   // Special symbols are always considered to be regular symbols.
@@ -776,7 +799,12 @@ Symbol_table::override_with_special(Size
 	}
       while (ssym != tosym);
     }
-  if (tosym->binding() == elfcpp::STB_LOCAL)
+  if (tosym->binding() == elfcpp::STB_LOCAL
+      || ((tosym->visibility() == elfcpp::STV_HIDDEN
+	   || tosym->visibility() == elfcpp::STV_INTERNAL)
+	  && (tosym->binding() == elfcpp::STB_GLOBAL
+	      || tosym->binding() == elfcpp::STB_WEAK)
+	  && !parameters->options().relocatable()))
     this->force_local(tosym);
 }
 
Index: symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.114
diff -p -u -r1.114 symtab.cc
--- symtab.cc	6 Feb 2009 19:20:09 -0000	1.114
+++ symtab.cc	27 Feb 2009 19:50:48 -0000
@@ -648,7 +648,8 @@ Symbol_table::resolve(Sized_symbol<size>
     this->gc_mark_dyn_syms(to);
 }
 
-// Record that a symbol is forced to be local by a version script.
+// Record that a symbol is forced to be local by a version script or
+// by visibility.
 
 void
 Symbol_table::force_local(Symbol* sym)
@@ -962,6 +963,15 @@ Symbol_table::add_from_object(Object* ob
 	this->tls_commons_.push_back(ret);
     }
 
+  // If we're not doing a relocatable link, then any symbol with
+  // hidden or internal visibility is local.
+  if ((ret->visibility() == elfcpp::STV_HIDDEN
+       || ret->visibility() == elfcpp::STV_INTERNAL)
+      && (ret->binding() == elfcpp::STB_GLOBAL
+	  || ret->binding() == elfcpp::STB_WEAK)
+      && !parameters->options().relocatable())
+    this->force_local(ret);
+
   if (def)
     ret->set_is_default();
   return ret;
@@ -1179,7 +1189,7 @@ Symbol_table::add_from_pluginobj(
 		              def, *sym, st_shndx, true, st_shndx);
 
   if (local)
-	this->force_local(res);
+    this->force_local(res);
 
   return res;
 }
Index: symtab.h
===================================================================
RCS file: /cvs/src/src/gold/symtab.h,v
retrieving revision 1.84
diff -p -u -r1.84 symtab.h
--- symtab.h	28 Jan 2009 02:25:33 -0000	1.84
+++ symtab.h	27 Feb 2009 19:50:49 -0000
@@ -1,6 +1,6 @@
 // symtab.h -- the gold symbol table   -*- C++ -*-
 
-// Copyright 2006, 2007, 2008 Free Software Foundation, Inc.
+// Copyright 2006, 2007, 2008, 2009 Free Software Foundation, Inc.
 // Written by Ian Lance Taylor <iant@google.com>.
 
 // This file is part of gold.
@@ -210,6 +210,15 @@ class Symbol
   visibility() const
   { return this->visibility_; }
 
+  // Set the visibility.
+  void
+  set_visibility(elfcpp::STV visibility)
+  { this->visibility_ = visibility; }
+
+  // Override symbol visibility.
+  void
+  override_visibility(elfcpp::STV);
+
   // Return the non-visibility part of the st_other field.
   unsigned char
   nonvis() const
@@ -1384,7 +1393,8 @@ class Symbol_table
   void
   resolve(Sized_symbol<size>* to, const Sized_symbol<size>* from);
 
-  // Record that a symbol is forced to be local by a version script.
+  // Record that a symbol is forced to be local by a version script or
+  // by visibility.
   void
   force_local(Symbol*);
 
Index: testsuite/Makefile.am
===================================================================
RCS file: /cvs/src/src/gold/testsuite/Makefile.am,v
retrieving revision 1.84
diff -p -u -r1.84 Makefile.am
--- testsuite/Makefile.am	25 Feb 2009 19:05:21 -0000	1.84
+++ testsuite/Makefile.am	27 Feb 2009 19:50:49 -0000
@@ -742,6 +742,11 @@ ver_test_3.o: ver_test_3.cc
 ver_test_4.o: ver_test_4.cc
 	$(CXXCOMPILE) -c -fpic -o $@ $<
 
+check_SCRIPTS += ver_test_1.sh
+check_DATA += ver_test_1.syms
+ver_test_1.syms: ver_test_1.so
+	$(TEST_READELF) -s $< >$@ 2>/dev/null
+
 check_PROGRAMS += ver_test_2
 ver_test_2_SOURCES = ver_test_main_2.cc
 ver_test_2_DEPENDENCIES = gcctestdir/ld ver_test_4.so ver_test_2.so
Index: testsuite/ver_test_1.sh
===================================================================
RCS file: testsuite/ver_test_1.sh
diff -N testsuite/ver_test_1.sh
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/ver_test_1.sh	27 Feb 2009 19:50:49 -0000
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+# ver_test_1.sh -- check that protected symbols are local
+
+# Copyright 2009 Free Software Foundation, Inc.
+# Written by Ian Lance Taylor <iant@google.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.
+
+syms=`grep ' HIDDEN ' ver_test_1.syms | grep ' GLOBAL '`
+if test -n "$syms"; then
+  echo "Found GLOBAL HIDDEN symbols"
+  echo $syms
+  exit 1
+fi