update_vtable_references segfault

update_vtable_references segfault

[Nathan Sidwell]

Jan,
it looks like your recent changes to function_and_variable_visibility and 
friends causes regressions in targets that do not support aliases (PTX for example).

specifically, we get a segfault  in update_vtable_references (ipa-visibility.c) at
	*tp = symtab_node::get (*tp)->noninterposable_alias ()->decl;

because symtab_node::noninterposable_alias (symtab.c) returns NULL.  It does 
this on targets that do not support aliases:

#ifndef ASM_OUTPUT_DEF
   /* If aliases aren't supported by the assembler, fail.  */
   return NULL;
#endif

and update_vtable_references doesn't anticipate that happening, blindly 
dereferencing the returned value.  Is  the fix:

a) add  a check in update_vtable_references on noninterposable_alias's return value?

b) augment can_replace_by_local_alias_in_vtable to check whether aliases can be 
created?

c) augment function_and_variable_visibility to not attempt such replacement:
       /* Update virtual tables to point to local aliases where possible.  */
       if (DECL_VIRTUAL_P (vnode->decl)
	  && !DECL_EXTERNAL (vnode->decl))
	...
d) something else?

nathan

Re: update_vtable_references segfault

[Jan Hubicka]

> Jan,
> it looks like your recent changes to
> function_and_variable_visibility and friends causes regressions in
> targets that do not support aliases (PTX for example).
> 
> specifically, we get a segfault  in update_vtable_references (ipa-visibility.c) at
> 	*tp = symtab_node::get (*tp)->noninterposable_alias ()->decl;
> 
> because symtab_node::noninterposable_alias (symtab.c) returns NULL.
> It does this on targets that do not support aliases:
> 
> #ifndef ASM_OUTPUT_DEF
>   /* If aliases aren't supported by the assembler, fail.  */
>   return NULL;
> #endif
> 
> and update_vtable_references doesn't anticipate that happening,
> blindly dereferencing the returned value.  Is  the fix:
> 
> a) add  a check in update_vtable_references on noninterposable_alias's return value?
> 
> b) augment can_replace_by_local_alias_in_vtable to check whether
> aliases can be created?

I think this is best: can_replace_by_local_alias_in_vtable exists to prevent the
body walk in cases we are not going to create the alias.  This is because in LTO
we may need to stream in the constructor from the object file that is not copletely
free and thus it is better to not touch it unless necessary.

Thanks for looking into this!
Honza
> 
> c) augment function_and_variable_visibility to not attempt such replacement:
>       /* Update virtual tables to point to local aliases where possible.  */
>       if (DECL_VIRTUAL_P (vnode->decl)
> 	  && !DECL_EXTERNAL (vnode->decl))
> 	...
> d) something else?
> 
> nathan

Re: update_vtable_references segfault

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On 12/11/15 13:15, Jan Hubicka wrote:
>> Jan,

>> b) augment can_replace_by_local_alias_in_vtable to check whether
>> aliases can be created?
>
> I think this is best: can_replace_by_local_alias_in_vtable exists to prevent the
> body walk in cases we are not going to create the alias.  This is because in LTO
> we may need to stream in the constructor from the object file that is not copletely
> free and thus it is better to not touch it unless necessary.

I went with augmenting can_replace_by_local_alias, which 
can_replace_by_local_alias_in_vtable calls.  I also noticed that both should be 
static, which  I suspect will encourage the inliner to go inline them and then 
determine a bunch of code is unreachable.

tested on x86-linux and ptx-none.

ok?

nathan

