Linker plugins should be aware of --defsym during symbol resolution

Message ID CAAs8HmztRc_qZ3Kqxf1EwQ+mTPZnpoUs0cSRvda_wYnES++pCQ@mail.gmail.com
State New
Headers show
Series
  • Linker plugins should be aware of --defsym during symbol resolution
Related show

Commit Message

Jose E. Marchesi via Binutils Feb. 3, 2018, 12:33 a.m.
Hello,

   I have a patch that fixes symbol resolutions when defined or used
in defsym expressions. Please take a look.

include/plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

gold/
* expression.cc (Symbol_expression::is_symbol_in_expression):
      New method.
(Unary_expression::is_symbol_in_expression): New method.
(Binary_expression::is_symbol_in_expression): New method.
(Trinary_expression::is_symbol_in_expression): New method.
* plugin.cc (is_referenced_from_outside): Check if the symbol is
used in a defsym expression.
(get_symbol_resolution_info): Fix symbol resolution if defined or
used in defsyms.
* script.cc (Script_options::is_defsym_def): New method.
(Script_options::is_defsym_use): New method.
* script.h (Expression::is_symbol_in_expression): New method.
(Symbol_assignment::is_defsym): New method.
(Symbol_assignment::value): New method.
(Script_options::is_defsym_def): New method.
(Script_options::is_defsym_use): New method.
* testsuite/Makefile.am (plugin_test_defsym): New test.
* testsuite/Makefile.in: Regenerate.
* testsuite/plugin_test.c: Check for new symbol resolution.
* testsuite/plugin_test_defsym.sh: New script.
* testsuite/plugin_test_defsym.c: New test source.


Thanks
Sri
include/plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

	gold/
	* expression.cc (Symbol_expression::is_symbol_in_expression):
      	New method.
	(Unary_expression::is_symbol_in_expression): New method.
	(Binary_expression::is_symbol_in_expression): New method.
	(Trinary_expression::is_symbol_in_expression): New method.
	* plugin.cc (is_referenced_from_outside): Check if the symbol is
	used in a defsym expression.
	(get_symbol_resolution_info): Fix symbol resolution if defined or
	used in defsyms.
	* script.cc (Script_options::is_defsym_def): New method.
	(Script_options::is_defsym_use): New method.
	* script.h (Expression::is_symbol_in_expression): New method.
	(Symbol_assignment::is_defsym): New method.
	(Symbol_assignment::value): New method.
	(Script_options::is_defsym_def): New method.
	(Script_options::is_defsym_use): New method.
	* testsuite/Makefile.am (plugin_test_defsym): New test.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/plugin_test.c: Check for new symbol resolution.
	* testsuite/plugin_test_defsym.sh: New script.
	* testsuite/plugin_test_defsym.c: New test source.

Comments

Jose E. Marchesi via Binutils Feb. 12, 2018, 6:05 p.m. | #1
Ping.

include/plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

gold/
* expression.cc (Symbol_expression::is_symbol_in_expression):
      New method.
(Unary_expression::is_symbol_in_expression): New method.
(Binary_expression::is_symbol_in_expression): New method.
(Trinary_expression::is_symbol_in_expression): New method.
* plugin.cc (is_referenced_from_outside): Check if the symbol is
used in a defsym expression.
(get_symbol_resolution_info): Fix symbol resolution if defined or
used in defsyms.
* script.cc (Script_options::is_defsym_def): New method.
(Script_options::is_defsym_use): New method.
* script.h (Expression::is_symbol_in_expression): New method.
(Symbol_assignment::is_defsym): New method.
(Symbol_assignment::value): New method.
(Script_options::is_defsym_def): New method.
(Script_options::is_defsym_use): New method.
* testsuite/Makefile.am (plugin_test_defsym): New test.
* testsuite/Makefile.in: Regenerate.
* testsuite/plugin_test.c: Check for new symbol resolution.
* testsuite/plugin_test_defsym.sh: New script.
* testsuite/plugin_test_defsym.c: New test source.



On Fri, Feb 2, 2018 at 4:33 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hello,

>

>    I have a patch that fixes symbol resolutions when defined or used

> in defsym expressions. Please take a look.

>

> include/plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

>

> gold/

> * expression.cc (Symbol_expression::is_symbol_in_expression):

>       New method.

> (Unary_expression::is_symbol_in_expression): New method.

> (Binary_expression::is_symbol_in_expression): New method.

> (Trinary_expression::is_symbol_in_expression): New method.

> * plugin.cc (is_referenced_from_outside): Check if the symbol is

> used in a defsym expression.

> (get_symbol_resolution_info): Fix symbol resolution if defined or

> used in defsyms.

> * script.cc (Script_options::is_defsym_def): New method.

> (Script_options::is_defsym_use): New method.

> * script.h (Expression::is_symbol_in_expression): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::is_defsym_use): New method.

> * testsuite/Makefile.am (plugin_test_defsym): New test.

> * testsuite/Makefile.in: Regenerate.

> * testsuite/plugin_test.c: Check for new symbol resolution.

> * testsuite/plugin_test_defsym.sh: New script.

> * testsuite/plugin_test_defsym.c: New test source.

>

>

> Thanks

> Sri
include/plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

	gold/
	* expression.cc (Symbol_expression::is_symbol_in_expression):
      	New method.
	(Unary_expression::is_symbol_in_expression): New method.
	(Binary_expression::is_symbol_in_expression): New method.
	(Trinary_expression::is_symbol_in_expression): New method.
	* plugin.cc (is_referenced_from_outside): Check if the symbol is
	used in a defsym expression.
	(get_symbol_resolution_info): Fix symbol resolution if defined or
	used in defsyms.
	* script.cc (Script_options::is_defsym_def): New method.
	(Script_options::is_defsym_use): New method.
	* script.h (Expression::is_symbol_in_expression): New method.
	(Symbol_assignment::is_defsym): New method.
	(Symbol_assignment::value): New method.
	(Script_options::is_defsym_def): New method.
	(Script_options::is_defsym_use): New method.
	* testsuite/Makefile.am (plugin_test_defsym): New test.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/plugin_test.c: Check for new symbol resolution.
	* testsuite/plugin_test_defsym.sh: New script.
	* testsuite/plugin_test_defsym.c: New test source.
	

diff --git a/gold/expression.cc b/gold/expression.cc
index d764cc2a9d..bf65304833 100644
--- a/gold/expression.cc
+++ b/gold/expression.cc
@@ -205,6 +205,12 @@ class Symbol_expression : public Expression
   uint64_t
   value(const Expression_eval_info*);
 
+  bool
+  is_symbol_in_expression(const char* name)
+  {
+    return this->name_ == name;
+  }
+
   void
   print(FILE* f) const
   { fprintf(f, "%s", this->name_.c_str()); }
@@ -318,6 +324,10 @@ class Unary_expression : public Expression
   arg_print(FILE* f) const
   { this->arg_->print(f); }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  { return this->arg_->is_symbol_in_expression(name); }
+
  private:
   Expression* arg_;
 };
@@ -437,6 +447,13 @@ class Binary_expression : public Expression
     fprintf(f, ")");
   }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  {
+    return (this->left_->is_symbol_in_expression(name) ||
+            this->right_->is_symbol_in_expression(name));
+  }
+
  private:
   Expression* left_;
   Expression* right_;
@@ -622,6 +639,12 @@ class Trinary_expression : public Expression
   arg3_print(FILE* f) const
   { this->arg3_->print(f); }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  { return (this->arg1_->is_symbol_in_expression(name) ||
+	    this->arg2_->is_symbol_in_expression(name) ||
+	    this->arg3_->is_symbol_in_expression(name)); }
+
  private:
   Expression* arg1_;
   Expression* arg2_;
diff --git a/gold/plugin.cc b/gold/plugin.cc
index 02fef25dda..7fd42821af 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -942,6 +942,9 @@ is_referenced_from_outside(Symbol* lsym)
     return true;
   if (parameters->options().is_undefined(lsym->name()))
     return true;
+  Layout* layout = parameters->options().plugins()->layout();
+  if (layout->script_options()->is_defsym_use(lsym->name()))
+    return true;
   return false;
 }
 
@@ -989,6 +992,8 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
       return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
     }
 
+  Layout *layout = parameters->options().plugins()->layout();
+
   for (int i = 0; i < nsyms; i++)
     {
       ld_plugin_symbol* isym = &syms[i];
@@ -997,9 +1002,16 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
         lsym = symtab->resolve_forwards(lsym);
       ld_plugin_symbol_resolution res = LDPR_UNKNOWN;
 
-      if (lsym->is_undefined())
-        // The symbol remains undefined.
-        res = LDPR_UNDEF;
+      if (layout->script_options()->is_defsym_def(lsym->name()))
+	{
+	  // The symbol is redefined.
+	  res = LDPR_LINKER_REDEFINED;
+	}
+      else if (lsym->is_undefined())
+	{
+            // The symbol remains undefined.
+            res = LDPR_UNDEF;
+	}
       else if (isym->def == LDPK_UNDEF
                || isym->def == LDPK_WEAKUNDEF
                || isym->def == LDPK_COMMON)
diff --git a/gold/script.cc b/gold/script.cc
index db243cf435..e0a8d9a742 100644
--- a/gold/script.cc
+++ b/gold/script.cc
@@ -1112,6 +1112,33 @@ Script_options::is_pending_assignment(const char* name)
   return false;
 }
 
+// Returns trus if symbol with NAME is redefined by defsym.
+
+bool Script_options::is_defsym_def(const char* name)
+{
+  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    if ((*p)->is_defsym() && (*p)->name() == name)
+      return true;
+  return false;
+}
+
+// Returns trus if symbol with NAME is used in a defsym expression.
+
+bool
+Script_options::is_defsym_use(const char* name)
+{
+  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(name))
+        return true;
+    }
+  return false;
+}
+
 // Add a symbol to be defined.
 
 void
diff --git a/gold/script.h b/gold/script.h
index ec8ce8e110..2159f9f7e7 100644
--- a/gold/script.h
+++ b/gold/script.h
@@ -129,13 +129,18 @@ class Expression
   virtual uint64_t
   value(const Expression_eval_info*) = 0;
 
+  // Return true if symbol is part of the expression.
+  // The child class that uses this symbol returns true.
+  virtual bool
+  is_symbol_in_expression(const char*)
+  { return false; }
+
  private:
   // May not be copied.
   Expression(const Expression&);
   Expression& operator=(const Expression&);
 };
 
-
 // Version_script_info stores information parsed from the version
 // script, either provided by --version-script or as part of a linker
 // script.  A single Version_script_info object per target is owned by
@@ -344,6 +349,14 @@ class Symbol_assignment
   void
   finalize(Symbol_table*, const Layout*);
 
