From: Will Newton <will.newton@linaro.org>
To: "Joseph S. Myers" <joseph@codesourcery.com>
Cc: "libc-ports@sourceware.org" <libc-ports@sourceware.org>,
Patch Tracking <patches@linaro.org>
Subject: Re: [PATCH v2] ARM: Add pointer guard support.
Date: Fri, 27 Sep 2013 19:46:00 -0000 [thread overview]
Message-ID: <CANu=Dmi_odV2AENUaR_=5wgstpWnP1Z-3=wDDSON4jWaTm-RvQ@mail.gmail.com> (raw)
In-Reply-To: <Pine.LNX.4.64.1309271603370.7561@digraph.polyomino.org.uk>
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
prev parent reply other threads:[~2013-09-27 19:46 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
2013-09-27 19:46 ` Will Newton [this message]
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='CANu=Dmi_odV2AENUaR_=5wgstpWnP1Z-3=wDDSON4jWaTm-RvQ@mail.gmail.com' \
--to=will.newton@linaro.org \
--cc=joseph@codesourcery.com \
--cc=libc-ports@sourceware.org \
--cc=patches@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).