[v2] ARM: Add pointer guard support.

Add support for pointer mangling in glibc internal structures in C
and assembler code.

ports/ChangeLog.arm:

2013-09-27  Will Newton  <will.newton@linaro.org>

	* sysdeps/arm/__longjmp.S (__longjmp): Demangle fp, sp
	and lr when restoring register values.
	* sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Remove
	sp and lr from list and replace fp with a4.
	* sysdeps/arm/jmpbuf-unwind.h (_jmpbuf_sp): New function.
	(_JMPBUF_UNWINDS_ADJ): Call _jmpbuf_sp.
	* sysdeps/arm/setjmp.S (__sigsetjmp): Mangle fp, sp and lr
	before storing register values.
	* sysdeps/arm/sysdep.h (LDST_GLOBAL): New macro.
	* sysdeps/unix/sysv/linux/arm/sysdep.h (PTR_MANGLE): New macro.
	(PTR_DEMANGLE): Likewise. (PTR_MANGLE2): Likewise.
	(PTR_DEMANGLE2): Likewise.
---
 ports/sysdeps/arm/__longjmp.S              | 14 +++++++++++
 ports/sysdeps/arm/include/bits/setjmp.h    |  5 ++--
 ports/sysdeps/arm/jmpbuf-unwind.h          | 13 +++++++++-
 ports/sysdeps/arm/setjmp.S                 | 14 +++++++++++
 ports/sysdeps/arm/sysdep.h                 | 11 +++++++++
 ports/sysdeps/unix/sysv/linux/arm/sysdep.h | 39 +++++++++++++++++++++++++++---
 6 files changed, 90 insertions(+), 6 deletions(-)

Changes in v2:
 - Use a global instead of a thread local variable to store the guard value
 - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
   but without it I get undefined references to __pointer_chk_guard_local.

On Fri, 27 Sep 2013, Will Newton wrote:

>  - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
>    but without it I get undefined references to __pointer_chk_guard_local.

Please don't just state the symptom.  Please give a more detailed analysis 
of the issue, explaining *why* those undefined references arise - what 
code gets used where in nscd, what semantics it is meant to have there, 
what the correct way is to access the relevant variable in that context, 
and what it is specifically about nscd that makes IS_IN_nscd (rather than 
some other conditional that would also cover other executables built with 
glibc) the logically correct conditional, and whether your logic applies 
also to --disable-shared builds of glibc when nscd would be statically 
linked.

In general, neither this patch nor the previous iteration includes any 
rationale - you state what you do, but not anything about why that's a 
good thing to do in the first place, or what choices were made and the 
basis on which they were made.  Is there some overall design document 
(past mailing list message, etc.) explaining "pointer mangling in glibc 
internal structures" and discussing what macros should be defined, with 
what semantics, and used where?  If so, point to it in your submission.  
If not, your patch submission should include something of that explanation 
itself (with reference to other architectures where appropriate) - 
presumably you reverse-engineered this machinery yourself to write the 
patch, if there wasn't such an existing document, in which case you should 
write up the results of your reverse-engineering to benefit the reviewer 
and people implementing this for other architectures, rather than 
expecting people to repeat it again.  (An alternative to putting it all in 
the patch submission is to create such detailed documentation for pointer 
mangling on the wiki, then point to the wiki page in the patch submission 
email.  But any ARM-specific rationale should still go in the email.)

You define a macro LDST_GLOBAL.  This needs a comment explaining its 
semantics in detail, like the other nearby macros in sysdep.h.  You can 
avoid such detailed comments on the mangling macros if they have 
architecture-independent semantics documented somewhere appropriate, but 
not for any new architecture-specific macro.

On 27 September 2013 17:20, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Fri, 27 Sep 2013, Will Newton wrote:
>
>>  - The bit I don't like in this version of the patch is the test for !IS_IN_nscd,
>>    but without it I get undefined references to __pointer_chk_guard_local.
>
> Please don't just state the symptom.  Please give a more detailed analysis
> of the issue, explaining *why* those undefined references arise - what
> code gets used where in nscd, what semantics it is meant to have there,
> what the correct way is to access the relevant variable in that context,
> and what it is specifically about nscd that makes IS_IN_nscd (rather than
> some other conditional that would also cover other executables built with
> glibc) the logically correct conditional, and whether your logic applies
> also to --disable-shared builds of glibc when nscd would be statically
> linked.

It appears that __pointer_chk_guard is used by libc.so, but
__pointer_chk_guard_local is used by ldso and libc.a. I guess it would
be nicer to change the logic to !SHARED && !IS_IN_libc or similar. I'm
reverse engineering this stuff so I was hoping someone with more
knowledge of this stuff than I could clear up what is the intended
behaviour.

> In general, neither this patch nor the previous iteration includes any
> rationale - you state what you do, but not anything about why that's a
> good thing to do in the first place, or what choices were made and the

I had rather assumed that argument had been made by every other port
including this feature.

> basis on which they were made.  Is there some overall design document
> (past mailing list message, etc.) explaining "pointer mangling in glibc
> internal structures" and discussing what macros should be defined, with
> what semantics, and used where?  If so, point to it in your submission.

AFAIK there is this blog post:

http://udrepper.livejournal.com/13393.html

It's a bit light on detail and doesn't go as far as specifying the macros used.

> If not, your patch submission should include something of that explanation
> itself (with reference to other architectures where appropriate) -
> presumably you reverse-engineered this machinery yourself to write the
> patch, if there wasn't such an existing document, in which case you should
> write up the results of your reverse-engineering to benefit the reviewer
> and people implementing this for other architectures, rather than
> expecting people to repeat it again.  (An alternative to putting it all in
> the patch submission is to create such detailed documentation for pointer
> mangling on the wiki, then point to the wiki page in the patch submission
> email.  But any ARM-specific rationale should still go in the email.)

Ok.

> You define a macro LDST_GLOBAL.  This needs a comment explaining its
> semantics in detail, like the other nearby macros in sysdep.h.  You can
> avoid such detailed comments on the mangling macros if they have
> architecture-independent semantics documented somewhere appropriate, but
> not for any new architecture-specific macro.

Ok. I'll update the patch and resubmit.