+  bool
+  is_defsym() const
+  { return is_defsym_; }
+
+  Expression *
+  value() const
+  { return val_; }
+
   // Finalize the symbol value when it can refer to the dot symbol.
   void
   finalize_with_dot(Symbol_table*, const Layout*, uint64_t dot_value,
@@ -454,6 +467,14 @@ class Script_options
   bool
   define_symbol(const char* definition);
 
+  // Returns true if symbol is defined via defsym.
+  bool
+  is_defsym_def(const char *name);
+
+  // Returns true if symbol is used in a defsym.
+  bool
+  is_defsym_use(const char *name);
+
   // Create sections required by any linker scripts.
   void
   create_script_sections(Layout*);
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 16cae8004c..b94182856e 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -2332,6 +2332,19 @@ plugin_test_start_lib: unused.o plugin_start_lib_test.o plugin_start_lib_test_2.
 plugin_test_start_lib.err: plugin_test_start_lib
 	@touch plugin_test_start_lib.err
 
+check_PROGRAMS += plugin_test_defsym
+check_SCRIPTS += plugin_test_defsym.sh
+check_DATA += plugin_test_defsym.err
+MOSTLYCLEANFILES += plugin_test_defsym.err
+plugin_test_defsym.syms: plugin_test_defsym.o
+	$(TEST_READELF) -sW $< >$@ 2>/dev/null
+plugin_test_defsym.o: plugin_test_defsym.c
+	$(COMPILE) -c -o $@ $<
+plugin_test_defsym: plugin_test_defsym.o plugin_test_defsym.syms gcctestdir/ld plugin_test.so
+	$(CXXLINK) -Bgcctestdir/ -Wl,--no-demangle,--plugin,"./plugin_test.so" \
+		-Wl,--defsym,bar=foo+2 plugin_test_defsym.syms 2>plugin_test_defsym.err
+plugin_test_defsym.err: plugin_test_defsym
+	@touch plugin_test_defsym.err
 
 plugin_start_lib_test_2.syms: plugin_start_lib_test_2.o
 	$(TEST_READELF) -sW $< >$@ 2>/dev/null
diff --git a/gold/testsuite/plugin_test.c b/gold/testsuite/plugin_test.c
index c6df7b6f7d..b92fb8acf5 100644
--- a/gold/testsuite/plugin_test.c
+++ b/gold/testsuite/plugin_test.c
@@ -476,6 +476,9 @@ all_symbols_read_hook(void)
             case LDPR_RESOLVED_DYN:
               res = "RESOLVED_DYN";
               break;
+	    case LDPR_LINKER_REDEFINED:
+	      res = "LINKER_REDEFINED";
+	      break;
             default:
               res = "?";
               break;
diff --git a/gold/testsuite/plugin_test_defsym.c b/gold/testsuite/plugin_test_defsym.c
index e69de29bb2..a3c3b962f3 100644
--- a/gold/testsuite/plugin_test_defsym.c
+++ b/gold/testsuite/plugin_test_defsym.c
@@ -0,0 +1,32 @@
+/* plugin_test_defsym.c -- a test case for gold
+
+   Copyright (C) 2018 onwards Free Software Foundation, Inc.
+   Written by Sriraman Tallam <tmsriram@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.  */
+
+int foo(void);
+int foo(void) {
+  return 0;
+}
+
+int bar(void);
+
+int main(void) {
+  return bar();  
+}
diff --git a/gold/testsuite/plugin_test_defsym.sh b/gold/testsuite/plugin_test_defsym.sh
index e69de29bb2..3eb6446433 100755
--- a/gold/testsuite/plugin_test_defsym.sh
+++ b/gold/testsuite/plugin_test_defsym.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+# plugin_test_defsym.sh -- a test case for the plugin API.
+
+# Copyright (C) 2018 onwards Free Software Foundation, Inc.
+# Written by Sriraman Tallam <tmsriram@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.
+
+# This file goes with plugin_test.c, a simple plug-in library that
+# exercises the basic interfaces and prints out version numbers and
+# options passed to the plugin.
+
+# This checks if the symbol resolution withe export dynamic symbol is
+# as expected.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected output in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check plugin_test_defsym.err "API version:"
+check plugin_test_defsym.err "gold version:"
+check plugin_test_defsym.err "plugin_test_defsym.syms: claim file hook called"
+check plugin_test_defsym.err "plugin_test_defsym.syms: bar: LINKER_REDEFINED"
+check plugin_test_defsym.err "plugin_test_defsym.syms: foo: PREVAILING_DEF_REG"
+check plugin_test_defsym.err "cleanup hook called"
+
+exit 0
diff --git a/include/plugin-api.h b/include/plugin-api.h
index 03e21f4a0c..6c02947699 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -162,7 +162,11 @@ enum ld_plugin_symbol_resolution
      references from regular objects.  It is only referenced from IR
      code, but the symbol is exported and may be referenced from
      a dynamic object (not seen at link time).  */
-  LDPR_PREVAILING_DEF_IRONLY_EXP
+  LDPR_PREVAILING_DEF_IRONLY_EXP,
+
+  /* This symbol has been redefined by the linker, typically using
+     defsym.  */
+  LDPR_LINKER_REDEFINED,
 };
 
 /* The plugin library's "claim file" handler.  */
Cary Coutant Feb. 12, 2018, 8:19 p.m. | #2
> include/

> * plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.


As always, this change needs to be synced with the GCC tree, and the
whopr/driver wiki page needs updating.

Can you explain why this new resolution is needed? Why would
LDPR_PREEMPTED_REG not work?

As we did with LDPR_PREVAILING_DEF_IRONLY_EXP and LDPS_NO_SYMS, you'll
need to create a new version of get_symbols (LDPT_GET_SYMBOLS_V4), so
that old plugins that aren't prepared for the new resolution enum
don't break.

Hmmm, in get_symbol_resolution_info, we have this:

  if (nsyms > this->nsyms_)
    return LDPS_NO_SYMS;

That needs to be qualified with a test for version > 2 (as was done a
few lines below).

> gold/

> * expression.cc (Symbol_expression::is_symbol_in_expression):

>       New method.

> (Unary_expression::is_symbol_in_expression): New method.

> (Binary_expression::is_symbol_in_expression): New method.

> (Trinary_expression::is_symbol_in_expression): New method.

> * plugin.cc (is_referenced_from_outside): Check if the symbol is

> used in a defsym expression.

> (get_symbol_resolution_info): Fix symbol resolution if defined or

> used in defsyms.

> * script.cc (Script_options::is_defsym_def): New method.

> (Script_options::is_defsym_use): New method.

> * script.h (Expression::is_symbol_in_expression): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::is_defsym_use): New method.

> * testsuite/Makefile.am (plugin_test_defsym): New test.

> * testsuite/Makefile.in: Regenerate.

> * testsuite/plugin_test.c: Check for new symbol resolution.

> * testsuite/plugin_test_defsym.sh: New script.

> * testsuite/plugin_test_defsym.c: New test source.



+bool
+Script_options::is_defsym_use(const char* name)
+{
+  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(name))
+        return true;
+    }
+  return false;
+}

This looks like a pretty expensive test. Wouldn't it be better to just
call Symbol::set_in_real_elf() for each symbol we see used in an
expression?

+  bool
+  is_symbol_in_expression(const char* name)
+  {
+    return (this->left_->is_symbol_in_expression(name) ||
+            this->right_->is_symbol_in_expression(name));
+  }

Operators should be moved to the beginning of the next line.

+  bool
+  is_symbol_in_expression(const char* name)
+  { return (this->arg1_->is_symbol_in_expression(name) ||
+     this->arg2_->is_symbol_in_expression(name) ||
+     this->arg3_->is_symbol_in_expression(name)); }

Same, and the braces should be on their own lines here.

-cary
Cary Coutant Feb. 12, 2018, 8:26 p.m. | #3
> Hmmm, in get_symbol_resolution_info, we have this:

>

>   if (nsyms > this->nsyms_)

>     return LDPS_NO_SYMS;

>

> That needs to be qualified with a test for version > 2 (as was done a

> few lines below).


Ignore this -- we always returned NO_SYMS in this case, but added the
new case just below with LDPT_GET_SYMBOLS_V3. Sorry!

-cary
Jose E. Marchesi via Binutils Feb. 13, 2018, 5:44 p.m. | #4
On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <ccoutant@gmail.com> wrote:

> > include/

> > * plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

>

> As always, this change needs to be synced with the GCC tree, and the

> whopr/driver wiki page needs updating.

>

> Can you explain why this new resolution is needed? Why would

> LDPR_PREEMPTED_REG not work?

>


They mean different things. For example, LDPR_PREEMPTED_REG will mean that
there is a prevailing definition in a regular object file. But if the IR
def has weak ODR linkage, the LTO implementation knows that all copies must
be the same, and we can and currently do keep that def around long enough
for inlining. That would be incorrect in the defsym case, where this symbol
is actually redefined by the linker. That is what we are trying to
distinguish.

Teresa


> As we did with LDPR_PREVAILING_DEF_IRONLY_EXP and LDPS_NO_SYMS, you'll

> need to create a new version of get_symbols (LDPT_GET_SYMBOLS_V4), so

> that old plugins that aren't prepared for the new resolution enum

> don't break.

>

> Hmmm, in get_symbol_resolution_info, we have this:

>

>   if (nsyms > this->nsyms_)

>     return LDPS_NO_SYMS;

>

> That needs to be qualified with a test for version > 2 (as was done a

> few lines below).

>

> > gold/

> > * expression.cc (Symbol_expression::is_symbol_in_expression):

> >       New method.

> > (Unary_expression::is_symbol_in_expression): New method.

> > (Binary_expression::is_symbol_in_expression): New method.

> > (Trinary_expression::is_symbol_in_expression): New method.

> > * plugin.cc (is_referenced_from_outside): Check if the symbol is

> > used in a defsym expression.

> > (get_symbol_resolution_info): Fix symbol resolution if defined or

> > used in defsyms.

> > * script.cc (Script_options::is_defsym_def): New method.

> > (Script_options::is_defsym_use): New method.

> > * script.h (Expression::is_symbol_in_expression): New method.

> > (Symbol_assignment::is_defsym): New method.

> > (Symbol_assignment::value): New method.

> > (Script_options::is_defsym_def): New method.

> > (Script_options::is_defsym_use): New method.

> > * testsuite/Makefile.am (plugin_test_defsym): New test.

> > * testsuite/Makefile.in: Regenerate.

> > * testsuite/plugin_test.c: Check for new symbol resolution.

> > * testsuite/plugin_test_defsym.sh: New script.

> > * testsuite/plugin_test_defsym.c: New test source.

>

>

> +bool

> +Script_options::is_defsym_use(const char* name)

> +{

> +  for (Symbol_assignments::iterator p = this->symbol_assignments_.begi

> n();

> +       p != this->symbol_assignments_.end();

> +       ++p)

> +    {

> +      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_ex

> pression(name))

> +        return true;

> +    }

> +  return false;

> +}

>

> This looks like a pretty expensive test. Wouldn't it be better to just

> call Symbol::set_in_real_elf() for each symbol we see used in an

> expression?

>

> +  bool

> +  is_symbol_in_expression(const char* name)

> +  {

> +    return (this->left_->is_symbol_in_expression(name) ||

> +            this->right_->is_symbol_in_expression(name));

> +  }

>

> Operators should be moved to the beginning of the next line.

>

> +  bool

> +  is_symbol_in_expression(const char* name)

> +  { return (this->arg1_->is_symbol_in_expression(name) ||

> +     this->arg2_->is_symbol_in_expression(name) ||

> +     this->arg3_->is_symbol_in_expression(name)); }

>

> Same, and the braces should be on their own lines here.

>

> -cary

>




-- 
Teresa Johnson |  Software Engineer |  tejohnson@google.com |  408-460-2413
<(408)%20460-2413>
Cary Coutant Feb. 13, 2018, 6:24 p.m. | #5
>> Can you explain why this new resolution is needed? Why would

>> LDPR_PREEMPTED_REG not work?

>

> They mean different things. For example, LDPR_PREEMPTED_REG will mean that

> there is a prevailing definition in a regular object file. But if the IR def

> has weak ODR linkage, the LTO implementation knows that all copies must be

> the same, and we can and currently do keep that def around long enough for

> inlining. That would be incorrect in the defsym case, where this symbol is

> actually redefined by the linker. That is what we are trying to distinguish.


Do you have a real-world example? I'm having trouble imagining a case
where --defsym would be used to override a symbol that's subject to
the ODR and yet remain a valid program.

-cary
Jose E. Marchesi via Binutils Feb. 13, 2018, 6:32 p.m. | #6
On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> include/

>> * plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

>

> As always, this change needs to be synced with the GCC tree, and the

> whopr/driver wiki page needs updating.

>

> Can you explain why this new resolution is needed? Why would

> LDPR_PREEMPTED_REG not work?

>

> As we did with LDPR_PREVAILING_DEF_IRONLY_EXP and LDPS_NO_SYMS, you'll

> need to create a new version of get_symbols (LDPT_GET_SYMBOLS_V4), so

> that old plugins that aren't prepared for the new resolution enum

> don't break.

>

> Hmmm, in get_symbol_resolution_info, we have this:

>

>   if (nsyms > this->nsyms_)

>     return LDPS_NO_SYMS;

>

> That needs to be qualified with a test for version > 2 (as was done a

> few lines below).

>

>> gold/

>> * expression.cc (Symbol_expression::is_symbol_in_expression):

>>       New method.

>> (Unary_expression::is_symbol_in_expression): New method.

>> (Binary_expression::is_symbol_in_expression): New method.

>> (Trinary_expression::is_symbol_in_expression): New method.

>> * plugin.cc (is_referenced_from_outside): Check if the symbol is

>> used in a defsym expression.

>> (get_symbol_resolution_info): Fix symbol resolution if defined or

>> used in defsyms.

>> * script.cc (Script_options::is_defsym_def): New method.

>> (Script_options::is_defsym_use): New method.

>> * script.h (Expression::is_symbol_in_expression): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::is_defsym_use): New method.

>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>> * testsuite/Makefile.in: Regenerate.

>> * testsuite/plugin_test.c: Check for new symbol resolution.

>> * testsuite/plugin_test_defsym.sh: New script.

>> * testsuite/plugin_test_defsym.c: New test source.

>

>

> +bool

> +Script_options::is_defsym_use(const char* name)

> +{

> +  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();

> +       p != this->symbol_assignments_.end();

> +       ++p)

> +    {

> +      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(name))

> +        return true;

> +    }

> +  return false;

> +}

>

> This looks like a pretty expensive test. Wouldn't it be better to just

> call Symbol::set_in_real_elf() for each symbol we see used in an

> expression?


This is a nice idea and I am able to make this work for defsym uses.
Anything I could do for definitions too?  I could add a defined_ field
in Symbol which is set to Defined::DEFSYM.  That would avoid
is_defsym_def too.

