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

["Joseph S. Myers" <joseph at codesourcery dot com>]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com