Re: [PATCH v2] ARM: Add pointer guard support.

[Will Newton <will dot newton at linaro dot org>]

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.

-- 
Will Newton
Toolchain Working Group, Linaro