expression.cc:
uint64_t
Symbol_expression::value(const Expression_eval_info* eei)
{
  Symbol* sym = eei->symtab->lookup(this->name_.c_str());

somewhere here would be too late.




>

> +  bool

> +  is_symbol_in_expression(const char* name)

> +  {

> +    return (this->left_->is_symbol_in_expression(name) ||

> +            this->right_->is_symbol_in_expression(name));

> +  }

>

> Operators should be moved to the beginning of the next line.

>

> +  bool

> +  is_symbol_in_expression(const char* name)

> +  { return (this->arg1_->is_symbol_in_expression(name) ||

> +     this->arg2_->is_symbol_in_expression(name) ||

> +     this->arg3_->is_symbol_in_expression(name)); }

>

> Same, and the braces should be on their own lines here.

>

> -cary
Jose E. Marchesi via Binutils Feb. 13, 2018, 6:33 p.m. | #7
On Tue, Feb 13, 2018 at 10:32 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>> include/

>>> * plugin-api.h (LDPR_LINKER_REDEFINED): New enum value.

>>

>> As always, this change needs to be synced with the GCC tree, and the

>> whopr/driver wiki page needs updating.

>>

>> Can you explain why this new resolution is needed? Why would

>> LDPR_PREEMPTED_REG not work?

>>

>> As we did with LDPR_PREVAILING_DEF_IRONLY_EXP and LDPS_NO_SYMS, you'll

>> need to create a new version of get_symbols (LDPT_GET_SYMBOLS_V4), so

>> that old plugins that aren't prepared for the new resolution enum

>> don't break.

>>

>> Hmmm, in get_symbol_resolution_info, we have this:

>>

>>   if (nsyms > this->nsyms_)

>>     return LDPS_NO_SYMS;

>>

>> That needs to be qualified with a test for version > 2 (as was done a

>> few lines below).

>>

>>> gold/

>>> * expression.cc (Symbol_expression::is_symbol_in_expression):

>>>       New method.

>>> (Unary_expression::is_symbol_in_expression): New method.

>>> (Binary_expression::is_symbol_in_expression): New method.

>>> (Trinary_expression::is_symbol_in_expression): New method.

>>> * plugin.cc (is_referenced_from_outside): Check if the symbol is

>>> used in a defsym expression.

>>> (get_symbol_resolution_info): Fix symbol resolution if defined or

>>> used in defsyms.

>>> * script.cc (Script_options::is_defsym_def): New method.

>>> (Script_options::is_defsym_use): New method.

>>> * script.h (Expression::is_symbol_in_expression): New method.

>>> (Symbol_assignment::is_defsym): New method.

>>> (Symbol_assignment::value): New method.

>>> (Script_options::is_defsym_def): New method.

>>> (Script_options::is_defsym_use): New method.

>>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>>> * testsuite/Makefile.in: Regenerate.

>>> * testsuite/plugin_test.c: Check for new symbol resolution.

>>> * testsuite/plugin_test_defsym.sh: New script.

>>> * testsuite/plugin_test_defsym.c: New test source.

>>

>>

>> +bool

>> +Script_options::is_defsym_use(const char* name)

>> +{

>> +  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();

>> +       p != this->symbol_assignments_.end();

>> +       ++p)

>> +    {

>> +      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(name))

>> +        return true;

>> +    }

>> +  return false;

>> +}

>>

>> This looks like a pretty expensive test. Wouldn't it be better to just

>> call Symbol::set_in_real_elf() for each symbol we see used in an

>> expression?

>

> This is a nice idea and I am able to make this work for defsym uses.

> Anything I could do for definitions too?  I could add a defined_ field

> in Symbol which is set to Defined::DEFSYM.  That would avoid

> is_defsym_def too.


Please ignore this part below:

 expression.cc:
 uint64_t
 Symbol_expression::value(const Expression_eval_info* eei)
 {
   Symbol* sym = eei->symtab->lookup(this->name_.c_str());

 somewhere here would be too late.



>

>

>

>

>>

>> +  bool

>> +  is_symbol_in_expression(const char* name)

>> +  {

>> +    return (this->left_->is_symbol_in_expression(name) ||

>> +            this->right_->is_symbol_in_expression(name));

>> +  }

>>

>> Operators should be moved to the beginning of the next line.

>>

>> +  bool

>> +  is_symbol_in_expression(const char* name)

>> +  { return (this->arg1_->is_symbol_in_expression(name) ||

>> +     this->arg2_->is_symbol_in_expression(name) ||

>> +     this->arg3_->is_symbol_in_expression(name)); }

>>

>> Same, and the braces should be on their own lines here.

>>

>> -cary
Jose E. Marchesi via Binutils Feb. 13, 2018, 7:51 p.m. | #8
On Tue, Feb 13, 2018 at 10:24 AM, Cary Coutant <ccoutant@gmail.com> wrote:

> >> Can you explain why this new resolution is needed? Why would

> >> LDPR_PREEMPTED_REG not work?

> >

> > They mean different things. For example, LDPR_PREEMPTED_REG will mean

> that

> > there is a prevailing definition in a regular object file. But if the IR

> def

> > has weak ODR linkage, the LTO implementation knows that all copies must

> be

> > the same, and we can and currently do keep that def around long enough

> for

> > inlining. That would be incorrect in the defsym case, where this symbol

> is

> > actually redefined by the linker. That is what we are trying to

> distinguish.

>

> Do you have a real-world example? I'm having trouble imagining a case

> where --defsym would be used to override a symbol that's subject to

> the ODR and yet remain a valid program.

>


I just concocted one:

$ cat bar.h
#include <stdio.h>
extern inline void bar()
{
  fprintf(stderr, "in bar\n");
}

extern inline void baz()
{
  fprintf(stderr, "in baz\n");
}

$ cat hello1.cc
#include <stdio.h>
#include "bar.h"

extern void foo();

int main() {
  baz();
  bar();
  foo();
  return 0;
}

$ cat hello2.cc
#include <stdio.h>
#include "bar.h"

void foo()
{
  bar();
}

Without defsym:

$ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold
$ a.out
in baz
in bar
in bar

With defsym:

$ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold
-Wl,--defsym,_Z3barv=_Z3bazv
$ a.out
in baz
in baz
in baz

In this case, because bar is inline it gets weak ODR linkage in the two
translation units.

Teresa


> -cary

>




-- 
Teresa Johnson |  Software Engineer |  tejohnson@google.com |  408-460-2413
<(408)%20460-2413>
Cary Coutant Feb. 13, 2018, 11:53 p.m. | #9
> This is a nice idea and I am able to make this work for defsym uses.

> Anything I could do for definitions too?  I could add a defined_ field

> in Symbol which is set to Defined::DEFSYM.  That would avoid

> is_defsym_def too.


I'd think Symbol::source() = Symbol::IS_CONSTANT would be the test you
need. That would also catch symbols defined in scripts, too.

-cary
Cary Coutant Feb. 14, 2018, 12:04 a.m. | #10
>> Do you have a real-world example? I'm having trouble imagining a case

>> where --defsym would be used to override a symbol that's subject to

>> the ODR and yet remain a valid program.

>

> I just concocted one:

> ...

> With defsym:

>

> $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

> -Wl,--defsym,_Z3barv=_Z3bazv


To me, this is the same as providing an overriding definition of bar()
that prints "in baz", which clearly violates the one-definition rule.
On what basis do you consider this a valid thing to do, to the extent
that you want to preserve the unoptimized behavior across LTO?

Is there a real-world example where someone would want to do this in
production code? I'm afraid I'd have zero sympathy for them. If they
want something like that to work, they should just turn off
cross-module inlining.

I need a lot more justification to extend the plugin API.

-cary
Jose E. Marchesi via Binutils Feb. 14, 2018, 12:08 a.m. | #11
On Tue, Feb 13, 2018 at 3:53 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> This is a nice idea and I am able to make this work for defsym uses.

>> Anything I could do for definitions too?  I could add a defined_ field

>> in Symbol which is set to Defined::DEFSYM.  That would avoid

>> is_defsym_def too.

>

> I'd think Symbol::source() = Symbol::IS_CONSTANT would be the test you

> need. That would also catch symbols defined in scripts, too.


I was looking at this.  This doesn't work when bar is declared in
foo.cc and then defined using defsym.  For example:

extern "C" {
int foo() {
  return 27;
}
extern int bar();
int main() {
  return bar();
}
}

$ gcc  -Wl,--defsym,bar=foo -Wl,--plugin,gold/testsuite/plugin_test.so
 foo.o.syms

bar is FROM_OBJECT.


>

> -cary
Jose E. Marchesi via Binutils Feb. 14, 2018, 12:13 a.m. | #12
On Tue, Feb 13, 2018 at 4:08 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Feb 13, 2018 at 3:53 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>> This is a nice idea and I am able to make this work for defsym uses.

>>> Anything I could do for definitions too?  I could add a defined_ field

>>> in Symbol which is set to Defined::DEFSYM.  That would avoid

>>> is_defsym_def too.

>>

>> I'd think Symbol::source() = Symbol::IS_CONSTANT would be the test you

>> need. That would also catch symbols defined in scripts, too.


It is too late for plugins before this gets done actually.
define_script_symbols gets called in gold.cc much after deferred
objects are processed.


>

> I was looking at this.  This doesn't work when bar is declared in

> foo.cc and then defined using defsym.  For example:

>

> extern "C" {

> int foo() {

>   return 27;

> }

> extern int bar();

> int main() {

>   return bar();

> }

> }

>

> $ gcc  -Wl,--defsym,bar=foo -Wl,--plugin,gold/testsuite/plugin_test.so

>  foo.o.syms

>

> bar is FROM_OBJECT.

>

>

>>

>> -cary
Jose E. Marchesi via Binutils Feb. 14, 2018, 12:49 a.m. | #13
On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

> >> Do you have a real-world example? I'm having trouble imagining a case

> >> where --defsym would be used to override a symbol that's subject to

> >> the ODR and yet remain a valid program.

> >

> > I just concocted one:

> > ...

> > With defsym:

> >

> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

> > -Wl,--defsym,_Z3barv=_Z3bazv

>

> To me, this is the same as providing an overriding definition of bar()

> that prints "in baz", which clearly violates the one-definition rule.


On what basis do you consider this a valid thing to do, to the extent
> that you want to preserve the unoptimized behavior across LTO?

>

> Is there a real-world example where someone would want to do this in

> production code? I'm afraid I'd have zero sympathy for them. If they

> want something like that to work, they should just turn off

> cross-module inlining.

>


Fair enough. It was a contrived example, not based on anything I have seen
so far. If that violates ODR then I agree all bets are off.

Let me try with a modified change that marks these with LDPR_PREEMPTED_REG.
Sri, would you mind changing the patch and I'll give the new version a try?

Thanks,
Teresa


> I need a lot more justification to extend the plugin API.

>

> -cary

>




-- 
Teresa Johnson |  Software Engineer |  tejohnson@google.com |  408-460-2413
Jose E. Marchesi via Binutils Feb. 14, 2018, 12:52 a.m. | #14
On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>
wrote:

>

>

> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>

>> >> Do you have a real-world example? I'm having trouble imagining a case

>> >> where --defsym would be used to override a symbol that's subject to

>> >> the ODR and yet remain a valid program.

>> >

>> > I just concocted one:

>> > ...

>> > With defsym:

>> >

>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>

>> To me, this is the same as providing an overriding definition of bar()

>> that prints "in baz", which clearly violates the one-definition rule.

>

> On what basis do you consider this a valid thing to do, to the extent

>> that you want to preserve the unoptimized behavior across LTO?

>>

>> Is there a real-world example where someone would want to do this in

>> production code? I'm afraid I'd have zero sympathy for them. If they

>> want something like that to work, they should just turn off

>> cross-module inlining.

>>

>

> Fair enough. It was a contrived example, not based on anything I have seen

> so far. If that violates ODR then I agree all bets are off.

>

> Let me try with a modified change that marks these with LDPR_PREEMPTED_REG.

> Sri, would you mind changing the patch and I'll give the new version a try?

>


No problem.


>

> Thanks,

> Teresa

>

>

>> I need a lot more justification to extend the plugin API.

>>

>> -cary

>>

>

>

>

> --

> Teresa Johnson |  Software Engineer |  tejohnson@google.com |

> 408-460-2413 <(408)%20460-2413>

>
Jose E. Marchesi via Binutils Feb. 14, 2018, 1:05 a.m. | #15
New patch attached with all changes made.

gold/
* expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
      New method.
(Unary_expression::set_expr_sym_in_real_elf): New method.
(Binary_expression::set_expr_sym_in_real_elf): New method.
(Trinary_expression::set_expr_sym_in_real_elf): New method.
* plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
defined or used in defsyms.
* script.cc (set_defsym_uses_in_real_elf): New method.
(Script_options::is_defsym_def): New method.
* script.h (Expression::set_expr_sym_in_real_elf): New method.
(Symbol_assignment::is_defsym): New method.
(Symbol_assignment::value): New method.
(Script_options::is_defsym_def): New method.
(Script_options::set_defsym_uses_in_real_elf): New method.

On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>

>

> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

> wrote:

>>

>>

>>

>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>> wrote:

>>>

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>

>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>> >> where --defsym would be used to override a symbol that's subject to

>>>> >> the ODR and yet remain a valid program.

>>>> >

>>>> > I just concocted one:

>>>> > ...

>>>> > With defsym:

>>>> >

>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>

>>>> To me, this is the same as providing an overriding definition of bar()

>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>

>>>> On what basis do you consider this a valid thing to do, to the extent

>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>

>>>> Is there a real-world example where someone would want to do this in

>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>> want something like that to work, they should just turn off

>>>> cross-module inlining.

>>>

>>>

>>> Fair enough. It was a contrived example, not based on anything I have

>>> seen so far. If that violates ODR then I agree all bets are off.

>>>

>>> Let me try with a modified change that marks these with

>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>> new version a try?

>

>

>

> New patch attached.

>

>

> gold/

> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * script.cc (set_defsym_uses_in_real_elf): New method.

> (Script_options::is_defsym_def): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

> * testsuite/Makefile.am (plugin_test_defsym): New test.

> * testsuite/Makefile.in: Regenerate.

> * testsuite/plugin_test.c: Check for new symbol resolution.

> * testsuite/plugin_test_defsym.sh: New script.

> * testsuite/plugin_test_defsym.c: New test source.

>

>

>

>>

>>

>> No problem.

>>

>>>

>>>

>>> Thanks,

>>> Teresa

>>>

>>>>

>>>> I need a lot more justification to extend the plugin API.

>>>>

>>>> -cary

>>>

>>>

>>>

>>>

>>> --

>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>

>>

>
gold/
	* expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
      	New method.
	(Unary_expression::set_expr_sym_in_real_elf): New method.
	(Binary_expression::set_expr_sym_in_real_elf): New method.
	(Trinary_expression::set_expr_sym_in_real_elf): New method.
	* plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
	defined or used in defsyms.
	* script.cc (set_defsym_uses_in_real_elf): New method.
	(Script_options::is_defsym_def): New method.
	* script.h (Expression::set_expr_sym_in_real_elf): New method.
	(Symbol_assignment::is_defsym): New method.
	(Symbol_assignment::value): New method.
	(Script_options::is_defsym_def): New method.
	(Script_options::set_defsym_uses_in_real_elf): New method.
	* testsuite/Makefile.am (plugin_test_defsym): New test.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/plugin_test.c: Check for new symbol resolution.
	* testsuite/plugin_test_defsym.sh: New script.
	* testsuite/plugin_test_defsym.c: New test source.
	
diff --git a/gold/expression.cc b/gold/expression.cc
index d764cc2a9d..ff807756b9 100644
--- a/gold/expression.cc
+++ b/gold/expression.cc
@@ -205,6 +205,13 @@ class Symbol_expression : public Expression
   uint64_t
   value(const Expression_eval_info*);
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    Symbol* sym = symtab->lookup(this->name_.c_str());
+    sym->set_in_real_elf();
+  }
+
   void
   print(FILE* f) const
   { fprintf(f, "%s", this->name_.c_str()); }