[-- Attachment #2: trunk-vis.patch --]
[-- Type: text/x-patch, Size: 2133 bytes --]

2015-12-12  Nathan Sidwell  <nathan@acm.org>

	* ipa-visibility.c (can_replace_by_local_alias): Make static,
	check ASM_OUTPUT_DEF.
	(can_replace_by_local_alias_in_vtable): Make static.
	(function_and_variable_visibility): Reformat overlong comment.

Index: ipa-visibility.c
===================================================================
--- ipa-visibility.c	(revision 231571)
+++ ipa-visibility.c	(working copy)
@@ -329,9 +329,13 @@ varpool_node::externally_visible_p (void
    Local aliases save dynamic linking overhead and enable more optimizations.
  */
 
-bool
+static bool
 can_replace_by_local_alias (symtab_node *node)
 {
+#ifndef ASM_OUTPUT_DEF
+  /* If aliases aren't supported, we can't do replacement.  */
+  return false;
+#endif
   /* Weakrefs have a reason to be non-local.  Be sure we do not replace
      them.  */
   while (node->transparent_alias && node->definition && !node->weakref)
@@ -344,11 +348,11 @@ can_replace_by_local_alias (symtab_node
 	  && !node->can_be_discarded_p ());
 }
 
-/* Return true if we can replace refernece to NODE by local alias
+/* Return true if we can replace reference to NODE by local alias
    within a virtual table.  Generally we can replace function pointers
    and virtual table pointers.  */
 
-bool
+static bool
 can_replace_by_local_alias_in_vtable (symtab_node *node)
 {
   if (is_a <varpool_node *> (node)
@@ -592,10 +596,11 @@ function_and_variable_visibility (bool w
       if (!node->local.local)
         node->local.local |= node->local_p ();
 
-      /* If we know that function can not be overwritten by a different semantics
-	 and moreover its section can not be discarded, replace all direct calls
-	 by calls to an noninterposable alias.  This make dynamic linking
-	 cheaper and enable more optimization.
+      /* If we know that function can not be overwritten by a
+	 different semantics and moreover its section can not be
+	 discarded, replace all direct calls by calls to an
+	 noninterposable alias.  This make dynamic linking cheaper and
+	 enable more optimization.
 
 	 TODO: We can also update virtual tables.  */
       if (node->callers 

Re: update_vtable_references segfault

[Nathan Sidwell]

On 12/12/15 09:44, Nathan Sidwell wrote:
> On 12/11/15 13:15, Jan Hubicka wrote:
>>> Jan,
>
>>> b) augment can_replace_by_local_alias_in_vtable to check whether
>>> aliases can be created?
>>
>> I think this is best: can_replace_by_local_alias_in_vtable exists to prevent the
>> body walk in cases we are not going to create the alias.  This is because in LTO
>> we may need to stream in the constructor from the object file that is not
>> copletely
>> free and thus it is better to not touch it unless necessary.
>
> I went with augmenting can_replace_by_local_alias, which
> can_replace_by_local_alias_in_vtable calls.  I also noticed that both should be
> static, which  I suspect will encourage the inliner to go inline them and then
> determine a bunch of code is unreachable.
>
> tested on x86-linux and ptx-none.
>
> ok?

https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01324.html

ping?

nathan

Re: update_vtable_references segfault

[Jan Hubicka]

> On 12/12/15 09:44, Nathan Sidwell wrote:
> >On 12/11/15 13:15, Jan Hubicka wrote:
> >>>Jan,
> >
> >>>b) augment can_replace_by_local_alias_in_vtable to check whether
> >>>aliases can be created?
> >>
> >>I think this is best: can_replace_by_local_alias_in_vtable exists to prevent the
> >>body walk in cases we are not going to create the alias.  This is because in LTO
> >>we may need to stream in the constructor from the object file that is not
> >>copletely
> >>free and thus it is better to not touch it unless necessary.
> >
> >I went with augmenting can_replace_by_local_alias, which
> >can_replace_by_local_alias_in_vtable calls.  I also noticed that both should be
> >static, which  I suspect will encourage the inliner to go inline them and then
> >determine a bunch of code is unreachable.
> >
> >tested on x86-linux and ptx-none.
> >
> >ok?
> 
> https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01324.html
OK, thanks!
Honza
> 
> ping?
> 
> nathan