diff mbox

section anchors and weak hidden symbols

Message ID 518A3D8E.5020800@acm.org
State New
Headers show

Commit Message

Nathan Sidwell May 8, 2013, 11:57 a.m. UTC 
This patch fixes a problem with section anchors.  Found on powerpc, but also 
appears on MIPS and ARM targets.

Section anchors can only be used for definitions known to bind in the current 
object file.  The default predicate uses the bind_local_p hook to determine 
this.  Unfortunately that hook determines whether the decl's binding is 
determined at static link time (i.e. within the dynamic object this object is 
linked).  That's very nearly the same, except for symbols that have a weak 
hidden definition in this object file.  For such symbols, binds_local_p returns 
true, because the binding must be within the dynamic object.  But we shouldn't 
use a section anchor as a definition in a different object file could win at 
static link time. (I'm not 100% sure there aren't other cases where 
module-binding and object-binding differ for a definition.)

It surprised me that binds_local_p has the semantics it does -- perhaps its 
meaning has changed, or it is simply poorly named.  I would have thought 
binds_module_p would be a better name.

Anyway, rather than go on a renaming exercise, I chose to adjust 
default_use_anchors_for_symbol_p to reject any weak symbol.

tested on powerpc-linux-gnu, ok?

nathan
2013-05-08  Nathan Sidwell  <nathan@codesourcery.com>

	gcc/
	* varasm.c (default_use_anchors_for_symbol_p): Reject WEAK.

	gcc/testsuite/
	* gcc.dg/visibility-21.c: New.

Comments

Jan Hubicka May 8, 2013, 1:11 p.m. UTC | #1 
> 2013-05-08  Nathan Sidwell  <nathan@codesourcery.com>
> 
> 	gcc/
> 	* varasm.c (default_use_anchors_for_symbol_p): Reject WEAK.
> 
> 	gcc/testsuite/
> 	* gcc.dg/visibility-21.c: New.
> 
> Index: gcc/varasm.c
> ===================================================================
> --- gcc/varasm.c	(revision 410150)
> +++ gcc/varasm.c	(working copy)
> @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_
>        if (!targetm.binds_local_p (decl))
>  	return false;
>  
> +      /* Weak decls might be overridden, but could still be local to
> +	 the module.  */
> +      if (DECL_WEAK (decl))
> +	return false;

With LTO we play more of similar tricks, by making use of the resolution file.
I.e. for COMMON and EXTERNAL.  Does it matter here?

I would preffer the renaming excercise, since the name confused me few times,
too and the other predicate would be useful for IPA code :)

Thanks a lot for looking into this!
Honza
Nathan Sidwell May 8, 2013, 1:44 p.m. UTC | #2 
On 05/08/13 14:11, Jan Hubicka wrote:

> With LTO we play more of similar tricks, by making use of the resolution file.
> I.e. for COMMON and EXTERNAL.  Does it matter here?

probably.  It'd be a missed optimization though, rather than wrong code emission.

>
> I would preffer the renaming excercise, since the name confused me few times,
> too and the other predicate would be useful for IPA code :)

before invoking sed, can we agree on the right names?  I propose

* renaming 'binds_local_p' to 'binds_module_p' (and any default implementations 
etc likewise).  Keeps the semantics of returning true if the symbol is known to 
bind in the dynamic object or executable the current object file ends up in.

* a new function or hook 'binds_here_p' (and any default implementations 
needed).  Returns true if the symbol is known to bind in the object file being 
emitted by the current compilation.

'here's not the best name, but sadly 'object' is overloaded and could mean the 
object being queried or the object file being emitted.  Similarly 'translation 
unit' is too restrictive in LTO mode, where multiple TUs are in play.  I'm open 
to a better name.

Clearly these are related in that binds_here_p returning true implies 
binds_module_p being true, and binds_module_p returning false implies 
binds_here_p being false.

Because of how gcc's currently structured the implementation of that new hook 
would be
   if (binds_local_p () && !WEAK ()) return true;

* finally change default_use_anchors_p to use the new hook.

thoughts?

nathan
Richard Sandiford May 8, 2013, 3:16 p.m. UTC | #3 
Thanks for fixing this.