@@ -318,6 +325,10 @@ class Unary_expression : public Expression
   arg_print(FILE* f) const
   { this->arg_->print(f); }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  { return this->arg_->set_expr_sym_in_real_elf(symtab); }
+
  private:
   Expression* arg_;
 };
@@ -437,6 +448,13 @@ class Binary_expression : public Expression
     fprintf(f, ")");
   }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    this->left_->set_expr_sym_in_real_elf(symtab);
+    this->right_->set_expr_sym_in_real_elf(symtab);
+  }
+
  private:
   Expression* left_;
   Expression* right_;
@@ -622,6 +640,14 @@ class Trinary_expression : public Expression
   arg3_print(FILE* f) const
   { this->arg3_->print(f); }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    this->arg1_->set_expr_sym_in_real_elf(symtab);
+    this->arg2_->set_expr_sym_in_real_elf(symtab);
+    this->arg3_->set_expr_sym_in_real_elf(symtab);
+  }
+
  private:
   Expression* arg1_;
   Expression* arg2_;
diff --git a/gold/plugin.cc b/gold/plugin.cc
index 02fef25dda..9c14b22c40 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -989,6 +989,9 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
       return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
     }
 
+  Layout *layout = parameters->options().plugins()->layout();
+  layout->script_options()->set_defsym_uses_in_real_elf(symtab);
+
   for (int i = 0; i < nsyms; i++)
     {
       ld_plugin_symbol* isym = &syms[i];
@@ -997,9 +1000,16 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
         lsym = symtab->resolve_forwards(lsym);
       ld_plugin_symbol_resolution res = LDPR_UNKNOWN;
 
-      if (lsym->is_undefined())
-        // The symbol remains undefined.
-        res = LDPR_UNDEF;
+      if (layout->script_options()->is_defsym_def(lsym->name()))
+	{
+	  // The symbol is redefined via defsym.
+	  res = LDPR_PREEMPTED_REG;
+	}
+      else if (lsym->is_undefined())
+	{
+            // The symbol remains undefined.
+            res = LDPR_UNDEF;
+	}
       else if (isym->def == LDPK_UNDEF
                || isym->def == LDPK_WEAKUNDEF
                || isym->def == LDPK_COMMON)
diff --git a/gold/script.cc b/gold/script.cc
index db243cf435..8aac6e97b2 100644
--- a/gold/script.cc
+++ b/gold/script.cc
@@ -1112,6 +1112,30 @@ Script_options::is_pending_assignment(const char* name)
   return false;
 }
 
+// Returns true if symbol with NAME is redefined by defsym.
+
+bool Script_options::is_defsym_def(const char* name) const
+{
+  for (Symbol_assignments::const_iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    if ((*p)->is_defsym() && (*p)->name() == name)
+      return true;
+  return false;
+}
+
+void
+Script_options::set_defsym_uses_in_real_elf(Symbol_table* symtab) const
+{
+  for (Symbol_assignments::const_iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym())
+        (*p)->value()->set_expr_sym_in_real_elf(symtab);
+    }
+}
+
 // Add a symbol to be defined.
 
 void
diff --git a/gold/script.h b/gold/script.h
index ec8ce8e110..46bad76888 100644
--- a/gold/script.h
+++ b/gold/script.h
@@ -129,13 +129,17 @@ class Expression
   virtual uint64_t
   value(const Expression_eval_info*) = 0;
 
+  // Sets all symbols used in expressions as seen in a real ELF object.
+  virtual void
+  set_expr_sym_in_real_elf(Symbol_table*) const
+  { return; }
+
  private:
   // May not be copied.
   Expression(const Expression&);
   Expression& operator=(const Expression&);
 };
 
-
 // Version_script_info stores information parsed from the version
 // script, either provided by --version-script or as part of a linker
 // script.  A single Version_script_info object per target is owned by
@@ -344,6 +348,14 @@ class Symbol_assignment
   void
   finalize(Symbol_table*, const Layout*);
 
+  bool
+  is_defsym() const
+  { return is_defsym_; }
+
+  Expression *
+  value() const
+  { return val_; }
+
   // Finalize the symbol value when it can refer to the dot symbol.
   void
   finalize_with_dot(Symbol_table*, const Layout*, uint64_t dot_value,
@@ -454,6 +466,13 @@ class Script_options
   bool
   define_symbol(const char* definition);
 
+  // Returns true if symbol is defined via defsym.
+  bool
+  is_defsym_def(const char *name) const;
+
+  // Set symbols used in defsym expressions as seen in a real ELF object.
+  void set_defsym_uses_in_real_elf(Symbol_table*) const;
+
   // Create sections required by any linker scripts.
   void
   create_script_sections(Layout*);
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 16cae8004c..7ec3e8d88f 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -2332,11 +2332,22 @@ plugin_test_start_lib: unused.o plugin_start_lib_test.o plugin_start_lib_test_2.
 plugin_test_start_lib.err: plugin_test_start_lib
 	@touch plugin_test_start_lib.err
 
+check_PROGRAMS += plugin_test_defsym
+check_SCRIPTS += plugin_test_defsym.sh
+check_DATA += plugin_test_defsym.err
+MOSTLYCLEANFILES += plugin_test_defsym.err
+plugin_test_defsym.syms: plugin_test_defsym.o
+	$(TEST_READELF) -sW $< >$@ 2>/dev/null
+plugin_test_defsym.o: plugin_test_defsym.c
+	$(COMPILE) -c -o $@ $<
+plugin_test_defsym: plugin_test_defsym.o plugin_test_defsym.syms gcctestdir/ld plugin_test.so
+	$(CXXLINK) -Bgcctestdir/ -Wl,--no-demangle,--plugin,"./plugin_test.so" -Wl,--defsym,bar=foo plugin_test_defsym.syms 2>plugin_test_defsym.err
+plugin_test_defsym.err: plugin_test_defsym
+	@touch plugin_test_defsym.err
 
 plugin_start_lib_test_2.syms: plugin_start_lib_test_2.o
 	$(TEST_READELF) -sW $< >$@ 2>/dev/null
 
-
 plugin_test.so: plugin_test.o gcctestdir/ld
 	$(LINK) -Bgcctestdir/ -shared plugin_test.o
 plugin_test.o: plugin_test.c
diff --git a/gold/testsuite/plugin_test_defsym.c b/gold/testsuite/plugin_test_defsym.c
index e69de29bb2..a3c3b962f3 100644
--- a/gold/testsuite/plugin_test_defsym.c
+++ b/gold/testsuite/plugin_test_defsym.c
@@ -0,0 +1,32 @@
+/* plugin_test_defsym.c -- a test case for gold
+
+   Copyright (C) 2018 onwards Free Software Foundation, Inc.
+   Written by Sriraman Tallam <tmsriram@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.  */
+
+int foo(void);
+int foo(void) {
+  return 0;
+}
+
+int bar(void);
+
+int main(void) {
+  return bar();  
+}
diff --git a/gold/testsuite/plugin_test_defsym.sh b/gold/testsuite/plugin_test_defsym.sh
index e69de29bb2..6066f9b0c9 100755
--- a/gold/testsuite/plugin_test_defsym.sh
+++ b/gold/testsuite/plugin_test_defsym.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+# plugin_test_defsym.sh -- a test case for the plugin API.
+
+# Copyright (C) 2018 onwards Free Software Foundation, Inc.
+# Written by Sriraman Tallam <tmsriram@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.
+
+# This file goes with plugin_test.c, a simple plug-in library that
+# exercises the basic interfaces and prints out version numbers and
+# options passed to the plugin.
+
+# This checks if the symbol resolution withe export dynamic symbol is
+# as expected.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected output in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check plugin_test_defsym.err "API version:"
+check plugin_test_defsym.err "gold version:"
+check plugin_test_defsym.err "plugin_test_defsym.syms: claim file hook called"
+check plugin_test_defsym.err "plugin_test_defsym.syms: bar: PREEMPTED_REG"
+check plugin_test_defsym.err "plugin_test_defsym.syms: foo: PREVAILING_DEF_REG"
+check plugin_test_defsym.err "cleanup hook called"
+
+exit 0
Jose E. Marchesi via Binutils Feb. 14, 2018, 4:15 a.m. | #16
On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> New patch attached with all changes made.


I just realized I forgot to version get_symbols, I will do that asap.

>

> gold/

> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * script.cc (set_defsym_uses_in_real_elf): New method.

> (Script_options::is_defsym_def): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

>

> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>

>>

>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>> wrote:

>>>

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>> wrote:

>>>>

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>

>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>> >> the ODR and yet remain a valid program.

>>>>> >

>>>>> > I just concocted one:

>>>>> > ...

>>>>> > With defsym:

>>>>> >

>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>

>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>

>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>

>>>>> Is there a real-world example where someone would want to do this in

>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>> want something like that to work, they should just turn off

>>>>> cross-module inlining.

>>>>

>>>>

>>>> Fair enough. It was a contrived example, not based on anything I have

>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>

>>>> Let me try with a modified change that marks these with

>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>> new version a try?

>>

>>

>>

>> New patch attached.

>>

>>

>> gold/

>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * script.cc (set_defsym_uses_in_real_elf): New method.

>> (Script_options::is_defsym_def): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>> * testsuite/Makefile.in: Regenerate.

>> * testsuite/plugin_test.c: Check for new symbol resolution.

>> * testsuite/plugin_test_defsym.sh: New script.

>> * testsuite/plugin_test_defsym.c: New test source.

>>

>>

>>

>>>

>>>

>>> No problem.

>>>

>>>>

>>>>

>>>> Thanks,

>>>> Teresa

>>>>

>>>>>

>>>>> I need a lot more justification to extend the plugin API.

>>>>>

>>>>> -cary

>>>>

>>>>

>>>>

>>>>

>>>> --

>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>

>>>

>>
Cary Coutant Feb. 14, 2018, 5:48 a.m. | #17
>> New patch attached with all changes made.

>

> I just realized I forgot to version get_symbols, I will do that asap.


Your new patch doesn't require a new version.

-cary
Cary Coutant Feb. 14, 2018, 5:59 a.m. | #18
@@ -989,6 +989,9 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
       return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
     }

+  Layout *layout = parameters->options().plugins()->layout();
+  layout->script_options()->set_defsym_uses_in_real_elf(symtab);

This is going to run set_defsym_uses_in_real_elf() every time the
get_symbols API is called -- i.e., once for each input object. It only
needs to be done once, so I think it would be appropriate to call this
from Plugin_manager::all_symbols_read().

If you're still worried about the performance of is_defsym_def(),
all_symbols_read() might also be a good time to build a quick little
hash table of all the symbols on the LHS of an assignment.

-cary


On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> New patch attached with all changes made.

>

> gold/

> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * script.cc (set_defsym_uses_in_real_elf): New method.

> (Script_options::is_defsym_def): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

>

> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>

>>

>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>> wrote:

>>>

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>> wrote:

>>>>

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>

>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>> >> the ODR and yet remain a valid program.

>>>>> >

>>>>> > I just concocted one:

>>>>> > ...

>>>>> > With defsym:

