Re: [RFC] [PATCH] [Aarch64] : Stack guard support in glibc

["Carlos O'Donell" <carlos@redhat.com>]

On 08/26/2013 12:15 AM, Venkataramanan Kumar wrote:
> I also checked the canary value and keeps changing from run to run.

Is it faster than accessing a global?

> glibc.tls.stack.guard.aarch64.diff

Please review:
http://sourceware.org/glibc/wiki/Contribution%20checklist

Please run the entire testsuite and report if there are no regressions.

You need a ChangeLog and you should also write up some text for a NEWS
entry that will go out with 2.19 talking about the new feature for ARM.
 
> Index: tls.h
> ===================================================================
> --- tls.h	(revision 23742)
> +++ tls.h	(working copy)
> @@ -68,10 +68,15 @@
>  # define TLS_TCB_SIZE		sizeof (tcbhead_t)
>  
>  /* This is the size we need before TCB.  */
> -# define TLS_PRE_TCB_SIZE	sizeof (struct pthread)
> +# define TLS_PRE_TCB_SIZE \
> +  (sizeof (struct pthread)                                              \
> +   + (PTHREAD_STRUCT_END_PADDING < 2 * sizeof (uintptr_t)               \
> +      ? ((2 * sizeof (uintptr_t) + __alignof__ (struct pthread) - 1)    \
> +         & ~(__alignof__ (struct pthread) - 1))                         \
> +      : 0))

Please use ALIGN_UP or ALIGN_DOWN from libc-internal.h.

I know you're copying this from IA64, but it should still use
the newer macros.
  
>  /* Alignment requirements for the TCB.  */
> -# define TLS_TCB_ALIGN		__alignof__ (tcbhead_t)
> +# define TLS_TCB_ALIGN __alignof__ (struct pthread)
>  /* Install the dtv pointer.  The pointer passed is to the element with
>     index -1 which contain the length.  */
> @@ -98,12 +103,28 @@
>  
>  /* Return the thread descriptor for the current thread.  */
>  # define THREAD_SELF \
> - ((struct pthread *)__builtin_thread_pointer () - 1)
> + ((struct pthread *)((char *) __builtin_thread_pointer () - TLS_PRE_TCB_SIZE))
>  
>  /* Magic for libthread_db to know how to do THREAD_SELF.  */
>  # define DB_THREAD_SELF \
> -  CONST_THREAD_AREA (64, sizeof (struct pthread))
> +  CONST_THREAD_AREA (64, TLS_PRE_TCB_SIZE)

Did you check to see if this impacts gdb in any way?

e.g. Install new glibc into a system, then build and run the gdb testsuite
and see if anything fails?

> +/* Set the stack guard field in TCB head.  */
> +#define THREAD_SET_STACK_GUARD(value) \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-1] = (value))
> +#define THREAD_COPY_STACK_GUARD(descr) \
> +  (((uintptr_t *) ((char *) (descr) + TLS_PRE_TCB_SIZE))[-1] \
> +   = ((uintptr_t *) __builtin_thread_pointer ())[-1])
> +
> +/* Set the pointer guard field in TCB head.  */
> +#define THREAD_GET_POINTER_GUARD() \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-2])
> +#define THREAD_SET_POINTER_GUARD(value) \
> +  (((uintptr_t *) __builtin_thread_pointer ())[-2] = (value))
> +#define THREAD_COPY_POINTER_GUARD(descr) \
> +  (((uintptr_t *) ((char *) (descr) + TLS_PRE_TCB_SIZE))[-2] \
> +   = THREAD_GET_POINTER_GUARD ())
> +

Please see the Style and Conventions:
http://sourceware.org/glibc/wiki/Style_and_Conventions#Nested_C_Preprocessor_Directives

>  /* Access to data in the thread descriptor is easy.  */
>  # define THREAD_GETMEM(descr, member) \
>    descr->member

I've only done a very light review here, please repost v2
and we can do a more thorough review.

Cheers,
Carlos.