Nathan Sidwell <nathan@acm.org> writes:
>> I would preffer the renaming excercise, since the name confused me few times,
>> too and the other predicate would be useful for IPA code :)
>
> before invoking sed, can we agree on the right names?  I propose
>
> * renaming 'binds_local_p' to 'binds_module_p' (and any default implementations 
> etc likewise).  Keeps the semantics of returning true if the symbol is known to 
> bind in the dynamic object or executable the current object file ends up in.
>
> * a new function or hook 'binds_here_p' (and any default implementations 
> needed).  Returns true if the symbol is known to bind in the object file being 
> emitted by the current compilation.
>
> 'here's not the best name, but sadly 'object' is overloaded and could mean the 
> object being queried or the object file being emitted.  Similarly 'translation 
> unit' is too restrictive in LTO mode, where multiple TUs are in play.  I'm open 
> to a better name.
>
> Clearly these are related in that binds_here_p returning true implies 
> binds_module_p being true, and binds_module_p returning false implies 
> binds_here_p being false.
>
> Because of how gcc's currently structured the implementation of that new hook 
> would be
>    if (binds_local_p () && !WEAK ()) return true;
>
> * finally change default_use_anchors_p to use the new hook.
>
> thoughts?

Not really a useful answer, but sometimes if everyone agrees you get no
replies, so: sounds great to me FWIW, including the "here" thing.

Richard
Jan Hubicka May 8, 2013, 5:47 p.m. UTC | #4 
> On 05/08/13 14:11, Jan Hubicka wrote:
> 
> >With LTO we play more of similar tricks, by making use of the resolution file.
> >I.e. for COMMON and EXTERNAL.  Does it matter here?
> 
> probably.  It'd be a missed optimization though, rather than wrong code emission.

Well, the resolution info turns !bind_local_p into binds_local_p knowing the
fact that they was bound into non-LTO code from the same module.  I think that
may also cause anchors to be confused.
> 
> >
> >I would preffer the renaming excercise, since the name confused me few times,
> >too and the other predicate would be useful for IPA code :)
> 
> before invoking sed, can we agree on the right names?  I propose
> 
> * renaming 'binds_local_p' to 'binds_module_p' (and any default
> implementations etc likewise).  Keeps the semantics of returning
> true if the symbol is known to bind in the dynamic object or
> executable the current object file ends up in.

Sounds good to me. 
> 
> * a new function or hook 'binds_here_p' (and any default
> implementations needed).  Returns true if the symbol is known to
> bind in the object file being emitted by the current compilation.

Thinking about it again, isn't decl_replaceable_p the thing you are looking for
here?
> 
> 'here's not the best name, but sadly 'object' is overloaded and
> could mean the object being queried or the object file being
> emitted.  Similarly 'translation unit' is too restrictive in LTO
> mode, where multiple TUs are in play.  I'm open to a better name.

Hmm, the whole thing is even slightly more complicated, part of it is already
handles as part of symtab_availability infrastructure.  We basicaly want to ask.

  1) will be decl bound to current module?
       - Now binds_local_p
       - Proposed binds_module_p
       - LLVM symbol->isLocalLinkage I think.

     Useful primarily when generating references (IP relative or GOT/PLT based)

  2) will be decl bound to particular definition in current unit

        - Now decl_replaceable_p perhaps?
        - Proposed binds_here_p.
	- LLVM !symbol->isWeakForLinker i think.

     this is useful for the anchors and also all optimizations that actually
     needs to hack on the definition.

  3) will be decl bound to definition that is equivalnet to one I am seeing?
     This differs from 1) i.e. for C++ one-decl-rule functions, in particular
     inlines.
     
         - Now cgraph_body_availability (node) >= AVAIL_AVAILABLE for functions
               const_value_known_p (node) for vairables.
               plus there is somewhat confusing cgraph_variable_initializer_availability
               that should be removed.
	 - Proposed ???
         - LLVM symbol->mayBeOverridden()

     Useful for constant propagation, inlining and other stuff.

I wonder if we can come with something more clearly named.
When it comes to weird predicates on symbols, we also have
can_refer_decl_in_current_unit_p.

Honza
     
> 
> Clearly these are related in that binds_here_p returning true
> implies binds_module_p being true, and binds_module_p returning
> false implies binds_here_p being false.
> 
> Because of how gcc's currently structured the implementation of that
> new hook would be
>   if (binds_local_p () && !WEAK ()) return true;
> 
> * finally change default_use_anchors_p to use the new hook.
> 
> thoughts?
> 
> nathan
Bernhard Reutner-Fischer May 8, 2013, 8:11 p.m. UTC | #5 
On 8 May 2013 15:11:18 Jan Hubicka <hubicka@ucw.cz> wrote:
> > 2013-05-08  Nathan Sidwell  <nathan@codesourcery.com>
> > 	gcc/
> > 	* varasm.c (default_use_anchors_for_symbol_p): Reject WEAK.
> > 	gcc/testsuite/
> > 	* gcc.dg/visibility-21.c: New.
> > Index: gcc/varasm.c
> > ===================================================================
> > --- gcc/varasm.c	(revision 410150)
> > +++ gcc/varasm.c	(working copy)
> > @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_
> >        if (!targetm.binds_local_p (decl))
> >  	return false;
> > +      /* Weak decls might be overridden, but could still be local to
> > +	 the module.  */
> > +      if (DECL_WEAK (decl))
> > +	return false;
>
> With LTO we play more of similar tricks, by making use of the resolution file.
> I.e. for COMMON and EXTERNAL.  Does it matter here?
>
> I would preffer the renaming excercise, since the name confused me few times,
> too and the other predicate would be useful for IPA code :)
>
> Thanks a lot for looking into this!
> Honza