>>>>> >

>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>

>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>

>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>

>>>>> Is there a real-world example where someone would want to do this in

>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>> want something like that to work, they should just turn off

>>>>> cross-module inlining.

>>>>

>>>>

>>>> Fair enough. It was a contrived example, not based on anything I have

>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>

>>>> Let me try with a modified change that marks these with

>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>> new version a try?

>>

>>

>>

>> New patch attached.

>>

>>

>> gold/

>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * script.cc (set_defsym_uses_in_real_elf): New method.

>> (Script_options::is_defsym_def): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>> * testsuite/Makefile.in: Regenerate.

>> * testsuite/plugin_test.c: Check for new symbol resolution.

>> * testsuite/plugin_test_defsym.sh: New script.

>> * testsuite/plugin_test_defsym.c: New test source.

>>

>>

>>

>>>

>>>

>>> No problem.

>>>

>>>>

>>>>

>>>> Thanks,

>>>> Teresa

>>>>

>>>>>

>>>>> I need a lot more justification to extend the plugin API.

>>>>>

>>>>> -cary

>>>>

>>>>

>>>>

>>>>

>>>> --

>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>

>>>

>>
Cary Coutant Feb. 14, 2018, 6:14 a.m. | #19
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    Symbol* sym = symtab->lookup(this->name_.c_str());
+    sym->set_in_real_elf();
+  }

Since you pointed out that this is running before
define_script_symbols(), I now realize that some of these symbols
might not be in the symbol table, and Symbol_table::lookup() can
return NULL. I think if that's the case, though, the symbol is not
referenced from IR, so it is unimportant, and you can just ignore
trying to set the in_real_elf flag if sym == NULL. Agreed?

-cary



On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> New patch attached with all changes made.

>

> gold/

> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * script.cc (set_defsym_uses_in_real_elf): New method.

> (Script_options::is_defsym_def): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::is_defsym_def): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

>

> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>

>>

>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>> wrote:

>>>

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>> wrote:

>>>>

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>

>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>> >> the ODR and yet remain a valid program.

>>>>> >

>>>>> > I just concocted one:

>>>>> > ...

>>>>> > With defsym:

>>>>> >

>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>

>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>

>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>

>>>>> Is there a real-world example where someone would want to do this in

>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>> want something like that to work, they should just turn off

>>>>> cross-module inlining.

>>>>

>>>>

>>>> Fair enough. It was a contrived example, not based on anything I have

>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>

>>>> Let me try with a modified change that marks these with

>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>> new version a try?

>>

>>

>>

>> New patch attached.

>>

>>

>> gold/

>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * script.cc (set_defsym_uses_in_real_elf): New method.

>> (Script_options::is_defsym_def): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>> * testsuite/Makefile.in: Regenerate.

>> * testsuite/plugin_test.c: Check for new symbol resolution.

>> * testsuite/plugin_test_defsym.sh: New script.

>> * testsuite/plugin_test_defsym.c: New test source.

>>

>>

>>

>>>

>>>

>>> No problem.

>>>

>>>>

>>>>

>>>> Thanks,

>>>> Teresa

>>>>

>>>>>

>>>>> I need a lot more justification to extend the plugin API.

>>>>>

>>>>> -cary

>>>>

>>>>

>>>>

>>>>

>>>> --

>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>

>>>

>>
Jose E. Marchesi via Binutils Feb. 14, 2018, 6:05 p.m. | #20
On Tue, Feb 13, 2018 at 9:59 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> @@ -989,6 +989,9 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,

>        return version > 2 ? LDPS_NO_SYMS : LDPS_OK;

>      }

>

> +  Layout *layout = parameters->options().plugins()->layout();

> +  layout->script_options()->set_defsym_uses_in_real_elf(symtab);

>

> This is going to run set_defsym_uses_in_real_elf() every time the

> get_symbols API is called -- i.e., once for each input object. It only

> needs to be done once, so I think it would be appropriate to call this

> from Plugin_manager::all_symbols_read().


I did think of this :) added a check and then removed it as I thought
there is no real reason to call get_symbol_resolution_info more than
once.  Now looking at the plugin, I see this can be called for every
claimed file, sorry about that.

>

> If you're still worried about the performance of is_defsym_def(),

> all_symbols_read() might also be a good time to build a quick little

> hash table of all the symbols on the LHS of an assignment.


I fear the performance could be bad with heavy use of linker scripts
but I have no idea on how long Script_options::symbol_assignments_
could potentially get.

Thanks
Sri


>

> -cary

>

>

> On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>> New patch attached with all changes made.

>>

>> gold/

>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * script.cc (set_defsym_uses_in_real_elf): New method.

>> (Script_options::is_defsym_def): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>

>> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>>> wrote:

>>>>

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>>> wrote:

>>>>>

>>>>>

>>>>>

>>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>>

>>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>>> >> the ODR and yet remain a valid program.

>>>>>> >

>>>>>> > I just concocted one:

>>>>>> > ...

>>>>>> > With defsym:

>>>>>> >

>>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>>

>>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>>

>>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>>

>>>>>> Is there a real-world example where someone would want to do this in

>>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>>> want something like that to work, they should just turn off

>>>>>> cross-module inlining.

>>>>>

>>>>>

>>>>> Fair enough. It was a contrived example, not based on anything I have

>>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>>

>>>>> Let me try with a modified change that marks these with

>>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>>> new version a try?

>>>

>>>

>>>

>>> New patch attached.

>>>

>>>

>>> gold/

>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>>       New method.

>>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>>> defined or used in defsyms.

>>> * script.cc (set_defsym_uses_in_real_elf): New method.

>>> (Script_options::is_defsym_def): New method.

>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>>> (Symbol_assignment::is_defsym): New method.

>>> (Symbol_assignment::value): New method.

>>> (Script_options::is_defsym_def): New method.

>>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>>> * testsuite/Makefile.in: Regenerate.

>>> * testsuite/plugin_test.c: Check for new symbol resolution.

>>> * testsuite/plugin_test_defsym.sh: New script.

>>> * testsuite/plugin_test_defsym.c: New test source.

>>>

>>>

>>>

>>>>

>>>>

>>>> No problem.

>>>>

>>>>>

>>>>>

>>>>> Thanks,

>>>>> Teresa

>>>>>

>>>>>>

>>>>>> I need a lot more justification to extend the plugin API.

>>>>>>

>>>>>> -cary

>>>>>

>>>>>

>>>>>

>>>>>

>>>>> --

>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>>

>>>>

>>>
Jose E. Marchesi via Binutils Feb. 14, 2018, 6:29 p.m. | #21
On Tue, Feb 13, 2018 at 10:14 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> +  void

> +  set_expr_sym_in_real_elf(Symbol_table* symtab) const

> +  {

> +    Symbol* sym = symtab->lookup(this->name_.c_str());

> +    sym->set_in_real_elf();

> +  }

>

> Since you pointed out that this is running before

> define_script_symbols(), I now realize that some of these symbols

> might not be in the symbol table, and Symbol_table::lookup() can

> return NULL. I think if that's the case, though, the symbol is not

> referenced from IR, so it is unimportant, and you can just ignore

> trying to set the in_real_elf flag if sym == NULL. Agreed?


Yes, thanks for pointing this out.  Will make that change, thanks!

>

> -cary

>

>

>

> On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>> New patch attached with all changes made.

>>

>> gold/

>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * script.cc (set_defsym_uses_in_real_elf): New method.

>> (Script_options::is_defsym_def): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::is_defsym_def): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>

>> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>>

>>>

>>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>>> wrote:

>>>>

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>>> wrote:

>>>>>

>>>>>

>>>>>

>>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>>

>>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>>> >> the ODR and yet remain a valid program.

>>>>>> >

>>>>>> > I just concocted one:

>>>>>> > ...

>>>>>> > With defsym:

>>>>>> >

>>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>>

>>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>>

>>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>>

>>>>>> Is there a real-world example where someone would want to do this in

>>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>>> want something like that to work, they should just turn off

>>>>>> cross-module inlining.

>>>>>

>>>>>

>>>>> Fair enough. It was a contrived example, not based on anything I have

>>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>>

>>>>> Let me try with a modified change that marks these with

>>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>>> new version a try?

>>>

>>>

>>>

>>> New patch attached.

>>>

>>>

>>> gold/

>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>>       New method.

>>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>>> defined or used in defsyms.

>>> * script.cc (set_defsym_uses_in_real_elf): New method.

>>> (Script_options::is_defsym_def): New method.

>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>>> (Symbol_assignment::is_defsym): New method.

>>> (Symbol_assignment::value): New method.

>>> (Script_options::is_defsym_def): New method.

>>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>>> * testsuite/Makefile.in: Regenerate.

>>> * testsuite/plugin_test.c: Check for new symbol resolution.

>>> * testsuite/plugin_test_defsym.sh: New script.

>>> * testsuite/plugin_test_defsym.c: New test source.

>>>

>>>

>>>

>>>>

>>>>

>>>> No problem.

>>>>

>>>>>

>>>>>

>>>>> Thanks,

>>>>> Teresa

>>>>>

>>>>>>

>>>>>> I need a lot more justification to extend the plugin API.

>>>>>>

>>>>>> -cary

>>>>>

>>>>>

>>>>>

>>>>>

>>>>> --

>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>>

>>>>

>>>
Jose E. Marchesi via Binutils Feb. 14, 2018, 8:57 p.m. | #22
New patch attached.


* expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
      New method.
(Unary_expression::set_expr_sym_in_real_elf): New method.
(Binary_expression::set_expr_sym_in_real_elf): New method.
(Trinary_expression::set_expr_sym_in_real_elf): New method.
* plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
defined or used in defsyms.
* plugin.h (Plugin_manager::is_defsym_def): New method.
(Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.
(Plugin_manager::defsym_defines_set_): New member.
(Plugin_manager::Defsym_defines_set): New typedef.
* script.cc (Script_options::set_defsym_uses_in_real_elf): New method.
(Script_options::find_defsym_defs): New method.
* script.h (Expression::set_expr_sym_in_real_elf): New method.
(Symbol_assignment::is_defsym): New method.
(Symbol_assignment::value): New method.
(Script_options::find_defsym_defs): New method.
(Script_options::set_defsym_uses_in_real_elf): New method.
* testsuite/Makefile.am (plugin_test_defsym): New test.
* testsuite/Makefile.in: Regenerate.
* testsuite/plugin_test.c: Check for new symbol resolution.
* testsuite/plugin_test_defsym.sh: New script.
* testsuite/plugin_test_defsym.c: New test source.


On Wed, Feb 14, 2018 at 10:29 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Feb 13, 2018 at 10:14 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>> +  void

>> +  set_expr_sym_in_real_elf(Symbol_table* symtab) const

>> +  {

>> +    Symbol* sym = symtab->lookup(this->name_.c_str());

>> +    sym->set_in_real_elf();

>> +  }

>>

>> Since you pointed out that this is running before

>> define_script_symbols(), I now realize that some of these symbols

>> might not be in the symbol table, and Symbol_table::lookup() can

>> return NULL. I think if that's the case, though, the symbol is not

>> referenced from IR, so it is unimportant, and you can just ignore

>> trying to set the in_real_elf flag if sym == NULL. Agreed?

>

> Yes, thanks for pointing this out.  Will make that change, thanks!

>

>>

>> -cary

>>

>>

>>

>> On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>> New patch attached with all changes made.

>>>

>>> gold/

>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>>       New method.

>>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>>> defined or used in defsyms.

>>> * script.cc (set_defsym_uses_in_real_elf): New method.

>>> (Script_options::is_defsym_def): New method.

>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>>> (Symbol_assignment::is_defsym): New method.

>>> (Symbol_assignment::value): New method.

>>> (Script_options::is_defsym_def): New method.

>>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>>

>>> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com> wrote:

>>>>

>>>>

>>>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com>

>>>> wrote:

>>>>>

>>>>>

>>>>>

>>>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <tejohnson@google.com>

>>>>> wrote:

>>>>>>

>>>>>>

>>>>>>

>>>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:

>>>>>>>

>>>>>>> >> Do you have a real-world example? I'm having trouble imagining a case

>>>>>>> >> where --defsym would be used to override a symbol that's subject to

>>>>>>> >> the ODR and yet remain a valid program.

>>>>>>> >

>>>>>>> > I just concocted one:

>>>>>>> > ...

>>>>>>> > With defsym:

>>>>>>> >

>>>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

>>>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

>>>>>>>

>>>>>>> To me, this is the same as providing an overriding definition of bar()

>>>>>>> that prints "in baz", which clearly violates the one-definition rule.

>>>>>>>

>>>>>>> On what basis do you consider this a valid thing to do, to the extent

>>>>>>> that you want to preserve the unoptimized behavior across LTO?

>>>>>>>

>>>>>>> Is there a real-world example where someone would want to do this in

>>>>>>> production code? I'm afraid I'd have zero sympathy for them. If they

>>>>>>> want something like that to work, they should just turn off

>>>>>>> cross-module inlining.

>>>>>>

>>>>>>

>>>>>> Fair enough. It was a contrived example, not based on anything I have

>>>>>> seen so far. If that violates ODR then I agree all bets are off.

>>>>>>

>>>>>> Let me try with a modified change that marks these with

>>>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll give the

>>>>>> new version a try?

>>>>

>>>>

>>>>

>>>> New patch attached.

>>>>

>>>>

>>>> gold/

>>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>>>       New method.

>>>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>>>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>>>> defined or used in defsyms.

>>>> * script.cc (set_defsym_uses_in_real_elf): New method.

>>>> (Script_options::is_defsym_def): New method.

>>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>>>> (Symbol_assignment::is_defsym): New method.

>>>> (Symbol_assignment::value): New method.

>>>> (Script_options::is_defsym_def): New method.

>>>> (Script_options::set_defsym_uses_in_real_elf): New method.

>>>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>>>> * testsuite/Makefile.in: Regenerate.

>>>> * testsuite/plugin_test.c: Check for new symbol resolution.

>>>> * testsuite/plugin_test_defsym.sh: New script.

>>>> * testsuite/plugin_test_defsym.c: New test source.

>>>>

>>>>

>>>>

>>>>>

>>>>>

>>>>> No problem.

>>>>>

>>>>>>

>>>>>>

>>>>>> Thanks,

>>>>>> Teresa

>>>>>>

>>>>>>>

>>>>>>> I need a lot more justification to extend the plugin API.

>>>>>>>

>>>>>>> -cary

>>>>>>

>>>>>>

>>>>>>

>>>>>>

>>>>>> --

>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

>>>>>

>>>>>

>>>>
* expression.cc (Symbol_expression::set_expr_sym_in_real_elf):
      	New method.
	(Unary_expression::set_expr_sym_in_real_elf): New method.
	(Binary_expression::set_expr_sym_in_real_elf): New method.
	(Trinary_expression::set_expr_sym_in_real_elf): New method.
	* plugin.cc (get_symbol_resolution_info): Fix symbol resolution if
	defined or used in defsyms.
	* plugin.h (Plugin_manager::is_defsym_def): New method.
	(Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.
	(Plugin_manager::defsym_defines_set_): New member.
	(Plugin_manager::Defsym_defines_set): New typedef.
	* script.cc (Script_options::set_defsym_uses_in_real_elf): New method.
	(Script_options::find_defsym_defs): New method.
	* script.h (Expression::set_expr_sym_in_real_elf): New method.
	(Symbol_assignment::is_defsym): New method.
	(Symbol_assignment::value): New method.
	(Script_options::find_defsym_defs): New method.
	(Script_options::set_defsym_uses_in_real_elf): New method.
	* testsuite/Makefile.am (plugin_test_defsym): New test.
	* testsuite/Makefile.in: Regenerate.
	* testsuite/plugin_test.c: Check for new symbol resolution.
	* testsuite/plugin_test_defsym.sh: New script.
	* testsuite/plugin_test_defsym.c: New test source.
	
diff --git a/gold/expression.cc b/gold/expression.cc
index d764cc2a9d..bbfaa1ee16 100644
--- a/gold/expression.cc
+++ b/gold/expression.cc
@@ -205,6 +205,14 @@ class Symbol_expression : public Expression
   uint64_t
   value(const Expression_eval_info*);
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    Symbol* sym = symtab->lookup(this->name_.c_str());
+    if (sym != NULL)
+      sym->set_in_real_elf();
+  }
+
   void
   print(FILE* f) const
   { fprintf(f, "%s", this->name_.c_str()); }
@@ -318,6 +326,10 @@ class Unary_expression : public Expression
   arg_print(FILE* f) const
   { this->arg_->print(f); }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  { return this->arg_->set_expr_sym_in_real_elf(symtab); }
+
  private:
   Expression* arg_;
 };
