public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
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

      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).