This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [PATCH v2] ARM: Add pointer guard support.
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Will Newton <will dot newton at linaro dot org>
- Cc: <libc-ports at sourceware dot org>, <patches at linaro dot org>
- Date: Fri, 27 Sep 2013 16:20:22 +0000
- Subject: Re: [PATCH v2] ARM: Add pointer guard support.
- Authentication-results: sourceware.org; auth=none
- References: <5245A8FF dot 4000602 at linaro dot 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