This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [RFC] [PATCH] [Aarch64] : Stack guard support in glibc
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Venkataramanan Kumar <venkataramanan dot kumar at linaro dot org>
- Cc: libc-ports at sourceware dot org, Marcus Shawcroft <marcus dot shawcroft at arm dot com>, Marcus Shawcroft <marcus dot shawcroft at gmail dot com>, Patch Tracking <patch at linaro dot org>
- Date: Tue, 27 Aug 2013 22:13:30 -0400
- Subject: Re: [RFC] [PATCH] [Aarch64] : Stack guard support in glibc
- Authentication-results: sourceware.org; auth=none
- References: <CAJK_mQ2V=Q4K-wRyOYaPcvD-QhkkAdtMEUrE+pN3NzBJh1d8dw at mail dot gmail dot 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.