Patchwork Fix undefined symbols in WHOPR build of Mozilla's javascript interpretter

login
register
mail settings
Submitter Jan Hubicka
Date July 5, 2010, 11:50 p.m.
Message ID <20100705235036.GC12132@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/57945/
State New
Headers show

Comments

Jan Hubicka - July 5, 2010, 11:50 p.m.
Hi,
this patch solves problem with building Mozilla javascript shell with
-fwhole-program and WHOPR.  The problem here is that cloning causes
devirtualization.  This devirtualization is recognized during virtual clone
materialization (i.e. late) because Martin did not merge his IPA
devritualization code yet.

Because WHOPR brings the function static and house it in different file
than the devirtualized call, we get undefined symbol on the partition.

This problem is not limited to WHOPR only. Same problem can appear in
normal compilation when call to COMDAT function is devirtualized after
COMDAT was declared dead.

This patch adds neccesary checking and prevents devirtualization when we
can't anymore.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* gimple-fold.c (gimple_fold_obj_type_ref_known_binfo): Check that
	function is still available to fold into.
Diego Novillo - July 6, 2010, 12:11 a.m.
On Mon, Jul 5, 2010 at 23:50, Jan Hubicka <hubicka@ucw.cz> wrote:

>   fndecl = TREE_VALUE (v);
> +  node = cgraph_get_node (fndecl);
> +  /* When cgraph node is missing and function is public, we can not

s/can not/cannot/

Shouldn't this check be earlier instead of when we are trying to fold?
 But other folders behave this way, so this is OK too.


Diego.
Jan Hubicka - July 6, 2010, 12:16 a.m.
> On Mon, Jul 5, 2010 at 23:50, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> >   fndecl = TREE_VALUE (v);
> > +  node = cgraph_get_node (fndecl);
> > +  /* When cgraph node is missing and function is public, we can not
> 
> s/can not/cannot/
> 
> Shouldn't this check be earlier instead of when we are trying to fold?
>  But other folders behave this way, so this is OK too.

Well, this is the first time we actually see the function, so I don't think we can
check anything earlier.

Honza
Diego Novillo - July 6, 2010, 12:32 a.m.
On Tue, Jul 6, 2010 at 00:16, Jan Hubicka <hubicka@ucw.cz> wrote:

> Well, this is the first time we actually see the function, so I don't think we can
> check anything earlier.

Ah, yes, never mind.  I had the wrong memory about this code.


Diego.
Richard Guenther - July 6, 2010, 9:23 a.m.
On Tue, 6 Jul 2010, Jan Hubicka wrote:

> Hi,
> this patch solves problem with building Mozilla javascript shell with
> -fwhole-program and WHOPR.  The problem here is that cloning causes
> devirtualization.  This devirtualization is recognized during virtual clone
> materialization (i.e. late) because Martin did not merge his IPA
> devritualization code yet.
> 
> Because WHOPR brings the function static and house it in different file
> than the devirtualized call, we get undefined symbol on the partition.
> 
> This problem is not limited to WHOPR only. Same problem can appear in
> normal compilation when call to COMDAT function is devirtualized after
> COMDAT was declared dead.
> 
> This patch adds neccesary checking and prevents devirtualization when we
> can't anymore.

Hm, I don't understand.  When there is a virtual call we should still
have the functions address taken and thus cannot eliminate it.  So
why can this be a problem?  At least I don't see how it can be for
non-WHOPR.  COMDAT cannot be declared dead when there is still a
virtual call to it.

What am I missing?

Richard.

> Bootstrapped/regtested x86_64-linux, OK?
> 
> Honza
> 
> 	* gimple-fold.c (gimple_fold_obj_type_ref_known_binfo): Check that
> 	function is still available to fold into.
> Index: gimple-fold.c
> ===================================================================
> --- gimple-fold.c	(revision 161847)
> +++ gimple-fold.c	(working copy)
> @@ -1351,6 +1351,7 @@ gimple_fold_obj_type_ref_known_binfo (HO
>  {
>    HOST_WIDE_INT i;
>    tree v, fndecl;
> +  struct cgraph_node *node;
>  
>    v = BINFO_VIRTUALS (known_binfo);
>    i = 0;
> @@ -1362,6 +1363,14 @@ gimple_fold_obj_type_ref_known_binfo (HO
>      }
>  
>    fndecl = TREE_VALUE (v);
> +  node = cgraph_get_node (fndecl);
> +  /* When cgraph node is missing and function is public, we can not
> +     devirtualize.  This can happen in WHOPR when the actual method
> +     ends up in other partition, because we found devirtualization
> +     possibility too late.  */
> +  if ((!node || !node->analyzed)
> +      && (!TREE_PUBLIC (fndecl) || DECL_COMDAT (fndecl)))
> +    return NULL;
>    return build_fold_addr_expr (fndecl);
>  }
>  
> 
>
Jan Hubicka - July 6, 2010, 11:32 a.m.
> On Tue, 6 Jul 2010, Jan Hubicka wrote:
> 
> > Hi,
> > this patch solves problem with building Mozilla javascript shell with
> > -fwhole-program and WHOPR.  The problem here is that cloning causes
> > devirtualization.  This devirtualization is recognized during virtual clone
> > materialization (i.e. late) because Martin did not merge his IPA
> > devritualization code yet.
> > 
> > Because WHOPR brings the function static and house it in different file
> > than the devirtualized call, we get undefined symbol on the partition.
> > 
> > This problem is not limited to WHOPR only. Same problem can appear in
> > normal compilation when call to COMDAT function is devirtualized after
> > COMDAT was declared dead.
> > 
> > This patch adds neccesary checking and prevents devirtualization when we
> > can't anymore.
> 
> Hm, I don't understand.  When there is a virtual call we should still
> have the functions address taken and thus cannot eliminate it.  So
> why can this be a problem?  At least I don't see how it can be for
> non-WHOPR.  COMDAT cannot be declared dead when there is still a
> virtual call to it.
> 
> What am I missing?

Hmm, in non-WHOPR one would probably need keyed vtable containing pointer
to non-keyed COMDAT functions from ancestor.  This way we eliminate vtable,
but keep vinfos.  But I am not sure it happens in reality.

With WHOPR it is easy - since we don't see any connection in between
function and method before we devirtualize, we end up putting method
into different partition.  In any case we want devirutalization happen
early, so this is not much of problem.
Only thing I worry about is that I believe we should handle this already
- it is the case when ipa-cp creates clone that allows devirutalizing.
This is same to what inliner does. Martin?

Honza

Patch

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 161847)
+++ gimple-fold.c	(working copy)
@@ -1351,6 +1351,7 @@  gimple_fold_obj_type_ref_known_binfo (HO
 {
   HOST_WIDE_INT i;
   tree v, fndecl;
+  struct cgraph_node *node;
 
   v = BINFO_VIRTUALS (known_binfo);
   i = 0;
@@ -1362,6 +1363,14 @@  gimple_fold_obj_type_ref_known_binfo (HO
     }
 
   fndecl = TREE_VALUE (v);
+  node = cgraph_get_node (fndecl);
+  /* When cgraph node is missing and function is public, we can not
+     devirtualize.  This can happen in WHOPR when the actual method
+     ends up in other partition, because we found devirtualization
+     possibility too late.  */
+  if ((!node || !node->analyzed)
+      && (!TREE_PUBLIC (fndecl) || DECL_COMDAT (fndecl)))
+    return NULL;
   return build_fold_addr_expr (fndecl);
 }