@@ -437,6 +449,13 @@ class Binary_expression : public Expression
     fprintf(f, ")");
   }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    this->left_->set_expr_sym_in_real_elf(symtab);
+    this->right_->set_expr_sym_in_real_elf(symtab);
+  }
+
  private:
   Expression* left_;
   Expression* right_;
@@ -622,6 +641,14 @@ class Trinary_expression : public Expression
   arg3_print(FILE* f) const
   { this->arg3_->print(f); }
 
+  void
+  set_expr_sym_in_real_elf(Symbol_table* symtab) const
+  {
+    this->arg1_->set_expr_sym_in_real_elf(symtab);
+    this->arg2_->set_expr_sym_in_real_elf(symtab);
+    this->arg3_->set_expr_sym_in_real_elf(symtab);
+  }
+
  private:
   Expression* arg1_;
   Expression* arg2_;
diff --git a/gold/plugin.cc b/gold/plugin.cc
index 02fef25dda..566f1b7cae 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -580,6 +580,11 @@ Plugin_manager::all_symbols_read(Workqueue* workqueue, Task* task,
   this->mapfile_ = mapfile;
   this->this_blocker_ = NULL;
 
+  // Set symbols used in defsym expressions as seen in real ELF.
+  Layout *layout = parameters->options().plugins()->layout();
+  layout->script_options()->set_defsym_uses_in_real_elf(symtab);
+  layout->script_options()->find_defsym_defs(this->defsym_defines_set_);
+
   for (this->current_ = this->plugins_.begin();
        this->current_ != this->plugins_.end();
        ++this->current_)
@@ -989,6 +994,7 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
       return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
     }
 
+  Plugin_manager* plugins = parameters->options().plugins();
   for (int i = 0; i < nsyms; i++)
     {
       ld_plugin_symbol* isym = &syms[i];
@@ -997,9 +1003,16 @@ Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
         lsym = symtab->resolve_forwards(lsym);
       ld_plugin_symbol_resolution res = LDPR_UNKNOWN;
 
-      if (lsym->is_undefined())
-        // The symbol remains undefined.
-        res = LDPR_UNDEF;
+      if (plugins->is_defsym_def(lsym->name()))
+	{
+	  // The symbol is redefined via defsym.
+	  res = LDPR_PREEMPTED_REG;
+	}
+      else if (lsym->is_undefined())
+	{
+          // The symbol remains undefined.
+          res = LDPR_UNDEF;
+	}
       else if (isym->def == LDPK_UNDEF
                || isym->def == LDPK_WEAKUNDEF
                || isym->def == LDPK_COMMON)
diff --git a/gold/plugin.h b/gold/plugin.h
index db6093daa7..e64ee07bb6 100644
--- a/gold/plugin.h
+++ b/gold/plugin.h
@@ -146,11 +146,18 @@ class Plugin_manager
       options_(options), workqueue_(NULL), task_(NULL), input_objects_(NULL),
       symtab_(NULL), layout_(NULL), dirpath_(NULL), mapfile_(NULL),
       this_blocker_(NULL), extra_search_path_(), lock_(NULL),
-      initialize_lock_(&lock_)
+      initialize_lock_(&lock_), defsym_defines_set_()
   { this->current_ = plugins_.end(); }
 
   ~Plugin_manager();
 
+  // Returns true if the symbol name is used in the LHS of a defsym.
+  bool
+  is_defsym_def(const char* sym_name) const
+  {
+    return defsym_defines_set_.find(sym_name) != defsym_defines_set_.end();
+  }
+
   // Add a plugin library.
   void
   add_plugin(const char* filename)
@@ -402,6 +409,10 @@ class Plugin_manager
   std::string extra_search_path_;
   Lock* lock_;
   Initialize_lock initialize_lock_;
+
+  // Keep track of all symbols defined by defsym.
+  typedef Unordered_set<std::string> Defsym_defines_set;
+  Defsym_defines_set defsym_defines_set_;
 };
 
 
diff --git a/gold/script.cc b/gold/script.cc
index db243cf435..75b877daa2 100644
--- a/gold/script.cc
+++ b/gold/script.cc
@@ -1112,6 +1112,31 @@ Script_options::is_pending_assignment(const char* name)
   return false;
 }
 
+// Populates the set with symbols used in defsym LHS.
+
+void Script_options::find_defsym_defs(Unordered_set<std::string>& defsym_set)
+{
+  for (Symbol_assignments::const_iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym())
+	defsym_set.insert((*p)->name());
+    }
+}
+
+void
+Script_options::set_defsym_uses_in_real_elf(Symbol_table* symtab) const
+{
+  for (Symbol_assignments::const_iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym())
+        (*p)->value()->set_expr_sym_in_real_elf(symtab);
+    }
+}
+
 // Add a symbol to be defined.
 
 void
diff --git a/gold/script.h b/gold/script.h
index ec8ce8e110..aaf825a786 100644
--- a/gold/script.h
+++ b/gold/script.h
@@ -129,13 +129,17 @@ class Expression
   virtual uint64_t
   value(const Expression_eval_info*) = 0;
 
+  // Sets all symbols used in expressions as seen in a real ELF object.
+  virtual void
+  set_expr_sym_in_real_elf(Symbol_table*) const
+  { return; }
+
  private:
   // May not be copied.
   Expression(const Expression&);
   Expression& operator=(const Expression&);
 };
 
-
 // Version_script_info stores information parsed from the version
 // script, either provided by --version-script or as part of a linker
 // script.  A single Version_script_info object per target is owned by
@@ -344,6 +348,14 @@ class Symbol_assignment
   void
   finalize(Symbol_table*, const Layout*);
 
