Re: [Patch, updated] Make emulated TLS lto-friendly.

[Richard Henderson <rth at redhat dot com>]

On 07/03/2010 07:10 AM, IainS wrote:
> +      return GS_OK ;

s/ ;/;/g

There are at least 2 occurrences.

> +      if ((DECL_TLS_MODEL (decl) >= TLS_MODEL_EMULATED))

Extra ().

Possibly better written as != TLS_MODEL_NONE as well.

> +	  if (value && DECL_P (value) && 
> +	      (DECL_TLS_MODEL (value) >= TLS_MODEL_EMULATED))

> +  if (!DECL_INITIAL (to) && 
> +      DECL_INITIAL (decl) && (DECL_INITIAL (decl) != error_mark_node)) 
> +    if (! DECL_EXTERNAL (to) && ! DECL_COMMON (to))

Coding style -- && should be on the new line.
Extra ().
Merge the unnecessarily nested ifs.

> +  /* ??? It is not enough to check DECL_COMMON because variables might be 
> +     allocated in other uninitialized sections.  However, this might still
> +     not be an adequate test.  */
> +  if (DECL_INITIAL (h->to))

Huh?  Why is DECL_COMMON not sufficient?

> +  /* Refuse to build a zero-sized item, first make sure there's
> +     a valid layout.  */
> +  if (DECL_SIZE (h->base.from) == 0)
> +    layout_decl (h->base.from, 0);

Really?  What is a testcase for which the decl is not layed out?
That really sounds like a bug elsewhere.

> +  gcc_assert (DECL_SIZE (h->base.from) != 0);

I'm pretty sure you can declare zero sized objects as a gcc extension.

$ cat z.c
__thread int x[0] __attribute__((common));
$ cat z.s
	.file	"z.c"
	.tls_common	x,0,4
	.ident	"GCC: (GNU) 4.4.4 20100630 (Red Hat 4.4.4-10)"
	.section	.note.GNU-stack,"",@progbits

So here's a test case that will trigger that abort.

> +  /* We don't emit the userland vars for emulated TLS - they should never
> +     get to here, only the control vars should be emitted.  */
> +  gcc_assert (targetm.have_tls  
> +	      || (TREE_CODE (decl) != VAR_DECL)
> +	      || !DECL_THREAD_LOCAL_P (decl));

Extra ().  I'm sure there are more, but I won't comment again.
This pattern occurs many places.  Pull this out to a function.

> +  /* If we need the var, and it's an emulated TLS entity, that
> +     means we need the control var.  */
> +  if (!targetm.have_tls && DECL_THREAD_LOCAL_P (node->decl))
> +    {
> +      struct varpool_node *cv_node;
> +      cv_node = varpool_node (emutls_decl (node->decl)) ;
> +      varpool_mark_needed_node (cv_node);
> +    }

Do you really need this kind of thing anymore, since you're exposing
the use of the control variable so early?  I would have thought that
varpool.c would no longer need any special-casing for !have_tls.

Finally, as a follow-up, I think the alias oracle needs to be taught what 
__builtin_emutls_get_address does.  Namely, return the address of the
associated TLS variable.  This is the only way I can think of to avoid
the optimization penalty that you're introducing because of exposing the
control variable and the function call at this point.


r~