From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5304 invoked by alias); 27 Sep 2013 19:46:35 -0000 Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org Received: (qmail 5295 invoked by uid 89); 27 Sep 2013 19:46:35 -0000 Received: from mail-ie0-f182.google.com (HELO mail-ie0-f182.google.com) (209.85.223.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 27 Sep 2013 19:46:35 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,NO_RELAYS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f182.google.com Received: by mail-ie0-f182.google.com with SMTP id aq17so5039055iec.13 for ; Fri, 27 Sep 2013 12:46:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=shTCe3/vqSJIg/wEvQ2pl9BD2cKqLZOeD1CUfCs1Hlc=; b=cf9RDQzJWF9q9I/qrEkDQxBPwfucHJHTtTWy0k30GDCtMcgKi6VEsGbIb0TDMKq/dH gKZQeYQPwVve850VdnWdKFLviW//vCYPbXIlCEDdA0xaLJdJ7ZKjUhEU/OJCNMGGxZ9q krPr+Lfd8+kNA2cIjPucGZ4JM5RCD3tBDpnlK/leYl4imJIiqUHESjUaINNtehPOMErG TECROdTD25//rgAM1Iz8p6MLd1WeEEMdbOVeNDsOnn0YDyu1lb5gnpZPamvm8Mv90axG e+K7QPgbq1pKjgoGjVh56q4Iv+4qS00hDU/GiXHXKC0SD3aSUQYkNGApCoVLmxE0q2C4 rqfA== X-Gm-Message-State: ALoCoQm2lzLQe4/6NZu0lfUxmLqTtGCuJb4PDkncNpXQ4MzCszVdqJbUZTzG6H/rc/tVaAJXKdg9 MIME-Version: 1.0 X-Received: by 10.50.122.40 with SMTP id lp8mr3775960igb.24.1380311191854; Fri, 27 Sep 2013 12:46:31 -0700 (PDT) Received: by 10.64.20.52 with HTTP; Fri, 27 Sep 2013 12:46:31 -0700 (PDT) In-Reply-To: References: <5245A8FF.4000602@linaro.org> Date: Fri, 27 Sep 2013 19:46:00 -0000 Message-ID: Subject: Re: [PATCH v2] ARM: Add pointer guard support. From: Will Newton To: "Joseph S. Myers" Cc: "libc-ports@sourceware.org" , Patch Tracking Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00153.txt.bz2 On 27 September 2013 17:20, Joseph S. Myers 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