+  bool
+  is_defsym() const
+  { return is_defsym_; }
+
+  Expression *
+  value() const
+  { return val_; }
+
   // Finalize the symbol value when it can refer to the dot symbol.
   void
   finalize_with_dot(Symbol_table*, const Layout*, uint64_t dot_value,
@@ -454,6 +466,13 @@ class Script_options
   bool
   define_symbol(const char* definition);
 
+  // Populates the set with symbol names used in LHS of defsym.
+  void
+  find_defsym_defs(Unordered_set<std::string>&);
+
+  // Set symbols used in defsym expressions as seen in a real ELF object.
+  void set_defsym_uses_in_real_elf(Symbol_table*) const;
+
   // Create sections required by any linker scripts.
   void
   create_script_sections(Layout*);
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 16cae8004c..7ec3e8d88f 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -2332,11 +2332,22 @@ plugin_test_start_lib: unused.o plugin_start_lib_test.o plugin_start_lib_test_2.
 plugin_test_start_lib.err: plugin_test_start_lib
 	@touch plugin_test_start_lib.err
 
+check_PROGRAMS += plugin_test_defsym
+check_SCRIPTS += plugin_test_defsym.sh
+check_DATA += plugin_test_defsym.err
+MOSTLYCLEANFILES += plugin_test_defsym.err
+plugin_test_defsym.syms: plugin_test_defsym.o
+	$(TEST_READELF) -sW $< >$@ 2>/dev/null
+plugin_test_defsym.o: plugin_test_defsym.c
+	$(COMPILE) -c -o $@ $<
+plugin_test_defsym: plugin_test_defsym.o plugin_test_defsym.syms gcctestdir/ld plugin_test.so
+	$(CXXLINK) -Bgcctestdir/ -Wl,--no-demangle,--plugin,"./plugin_test.so" -Wl,--defsym,bar=foo plugin_test_defsym.syms 2>plugin_test_defsym.err
+plugin_test_defsym.err: plugin_test_defsym
+	@touch plugin_test_defsym.err
 
 plugin_start_lib_test_2.syms: plugin_start_lib_test_2.o
 	$(TEST_READELF) -sW $< >$@ 2>/dev/null
 
-
 plugin_test.so: plugin_test.o gcctestdir/ld
 	$(LINK) -Bgcctestdir/ -shared plugin_test.o
 plugin_test.o: plugin_test.c
diff --git a/gold/testsuite/plugin_test_defsym.c b/gold/testsuite/plugin_test_defsym.c
index e69de29bb2..3b004c6520 100644
--- a/gold/testsuite/plugin_test_defsym.c
+++ b/gold/testsuite/plugin_test_defsym.c
@@ -0,0 +1,32 @@
+/* plugin_test_defsym.c -- a test case for gold
+
+   Copyright (C) 2018 onwards Free Software Foundation, Inc.
+   Written by Sriraman Tallam <tmsriram@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.  */
+
+int foo(void);
+int foo(void) {
+  return 0;
+}
+
+int bar(void);
+
+int main(void) {
+  return bar();
+}
diff --git a/gold/testsuite/plugin_test_defsym.sh b/gold/testsuite/plugin_test_defsym.sh
index e69de29bb2..6066f9b0c9 100755
--- a/gold/testsuite/plugin_test_defsym.sh
+++ b/gold/testsuite/plugin_test_defsym.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+# plugin_test_defsym.sh -- a test case for the plugin API.
+
+# Copyright (C) 2018 onwards Free Software Foundation, Inc.
+# Written by Sriraman Tallam <tmsriram@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.
+
+# This file goes with plugin_test.c, a simple plug-in library that
+# exercises the basic interfaces and prints out version numbers and
+# options passed to the plugin.
+
+# This checks if the symbol resolution withe export dynamic symbol is
+# as expected.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected output in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check plugin_test_defsym.err "API version:"
+check plugin_test_defsym.err "gold version:"
+check plugin_test_defsym.err "plugin_test_defsym.syms: claim file hook called"
+check plugin_test_defsym.err "plugin_test_defsym.syms: bar: PREEMPTED_REG"
+check plugin_test_defsym.err "plugin_test_defsym.syms: foo: PREVAILING_DEF_REG"
+check plugin_test_defsym.err "cleanup hook called"
+
+exit 0
Jose E. Marchesi via Binutils Feb. 15, 2018, 3:14 a.m. | #23
Confirmed this works for my LTO test cases (and no change required to the
LLVM gold plugin with this approach).

Thanks,
Teresa

On Wed, Feb 14, 2018 at 12:57 PM, Sriraman Tallam <tmsriram@google.com>
wrote:

> New patch attached.

>

>

> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * plugin.h (Plugin_manager::is_defsym_def): New method.

> (Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.

> (Plugin_manager::defsym_defines_set_): New member.

> (Plugin_manager::Defsym_defines_set): New typedef.

> * script.cc (Script_options::set_defsym_uses_in_real_elf): New method.

> (Script_options::find_defsym_defs): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::find_defsym_defs): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

> * testsuite/Makefile.am (plugin_test_defsym): New test.

> * testsuite/Makefile.in: Regenerate.

> * testsuite/plugin_test.c: Check for new symbol resolution.

> * testsuite/plugin_test_defsym.sh: New script.

> * testsuite/plugin_test_defsym.c: New test source.

>

>

> On Wed, Feb 14, 2018 at 10:29 AM, Sriraman Tallam <tmsriram@google.com>

> wrote:

> > On Tue, Feb 13, 2018 at 10:14 PM, Cary Coutant <ccoutant@gmail.com>

> wrote:

> >> +  void

> >> +  set_expr_sym_in_real_elf(Symbol_table* symtab) const

> >> +  {

> >> +    Symbol* sym = symtab->lookup(this->name_.c_str());

> >> +    sym->set_in_real_elf();

> >> +  }

> >>

> >> Since you pointed out that this is running before

> >> define_script_symbols(), I now realize that some of these symbols

> >> might not be in the symbol table, and Symbol_table::lookup() can

> >> return NULL. I think if that's the case, though, the symbol is not

> >> referenced from IR, so it is unimportant, and you can just ignore

> >> trying to set the in_real_elf flag if sym == NULL. Agreed?

> >

> > Yes, thanks for pointing this out.  Will make that change, thanks!

> >

> >>

> >> -cary

> >>

> >>

> >>

> >> On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <tmsriram@google.com>

> wrote:

> >>> New patch attached with all changes made.

> >>>

> >>> gold/

> >>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

> >>>       New method.

> >>> (Unary_expression::set_expr_sym_in_real_elf): New method.

> >>> (Binary_expression::set_expr_sym_in_real_elf): New method.

> >>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> >>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> >>> defined or used in defsyms.

> >>> * script.cc (set_defsym_uses_in_real_elf): New method.

> >>> (Script_options::is_defsym_def): New method.

> >>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> >>> (Symbol_assignment::is_defsym): New method.

> >>> (Symbol_assignment::value): New method.

> >>> (Script_options::is_defsym_def): New method.

> >>> (Script_options::set_defsym_uses_in_real_elf): New method.

> >>>

> >>> On Tue, Feb 13, 2018 at 5:03 PM, Sriraman Tallam <tmsriram@google.com>

> wrote:

> >>>>

> >>>>

> >>>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <tmsriram@google.com

> >

> >>>> wrote:

> >>>>>

> >>>>>

> >>>>>

> >>>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <

> tejohnson@google.com>

> >>>>> wrote:

> >>>>>>

> >>>>>>

> >>>>>>

> >>>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <ccoutant@gmail.com>

> wrote:

> >>>>>>>

> >>>>>>> >> Do you have a real-world example? I'm having trouble imagining

> a case

> >>>>>>> >> where --defsym would be used to override a symbol that's

> subject to

> >>>>>>> >> the ODR and yet remain a valid program.

> >>>>>>> >

> >>>>>>> > I just concocted one:

> >>>>>>> > ...

> >>>>>>> > With defsym:

> >>>>>>> >

> >>>>>>> > $ ~/llvm/llvm_8_build/bin/clang++ hello[12].cc  -fuse-ld=gold

> >>>>>>> > -Wl,--defsym,_Z3barv=_Z3bazv

> >>>>>>>

> >>>>>>> To me, this is the same as providing an overriding definition of

> bar()

> >>>>>>> that prints "in baz", which clearly violates the one-definition

> rule.

> >>>>>>>

> >>>>>>> On what basis do you consider this a valid thing to do, to the

> extent

> >>>>>>> that you want to preserve the unoptimized behavior across LTO?

> >>>>>>>

> >>>>>>> Is there a real-world example where someone would want to do this

> in

> >>>>>>> production code? I'm afraid I'd have zero sympathy for them. If

> they

> >>>>>>> want something like that to work, they should just turn off

> >>>>>>> cross-module inlining.

> >>>>>>

> >>>>>>

> >>>>>> Fair enough. It was a contrived example, not based on anything I

> have

> >>>>>> seen so far. If that violates ODR then I agree all bets are off.

> >>>>>>

> >>>>>> Let me try with a modified change that marks these with

> >>>>>> LDPR_PREEMPTED_REG. Sri, would you mind changing the patch and I'll

> give the

> >>>>>> new version a try?

> >>>>

> >>>>

> >>>>

> >>>> New patch attached.

> >>>>

> >>>>

> >>>> gold/

> >>>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

> >>>>       New method.

> >>>> (Unary_expression::set_expr_sym_in_real_elf): New method.

> >>>> (Binary_expression::set_expr_sym_in_real_elf): New method.

> >>>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> >>>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> >>>> defined or used in defsyms.

> >>>> * script.cc (set_defsym_uses_in_real_elf): New method.

> >>>> (Script_options::is_defsym_def): New method.

> >>>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> >>>> (Symbol_assignment::is_defsym): New method.

> >>>> (Symbol_assignment::value): New method.

> >>>> (Script_options::is_defsym_def): New method.

> >>>> (Script_options::set_defsym_uses_in_real_elf): New method.

> >>>> * testsuite/Makefile.am (plugin_test_defsym): New test.

> >>>> * testsuite/Makefile.in: Regenerate.

> >>>> * testsuite/plugin_test.c: Check for new symbol resolution.

> >>>> * testsuite/plugin_test_defsym.sh: New script.

> >>>> * testsuite/plugin_test_defsym.c: New test source.

> >>>>

> >>>>

> >>>>

> >>>>>

> >>>>>

> >>>>> No problem.

> >>>>>

> >>>>>>

> >>>>>>

> >>>>>> Thanks,

> >>>>>> Teresa

> >>>>>>

> >>>>>>>

> >>>>>>> I need a lot more justification to extend the plugin API.

> >>>>>>>

> >>>>>>> -cary

> >>>>>>

> >>>>>>

> >>>>>>

> >>>>>>

> >>>>>> --

> >>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com |

> 408-460-2413

> >>>>>

> >>>>>

> >>>>

>




-- 
Teresa Johnson |  Software Engineer |  tejohnson@google.com |  408-460-2413
Cary Coutant Feb. 15, 2018, 10:21 p.m. | #24
> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>       New method.

> (Unary_expression::set_expr_sym_in_real_elf): New method.

> (Binary_expression::set_expr_sym_in_real_elf): New method.

> (Trinary_expression::set_expr_sym_in_real_elf): New method.

> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

> defined or used in defsyms.

> * plugin.h (Plugin_manager::is_defsym_def): New method.

> (Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.

> (Plugin_manager::defsym_defines_set_): New member.

> (Plugin_manager::Defsym_defines_set): New typedef.

> * script.cc (Script_options::set_defsym_uses_in_real_elf): New method.

> (Script_options::find_defsym_defs): New method.

> * script.h (Expression::set_expr_sym_in_real_elf): New method.

> (Symbol_assignment::is_defsym): New method.

> (Symbol_assignment::value): New method.

> (Script_options::find_defsym_defs): New method.

> (Script_options::set_defsym_uses_in_real_elf): New method.

> * testsuite/Makefile.am (plugin_test_defsym): New test.

> * testsuite/Makefile.in: Regenerate.

> * testsuite/plugin_test.c: Check for new symbol resolution.

> * testsuite/plugin_test_defsym.sh: New script.

> * testsuite/plugin_test_defsym.c: New test source.


+// Populates the set with symbols used in defsym LHS.
+
+void Script_options::find_defsym_defs(Unordered_set<std::string>& defsym_set)
+{
+  for (Symbol_assignments::const_iterator p =
this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym())
+        defsym_set.insert((*p)->name());
+    }
+}
+
+void
+Script_options::set_defsym_uses_in_real_elf(Symbol_table* symtab) const
+{
+  for (Symbol_assignments::const_iterator p =
this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym())
+        (*p)->value()->set_expr_sym_in_real_elf(symtab);
+    }
+}

Are you intentionally excluding symbols defined in scripts? It seems
to me that they should be treated the same as --defsym.

+  // Sets all symbols used in expressions as seen in a real ELF object.
+  virtual void
+  set_expr_sym_in_real_elf(Symbol_table*) const
+  { return; }

Omit "return;".

-cary
Cary Coutant Feb. 15, 2018, 10:28 p.m. | #25
> Fair enough. It was a contrived example, not based on anything I have seen

> so far. If that violates ODR then I agree all bets are off.


BTW, Ian wrote this comment in script.cc:

// The GNU linker lets symbol assignments in the linker script
// silently override defined symbols in object files.  We are
// compatible.  FIXME: Should we issue a warning?

I think this is a misfeature, and the comment suggests that Ian thought so too.

-cary
Jose E. Marchesi via Binutils Feb. 15, 2018, 11:31 p.m. | #26
On Thu, Feb 15, 2018 at 2:21 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> * expression.cc (Symbol_expression::set_expr_sym_in_real_elf):

>>       New method.

>> (Unary_expression::set_expr_sym_in_real_elf): New method.

>> (Binary_expression::set_expr_sym_in_real_elf): New method.

>> (Trinary_expression::set_expr_sym_in_real_elf): New method.

>> * plugin.cc (get_symbol_resolution_info): Fix symbol resolution if

>> defined or used in defsyms.

>> * plugin.h (Plugin_manager::is_defsym_def): New method.

>> (Plugin_manager::Plugin_manager): Initialize defsym_defines_set_.

>> (Plugin_manager::defsym_defines_set_): New member.

>> (Plugin_manager::Defsym_defines_set): New typedef.

>> * script.cc (Script_options::set_defsym_uses_in_real_elf): New method.

>> (Script_options::find_defsym_defs): New method.

>> * script.h (Expression::set_expr_sym_in_real_elf): New method.

>> (Symbol_assignment::is_defsym): New method.

>> (Symbol_assignment::value): New method.

>> (Script_options::find_defsym_defs): New method.

>> (Script_options::set_defsym_uses_in_real_elf): New method.

>> * testsuite/Makefile.am (plugin_test_defsym): New test.

>> * testsuite/Makefile.in: Regenerate.

>> * testsuite/plugin_test.c: Check for new symbol resolution.

>> * testsuite/plugin_test_defsym.sh: New script.

>> * testsuite/plugin_test_defsym.c: New test source.

>

> +// Populates the set with symbols used in defsym LHS.

> +

> +void Script_options::find_defsym_defs(Unordered_set<std::string>& defsym_set)

> +{

> +  for (Symbol_assignments::const_iterator p =

> this->symbol_assignments_.begin();

> +       p != this->symbol_assignments_.end();

> +       ++p)

> +    {

> +      if ((*p)->is_defsym())

> +        defsym_set.insert((*p)->name());

> +    }

> +}

> +

> +void

> +Script_options::set_defsym_uses_in_real_elf(Symbol_table* symtab) const

> +{

> +  for (Symbol_assignments::const_iterator p =

> this->symbol_assignments_.begin();

> +       p != this->symbol_assignments_.end();

> +       ++p)

> +    {

> +      if ((*p)->is_defsym())

> +        (*p)->value()->set_expr_sym_in_real_elf(symtab);

> +    }

> +}

>

> Are you intentionally excluding symbols defined in scripts? It seems

> to me that they should be treated the same as --defsym.


Ok,  it is just that I do not have test cases with scripts and I
haven't looked into creating a few yet.  If this is as simple as
omitting the is_defsym() check then I can make that change.


>

> +  // Sets all symbols used in expressions as seen in a real ELF object.

> +  virtual void

> +  set_expr_sym_in_real_elf(Symbol_table*) const

> +  { return; }

>

> Omit "return;".

>

> -cary
Cary Coutant Feb. 16, 2018, 12:58 a.m. | #27
>> Are you intentionally excluding symbols defined in scripts? It seems

>> to me that they should be treated the same as --defsym.

>

> Ok,  it is just that I do not have test cases with scripts and I

> haven't looked into creating a few yet.  If this is as simple as

> omitting the is_defsym() check then I can make that change.


Yes, that's all it is.

Your patch is OK with those changes.

-cary
Jose E. Marchesi via Binutils Feb. 16, 2018, 1:38 a.m. | #28
On Thu, Feb 15, 2018 at 4:58 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> Are you intentionally excluding symbols defined in scripts? It seems

>>> to me that they should be treated the same as --defsym.

>>

>> Ok,  it is just that I do not have test cases with scripts and I

>> haven't looked into creating a few yet.  If this is as simple as

>> omitting the is_defsym() check then I can make that change.

