diff mbox

Relat TLS model merging in lto-symtab

Message ID 20150128195907.GA20192@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Jan. 28, 2015, 7:59 p.m. UTC
Hi,
newer Firefox trees fails to build because jsmalloc contain variable that is
declared as tls-initial-exec in one unit but used as tls-global-dynamic
in others.

As discussed with Jakub on IRC, the linker supports some model transitions,
so we should do the same in symtab.c too.

Bootstrapped/regtested x86_64-linux, tested on firefox, comitted.

Honza

	* lto-symtab.c (lto_varpool_replace_node): Merge TLS models.

Comments

Jakub Jelinek Jan. 28, 2015, 8:18 p.m. UTC | #1
On Wed, Jan 28, 2015 at 08:59:07PM +0100, Jan Hubicka wrote:
> --- lto-symtab.c	(revision 220212)
> +++ lto-symtab.c	(working copy)
> @@ -158,11 +158,44 @@ lto_varpool_replace_node (varpool_node *
>  
>    if (vnode->tls_model != prevailing_node->tls_model)
>      {
> -      error_at (DECL_SOURCE_LOCATION (vnode->decl),
> -		"%qD is defined as %s", vnode->decl, tls_model_names [vnode->tls_model]);
> -      inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
> -	      "previously defined here as %s",
> -	      tls_model_names [prevailing_node->tls_model]);
> +      bool error = false;
> +
> +      /* Non-TLS and TLS never mix together.  Also emulated model is not
> +	 compatible with anything else.  */
> +      if (prevailing_node->tls_model == TLS_MODEL_NONE
> +	  || prevailing_node->tls_model == TLS_MODEL_EMULATED
> +	  || vnode->tls_model == TLS_MODEL_NONE
> +	  || vnode->tls_model == TLS_MODEL_EMULATED)
> +	error = true;
> +      /* Linked is silently supporting transitions

Linker instead of Linked.

> +	 GD -> IE, GD -> LE, LD -> LE, IE -> LE, LD -> IE.

Looking at linker, there is actually no LD -> IE transition, so
linker if there were LD and IE accesses to the same TLS var and no
LE accesses, then the linker would probably keep LD and IE accesses
untouched.  That said, this is before the RTL is generated, and IE is
cheaper, so I think it is fine to also transform LD -> IE in the compiler.

I think you could do similarly a GD -> LD transition, if anything is
accessed using LD model, then by definition it has to be defined in the
current shared library (or executable), not using a definition from
somewhere else, and if something else accesses it globally, it is fine to
access it locally too.  Just make it clear what transitions are done
by the linker and what you do on top of that.
Though, once you add the LD -> IE and GD -> LD transitions the linker
doesn't do, you can just do
prevailing_node->tls_model = MAX (prevailing_node->tls_model, vnode->tls_model);
after checking the NONE/EMULATED cases.

> +	 Do the same transitions and error out on others.  */
> +      else if ((prevailing_node->tls_model == TLS_MODEL_REAL

I'd use TLS_MODEL_GLOBAL_DYNAMIC instead of TLS_MODEL_REAL, it is the
same value, but the former makes it more obvious and will better match
the comment.

> +		|| prevailing_node->tls_model == TLS_MODEL_LOCAL_DYNAMIC)
> +	       && (vnode->tls_model == TLS_MODEL_INITIAL_EXEC
> +		   || vnode->tls_model == TLS_MODEL_LOCAL_EXEC))
> +	prevailing_node->tls_model = vnode->tls_model;
> +      else if ((vnode->tls_model == TLS_MODEL_REAL
> +		|| vnode->tls_model == TLS_MODEL_LOCAL_DYNAMIC)
> +	       && (prevailing_node->tls_model == TLS_MODEL_INITIAL_EXEC
> +		   || prevailing_node->tls_model == TLS_MODEL_LOCAL_EXEC))
> +	;
> +      else if (prevailing_node->tls_model == TLS_MODEL_INITIAL_EXEC
> +	       && vnode->tls_model == TLS_MODEL_LOCAL_EXEC)
> +	prevailing_node->tls_model = vnode->tls_model;
> +      else if (vnode->tls_model == TLS_MODEL_INITIAL_EXEC
> +	       && prevailing_node->tls_model == TLS_MODEL_LOCAL_EXEC)
> +	;
> +      else
> +	error = true;
> +      if (error)
> +	{
> +	  error_at (DECL_SOURCE_LOCATION (vnode->decl),
> +		    "%qD is defined with tls model %s", vnode->decl, tls_model_names [vnode->tls_model]);
> +	  inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
> +		  "previously defined here as %s",
> +		  tls_model_names [prevailing_node->tls_model]);
> +	}
>      }
>    /* Finally remove the replaced node.  */
>    vnode->remove ();

	Jakub
diff mbox

Patch

Index: lto-symtab.c
===================================================================
--- lto-symtab.c	(revision 220212)
+++ lto-symtab.c	(working copy)
@@ -158,11 +158,44 @@  lto_varpool_replace_node (varpool_node *
 
   if (vnode->tls_model != prevailing_node->tls_model)
     {
-      error_at (DECL_SOURCE_LOCATION (vnode->decl),
-		"%qD is defined as %s", vnode->decl, tls_model_names [vnode->tls_model]);
-      inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
-	      "previously defined here as %s",
-	      tls_model_names [prevailing_node->tls_model]);
+      bool error = false;
+
+      /* Non-TLS and TLS never mix together.  Also emulated model is not
+	 compatible with anything else.  */
+      if (prevailing_node->tls_model == TLS_MODEL_NONE
+	  || prevailing_node->tls_model == TLS_MODEL_EMULATED
+	  || vnode->tls_model == TLS_MODEL_NONE
+	  || vnode->tls_model == TLS_MODEL_EMULATED)
+	error = true;
+      /* Linked is silently supporting transitions
+	 GD -> IE, GD -> LE, LD -> LE, IE -> LE, LD -> IE.
+	 Do the same transitions and error out on others.  */
+      else if ((prevailing_node->tls_model == TLS_MODEL_REAL
+		|| prevailing_node->tls_model == TLS_MODEL_LOCAL_DYNAMIC)
+	       && (vnode->tls_model == TLS_MODEL_INITIAL_EXEC
+		   || vnode->tls_model == TLS_MODEL_LOCAL_EXEC))
+	prevailing_node->tls_model = vnode->tls_model;
+      else if ((vnode->tls_model == TLS_MODEL_REAL
+		|| vnode->tls_model == TLS_MODEL_LOCAL_DYNAMIC)
+	       && (prevailing_node->tls_model == TLS_MODEL_INITIAL_EXEC
+		   || prevailing_node->tls_model == TLS_MODEL_LOCAL_EXEC))
+	;
+      else if (prevailing_node->tls_model == TLS_MODEL_INITIAL_EXEC
+	       && vnode->tls_model == TLS_MODEL_LOCAL_EXEC)
+	prevailing_node->tls_model = vnode->tls_model;
+      else if (vnode->tls_model == TLS_MODEL_INITIAL_EXEC
+	       && prevailing_node->tls_model == TLS_MODEL_LOCAL_EXEC)
+	;
+      else
+	error = true;
+      if (error)
+	{
+	  error_at (DECL_SOURCE_LOCATION (vnode->decl),
+		    "%qD is defined with tls model %s", vnode->decl, tls_model_names [vnode->tls_model]);
+	  inform (DECL_SOURCE_LOCATION (prevailing_node->decl),
+		  "previously defined here as %s",
+		  tls_model_names [prevailing_node->tls_model]);
+	}
     }
   /* Finally remove the replaced node.  */
   vnode->remove ();