From: "Joseph S. Myers" <joseph@codesourcery.com>
To: Will Newton <will.newton@linaro.org>
Cc: <libc-ports@sourceware.org>, <patches@linaro.org>
Subject: Re: [PATCH v2] ARM: Add pointer guard support.
Date: Fri, 27 Sep 2013 16:20:00 -0000 [thread overview]
Message-ID: <Pine.LNX.4.64.1309271603370.7561@digraph.polyomino.org.uk> (raw)
In-Reply-To: <5245A8FF.4000602@linaro.org>
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
next prev parent 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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.1309271603370.7561@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=libc-ports@sourceware.org \
--cc=patches@linaro.org \
--cc=will.newton@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).