public inbox for
 help / color / mirror / Atom feed
From: "Joseph S. Myers" <>
To: Will Newton <>
Cc: <>, <>
Subject: Re: [PATCH v2] ARM: Add pointer guard support.
Date: Fri, 27 Sep 2013 16:20:00 -0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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 

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

  reply	other threads:[~2013-09-27 16:20 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-27 15:49 Will Newton
2013-09-27 16:20 ` Joseph S. Myers [this message]
2013-09-27 19:46   ` Will Newton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).