Does this regress PR32219 ?
http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html
I don't remember if the patch and test case were applied yet and don't have 
the sources at hand..

Thanks,


Sent with AquaMail for Android
http://www.aqua-mail.com
Chung-Lin Tang May 9, 2013, 5:36 a.m. UTC | #6 
On 2013/5/9 04:11 AM, Bernhard Reutner-Fischer wrote:
> On 8 May 2013 15:11:18 Jan Hubicka <hubicka@ucw.cz> wrote:
>> > 2013-05-08  Nathan Sidwell  <nathan@codesourcery.com>
>> >     gcc/
>> >     * varasm.c (default_use_anchors_for_symbol_p): Reject WEAK.
>> >     gcc/testsuite/
>> >     * gcc.dg/visibility-21.c: New.
>> > Index: gcc/varasm.c
>> > ===================================================================
>> > --- gcc/varasm.c    (revision 410150)
>> > +++ gcc/varasm.c    (working copy)
>> > @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_
>> >        if (!targetm.binds_local_p (decl))
>> >      return false;
>> > +      /* Weak decls might be overridden, but could still be local to
>> > +     the module.  */
>> > +      if (DECL_WEAK (decl))
>> > +    return false;
>>
>> With LTO we play more of similar tricks, by making use of the
>> resolution file.
>> I.e. for COMMON and EXTERNAL.  Does it matter here?
>>
>> I would preffer the renaming excercise, since the name confused me few
>> times,
>> too and the other predicate would be useful for IPA code :)
>>
>> Thanks a lot for looking into this!
>> Honza
> 
> Does this regress PR32219 ?
> http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html
> I don't remember if the patch and test case were applied yet and don't
> have the sources at hand..
> 
> Thanks,

I don't think so (Nathan and I were also discussing that PR in the
context of this fix). The testcase in that PR still segfaults, which I
have another fix. Will post later.

Chung-Lin
Nathan Sidwell May 9, 2013, 6:02 a.m. UTC | #7 
On 05/08/13 18:47, Jan Hubicka wrote:
>> On 05/08/13 14:11, Jan Hubicka wrote:

>> * a new function or hook 'binds_here_p' (and any default
>> implementations needed).  Returns true if the symbol is known to
>> bind in the object file being emitted by the current compilation.
>
> Thinking about it again, isn't decl_replaceable_p the thing you are looking for
> here?

that looks promising.  I'll try !decl_replaceable_p in the section anchor hook.

> I wonder if we can come with something more clearly named.
> When it comes to weird predicates on symbols, we also have
> can_refer_decl_in_current_unit_p.

thanks for your comments, if decl_replaceable_p is the right existing predicate, 
that does make figuring out the right set of names less urgent.

nathan
diff mbox

Patch

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 410150)
+++ gcc/varasm.c	(working copy)
@@ -6871,6 +6871,11 @@  default_use_anchors_for_symbol_p (const_
       if (!targetm.binds_local_p (decl))
 	return false;
 
+      /* Weak decls might be overridden, but could still be local to
+	 the module.  */
+      if (DECL_WEAK (decl))
+	return false;
+
       /* Don't use section anchors for decls that will be placed in a
 	 small data section.  */
       /* ??? Ideally, this check would be redundant with the SECTION_SMALL
Index: gcc/testsuite/gcc.dg/visibility-21.c
===================================================================
--- gcc/testsuite/gcc.dg/visibility-21.c	(revision 0)
+++ gcc/testsuite/gcc.dg/visibility-21.c	(revision 0)
@@ -0,0 +1,13 @@ 
+/* Test visibility attribute on function definition. */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fsection-anchors" } */
+/* { dg-require-visibility "" } */
+/* { dg-require-weak "" } */
+/* { dg-final { scan-assembler-not "ANCHOR" } } */
+
+int __attribute__((weak, visibility("hidden"))) weak_hidden[3];
+
+int *f_weak_hidden ()
+{
+  return weak_hidden;
+}