>

> Yes, that's all it is.

>

> Your patch is OK with those changes.


Thanks, made that change and committed the patch.

Sri

>

> -cary
Alan Modra Feb. 16, 2018, 8:23 a.m. | #29
On Thu, Feb 15, 2018 at 02:28:05PM -0800, Cary Coutant wrote:
> > Fair enough. It was a contrived example, not based on anything I have seen

> > so far. If that violates ODR then I agree all bets are off.

> 

> BTW, Ian wrote this comment in script.cc:

> 

> // The GNU linker lets symbol assignments in the linker script

> // silently override defined symbols in object files.  We are

> // compatible.  FIXME: Should we issue a warning?

> 

> I think this is a misfeature, and the comment suggests that Ian thought so too.


FWIW, so do I, but see the following in ld/ldexp.c

		  if (!update_definedness (tree->assign.dst, h) && 0)
		    {
		      /* Symbol was already defined.  For now this error
			 is disabled because it causes failures in the ld
			 testsuite: ld-elf/var1, ld-scripts/defined5, and
			 ld-scripts/pr14962.  Some of these no doubt
			 reflect scripts used in the wild.  */
		      (*link_info.callbacks->multiple_definition)
			(&link_info, h, link_info.output_bfd,
			 expld.result.section, expld.result.value);
		    }

ld-elf/var1 and ld-scripts/pr14962 both came from real-world use by
a boot loader.

-- 
Alan Modra
Australia Development Lab, IBM
Cary Coutant Feb. 16, 2018, 5:05 p.m. | #30
>> // The GNU linker lets symbol assignments in the linker script

>> // silently override defined symbols in object files.  We are

>> // compatible.  FIXME: Should we issue a warning?

>>

>> I think this is a misfeature, and the comment suggests that Ian thought so too.

>

> FWIW, so do I, but see the following in ld/ldexp.c

>

>                   if (!update_definedness (tree->assign.dst, h) && 0)

>                     {

>                       /* Symbol was already defined.  For now this error

>                          is disabled because it causes failures in the ld

>                          testsuite: ld-elf/var1, ld-scripts/defined5, and

>                          ld-scripts/pr14962.  Some of these no doubt

>                          reflect scripts used in the wild.  */

>                       (*link_info.callbacks->multiple_definition)

>                         (&link_info, h, link_info.output_bfd,

>                          expld.result.section, expld.result.value);

>                     }

>

> ld-elf/var1 and ld-scripts/pr14962 both came from real-world use by

> a boot loader.


Yeah, I had a feeling it would be problematic to fix it.

-cary

Patch

diff --git a/gold/expression.cc b/gold/expression.cc
index d764cc2a9d..bf65304833 100644
--- a/gold/expression.cc
+++ b/gold/expression.cc
@@ -205,6 +205,12 @@  class Symbol_expression : public Expression
   uint64_t
   value(const Expression_eval_info*);
 
+  bool
+  is_symbol_in_expression(const char* name)
+  {
+    return this->name_ == name;
+  }
+
   void
   print(FILE* f) const
   { fprintf(f, "%s", this->name_.c_str()); }
@@ -318,6 +324,10 @@  class Unary_expression : public Expression
   arg_print(FILE* f) const
   { this->arg_->print(f); }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  { return this->arg_->is_symbol_in_expression(name); }
+
  private:
   Expression* arg_;
 };
@@ -437,6 +447,13 @@  class Binary_expression : public Expression
     fprintf(f, ")");
   }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  {
+    return (this->left_->is_symbol_in_expression(name) ||
+            this->right_->is_symbol_in_expression(name));
+  }
+
  private:
   Expression* left_;
   Expression* right_;
@@ -622,6 +639,12 @@  class Trinary_expression : public Expression
   arg3_print(FILE* f) const
   { this->arg3_->print(f); }
 
+  bool
+  is_symbol_in_expression(const char* name)
+  { return (this->arg1_->is_symbol_in_expression(name) ||
+	    this->arg2_->is_symbol_in_expression(name) ||
+	    this->arg3_->is_symbol_in_expression(name)); }
+
  private:
   Expression* arg1_;
   Expression* arg2_;
diff --git a/gold/plugin.cc b/gold/plugin.cc
index 02fef25dda..7fd42821af 100644
--- a/gold/plugin.cc
+++ b/gold/plugin.cc
@@ -942,6 +942,9 @@  is_referenced_from_outside(Symbol* lsym)
     return true;
   if (parameters->options().is_undefined(lsym->name()))
     return true;
+  Layout* layout = parameters->options().plugins()->layout();
+  if (layout->script_options()->is_defsym_use(lsym->name()))
+    return true;
   return false;
 }
 
@@ -989,6 +992,8 @@  Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
       return version > 2 ? LDPS_NO_SYMS : LDPS_OK;
     }
 
+  Layout *layout = parameters->options().plugins()->layout();
+
   for (int i = 0; i < nsyms; i++)
     {
       ld_plugin_symbol* isym = &syms[i];
@@ -997,9 +1002,16 @@  Pluginobj::get_symbol_resolution_info(Symbol_table* symtab,
         lsym = symtab->resolve_forwards(lsym);
       ld_plugin_symbol_resolution res = LDPR_UNKNOWN;
 
-      if (lsym->is_undefined())
-        // The symbol remains undefined.
-        res = LDPR_UNDEF;
+      if (layout->script_options()->is_defsym_def(lsym->name()))
+	{
+	  // The symbol is redefined.
+	  res = LDPR_LINKER_REDEFINED;
+	}
+      else if (lsym->is_undefined())
+	{
+            // The symbol remains undefined.
+            res = LDPR_UNDEF;
+	}
       else if (isym->def == LDPK_UNDEF
                || isym->def == LDPK_WEAKUNDEF
                || isym->def == LDPK_COMMON)
diff --git a/gold/script.cc b/gold/script.cc
index db243cf435..e0a8d9a742 100644
--- a/gold/script.cc
+++ b/gold/script.cc
@@ -1112,6 +1112,33 @@  Script_options::is_pending_assignment(const char* name)
   return false;
 }
 
+// Returns trus if symbol with NAME is redefined by defsym.
+
+bool Script_options::is_defsym_def(const char* name)
+{
+  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    if ((*p)->is_defsym() && (*p)->name() == name)
+      return true;
+  return false;
+}
+
+// Returns trus if symbol with NAME is used in a defsym expression.
+
+bool
+Script_options::is_defsym_use(const char* name)
+{
+  for (Symbol_assignments::iterator p = this->symbol_assignments_.begin();
+       p != this->symbol_assignments_.end();
+       ++p)
+    {
+      if ((*p)->is_defsym() && (*p)->value()->is_symbol_in_expression(name))
+        return true;
+    }
+  return false;
+}
+
 // Add a symbol to be defined.
 
 void
diff --git a/gold/script.h b/gold/script.h
index ec8ce8e110..2159f9f7e7 100644
--- a/gold/script.h
+++ b/gold/script.h
@@ -129,13 +129,18 @@  class Expression
   virtual uint64_t
   value(const Expression_eval_info*) = 0;
 
+  // Return true if symbol is part of the expression.
+  // The child class that uses this symbol returns true.
+  virtual bool
+  is_symbol_in_expression(const char*)
+  { return false; }
+
  private:
   // May not be copied.
   Expression(const Expression&);
   Expression& operator=(const Expression&);
 };
 
-
 // Version_script_info stores information parsed from the version
 // script, either provided by --version-script or as part of a linker
 // script.  A single Version_script_info object per target is owned by
@@ -344,6 +349,14 @@  class Symbol_assignment
   void
   finalize(Symbol_table*, const Layout*);
 
+  bool
+  is_defsym() const
+  { return is_defsym_; }
+
+  Expression *
+  value() const
+  { return val_; }
+
   // Finalize the symbol value when it can refer to the dot symbol.
   void
   finalize_with_dot(Symbol_table*, const Layout*, uint64_t dot_value,
@@ -454,6 +467,14 @@  class Script_options
   bool
   define_symbol(const char* definition);
 
+  // Returns true if symbol is defined via defsym.
+  bool
+  is_defsym_def(const char *name);
+
+  // Returns true if symbol is used in a defsym.
+  bool
+  is_defsym_use(const char *name);
+
   // Create sections required by any linker scripts.
   void
   create_script_sections(Layout*);
diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
index 16cae8004c..b94182856e 100644
--- a/gold/testsuite/Makefile.am
+++ b/gold/testsuite/Makefile.am
@@ -2332,6 +2332,19 @@  plugin_test_start_lib: unused.o plugin_start_lib_test.o plugin_start_lib_test_2.
 plugin_test_start_lib.err: plugin_test_start_lib
 	@touch plugin_test_start_lib.err
 
+check_PROGRAMS += plugin_test_defsym
+check_SCRIPTS += plugin_test_defsym.sh
+check_DATA += plugin_test_defsym.err
+MOSTLYCLEANFILES += plugin_test_defsym.err
+plugin_test_defsym.syms: plugin_test_defsym.o
+	$(TEST_READELF) -sW $< >$@ 2>/dev/null
+plugin_test_defsym.o: plugin_test_defsym.c
+	$(COMPILE) -c -o $@ $<
+plugin_test_defsym: plugin_test_defsym.o plugin_test_defsym.syms gcctestdir/ld plugin_test.so
+	$(CXXLINK) -Bgcctestdir/ -Wl,--no-demangle,--plugin,"./plugin_test.so" \
+		-Wl,--defsym,bar=foo+2 plugin_test_defsym.syms 2>plugin_test_defsym.err
+plugin_test_defsym.err: plugin_test_defsym
+	@touch plugin_test_defsym.err
 
 plugin_start_lib_test_2.syms: plugin_start_lib_test_2.o
 	$(TEST_READELF) -sW $< >$@ 2>/dev/null
diff --git a/gold/testsuite/plugin_test.c b/gold/testsuite/plugin_test.c
index c6df7b6f7d..b92fb8acf5 100644
--- a/gold/testsuite/plugin_test.c
+++ b/gold/testsuite/plugin_test.c
@@ -476,6 +476,9 @@  all_symbols_read_hook(void)
             case LDPR_RESOLVED_DYN:
               res = "RESOLVED_DYN";
               break;
+	    case LDPR_LINKER_REDEFINED:
+	      res = "LINKER_REDEFINED";
+	      break;
             default:
               res = "?";
               break;
diff --git a/gold/testsuite/plugin_test_defsym.c b/gold/testsuite/plugin_test_defsym.c
index e69de29bb2..a3c3b962f3 100644
--- a/gold/testsuite/plugin_test_defsym.c
+++ b/gold/testsuite/plugin_test_defsym.c
@@ -0,0 +1,32 @@ 
+/* plugin_test_defsym.c -- a test case for gold
+
+   Copyright (C) 2018 onwards Free Software Foundation, Inc.
+   Written by Sriraman Tallam <tmsriram@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.  */
+
+int foo(void);
+int foo(void) {
+  return 0;
+}
+
+int bar(void);
+
+int main(void) {
+  return bar();  
+}
diff --git a/gold/testsuite/plugin_test_defsym.sh b/gold/testsuite/plugin_test_defsym.sh
index e69de29bb2..3eb6446433 100755
--- a/gold/testsuite/plugin_test_defsym.sh
+++ b/gold/testsuite/plugin_test_defsym.sh
@@ -0,0 +1,52 @@ 
+#!/bin/sh
+
+# plugin_test_defsym.sh -- a test case for the plugin API.
+
+# Copyright (C) 2018 onwards Free Software Foundation, Inc.
+# Written by Sriraman Tallam <tmsriram@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.
+
+# This file goes with plugin_test.c, a simple plug-in library that
+# exercises the basic interfaces and prints out version numbers and
+# options passed to the plugin.
+
+# This checks if the symbol resolution withe export dynamic symbol is
+# as expected.
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+	echo "Did not find expected output in $1:"
+	echo "   $2"
+	echo ""
+	echo "Actual output below:"
+	cat "$1"
+	exit 1
+    fi
+}
+
+check plugin_test_defsym.err "API version:"
+check plugin_test_defsym.err "gold version:"
+check plugin_test_defsym.err "plugin_test_defsym.syms: claim file hook called"
+check plugin_test_defsym.err "plugin_test_defsym.syms: bar: LINKER_REDEFINED"
+check plugin_test_defsym.err "plugin_test_defsym.syms: foo: PREVAILING_DEF_REG"
+check plugin_test_defsym.err "cleanup hook called"
+
+exit 0
diff --git a/include/plugin-api.h b/include/plugin-api.h
index 03e21f4a0c..6c02947699 100644
--- a/include/plugin-api.h
+++ b/include/plugin-api.h
@@ -162,7 +162,11 @@  enum ld_plugin_symbol_resolution
      references from regular objects.  It is only referenced from IR
      code, but the symbol is exported and may be referenced from
      a dynamic object (not seen at link time).  */
-  LDPR_PREVAILING_DEF_IRONLY_EXP
+  LDPR_PREVAILING_DEF_IRONLY_EXP,
+
+  /* This symbol has been redefined by the linker, typically using
+     defsym.  */
+  LDPR_LINKER_REDEFINED,
 };
 
 /* The plugin library's "claim file" handler.  */