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

classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
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

plugin_defsym_patch.txt (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
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 <[hidden email]> 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

plugin_defsym_patch.txt (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
> 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
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
In reply to this post by ccoutant
On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <[hidden email]> 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 |  [hidden email] |  408-460-2413
<(408)%20460-2413>
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
>> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
In reply to this post by ccoutant
On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
On Tue, Feb 13, 2018 at 10:32 AM, Sriraman Tallam <[hidden email]> wrote:

> On Mon, Feb 12, 2018 at 12:19 PM, Cary Coutant <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
In reply to this post by ccoutant
On Tue, Feb 13, 2018 at 10:24 AM, Cary Coutant <[hidden email]> 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 |  [hidden email] |  408-460-2413
<(408)%20460-2413>
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
In reply to this post by Sourceware - binutils list mailing list
> 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
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
In reply to this post by Sourceware - binutils list mailing list
>> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
In reply to this post by ccoutant
On Tue, Feb 13, 2018 at 3:53 PM, Cary Coutant <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
On Tue, Feb 13, 2018 at 4:08 PM, Sriraman Tallam <[hidden email]> wrote:
> On Tue, Feb 13, 2018 at 3:53 PM, Cary Coutant <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
In reply to this post by ccoutant
On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 |  [hidden email] |  408-460-2413
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <[hidden email]>
wrote:

>
>
> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 |  [hidden email] |
> 408-460-2413 <(408)%20460-2413>
>
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
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 <[hidden email]> wrote:

>
>
> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <[hidden email]>
> wrote:
>>
>>
>>
>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <[hidden email]>
>> wrote:
>>>
>>>
>>>
>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 | [hidden email] | 408-460-2413
>>
>>
>

plugin_defsym_patch.txt (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - binutils list mailing list
On Tue, Feb 13, 2018 at 5:05 PM, Sriraman Tallam <[hidden email]> 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 <[hidden email]> wrote:
>>
>>
>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <[hidden email]>
>> wrote:
>>>
>>>
>>>
>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <[hidden email]>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 | [hidden email] | 408-460-2413
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
>> 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
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
In reply to this post by Sourceware - binutils list mailing list
@@ -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 <[hidden email]> 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 <[hidden email]> wrote:
>>
>>
>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <[hidden email]>
>> wrote:
>>>
>>>
>>>
>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <[hidden email]>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 | [hidden email] | 408-460-2413
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

ccoutant
In reply to this post by Sourceware - binutils list mailing list
+  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 <[hidden email]> 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 <[hidden email]> wrote:
>>
>>
>> On Tue, Feb 13, 2018 at 4:52 PM, Sriraman Tallam <[hidden email]>
>> wrote:
>>>
>>>
>>>
>>> On Tue, Feb 13, 2018 at 4:49 PM, Teresa Johnson <[hidden email]>
>>> wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Feb 13, 2018 at 4:04 PM, Cary Coutant <[hidden email]> 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 | [hidden email] | 408-460-2413
>>>
>>>
>>
12