From patchwork Thu Nov 24 01:15:11 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 83776 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp2945883qge; Wed, 23 Nov 2016 17:15:44 -0800 (PST) X-Received: by 10.98.7.151 with SMTP id 23mr5695318pfh.5.1479950144865; Wed, 23 Nov 2016 17:15:44 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id m20si7811588pli.206.2016.11.23.17.15.44 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 23 Nov 2016 17:15:44 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-442494-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-442494-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-442494-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=rhYSkeu0YBXLKu+fG KTEE8Rbanwi1nbyHzKv0TVMWDQssz+yU2j8+6IAwzjcTNplscYy/Xt3B3KYF0WRX ecXEyH8lnBB93bWuciSdipDV+VzefJUkYvKTKeNghbJ4h1A/SLP9U2Y1FEkY8Tt3 9atDUqDs/Ju9GaSowKCH6bs6RA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:cc:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=Zh+SuEKD8qLWcr+GjLsBUP2 w7LI=; b=YcrLp5rXfrfHWhfYBcBEFfUJ0kL/8KA/sRjB0zwnk/Bv7L4HbGf57Wq GH22xW421KrY2knKiG6zYa39LRLpbzr0WvDEcdKATirUz69Uim2buMAysMDV7kCA D6RXzMwF/o67LkQ2Tq85DVzXTN7EK9BSQTOUD4qomNOdiAUHsIgc= Received: (qmail 27523 invoked by alias); 24 Nov 2016 01:15:27 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 27455 invoked by uid 89); 24 Nov 2016 01:15:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=xcalloc X-HELO: mail-qt0-f196.google.com Received: from mail-qt0-f196.google.com (HELO mail-qt0-f196.google.com) (209.85.216.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 24 Nov 2016 01:15:19 +0000 Received: by mail-qt0-f196.google.com with SMTP id m48so1748972qta.2 for ; Wed, 23 Nov 2016 17:15:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=Mg0+evymwGnBeakjq67B/pF99Yyxp/v6PGW+o5IFGlc=; b=WmL2igfCN59+AZbfRF3JhR0VgdlXJvTFKkLLb8g4eXqbiFvCpK5ssjIdVR4Rt4OJVj JIa+CwWnGAk3wixmPqqYiq7ZaWew2CRCcOlDBmFgERpoCbuFyun7qdZXOO0f1SJAZa09 0GmiBWG69Cb1foKj+17L40tpw/fYbkigIf+00rHBMzmlTSQ8mXjjVLciBqs50pt0iRjg uROH4JihsN3Ge50w0g2AZiv8HQCW0eKHZrcRIdtm5HlvwlKfVyufvZVZxgrh8hMkWHFR y/QdC4e7x2BOYPzgQmUM3XBKDXFFO9kn8s59FhC01p+1qR6NqERpOgcO45TEVqsnxAMO mb+A== X-Gm-Message-State: AKaTC02/dKTFNZQ0O8Erm8aI2Urkj4ZnA7BkCrTARhjBihc5GZ0QBTlxEG7MLu+j1z7EUg== X-Received: by 10.200.38.72 with SMTP id v8mr5973169qtv.292.1479950114390; Wed, 23 Nov 2016 17:15:14 -0800 (PST) Received: from [192.168.0.26] (75-166-206-79.hlrn.qwest.net. [75.166.206.79]) by smtp.gmail.com with ESMTPSA id r186sm17736838qke.25.2016.11.23.17.15.13 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 23 Nov 2016 17:15:13 -0800 (PST) Subject: Re: [PATCH] avoid calling alloca(0) To: Jeff Law , Bernd Edlinger References: <2915e7f6-99ce-e8e2-2001-6b65bbaeb345@gmail.com> <17b7b776-dfba-c3e9-3812-a1b82fcb5b40@gmail.com> <054dca99-7ba0-2363-ff47-8e3d759765b6@gmail.com> <03c10dd8-8d0a-e02a-ad6a-d3be12b23dba@gmail.com> Cc: Jakub Jelinek , "gcc-patches@gcc.gnu.org" From: Martin Sebor Message-ID: Date: Wed, 23 Nov 2016 18:15:11 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: X-IsSubscribed: yes On 11/23/2016 01:57 PM, Jeff Law wrote: > On 11/20/2016 04:06 PM, Martin Sebor wrote: >> On 11/20/2016 01:03 AM, Bernd Edlinger wrote: >>> On 11/20/16 00:43, Martin Sebor wrote: >>>> As best I can tell the result isn't actually used (the code that >>>> uses the result gets branched over). GCC just doesn't see it. >>>> I verified this by changing the XALLOCAVEC macro to >>>> >>>> #define XALLOCAVEC(T, N) (N ? alloca (sizeof (T) * N) : 0) >>>> >>>> and bootstrapping and running the regression test suite with >>>> no apparent regressions. >>>> >>> >>> I would like this solution with braces around N better than >>> allocating one element when actually zero were requested. >>> >>> The disadvantage of N+1 or N+!N is that it will hide true >>> programming errors from the sanitizer. >> >> I agree. Let me post an updated patch with the above fix and >> leave it to others to choose which approach to go with. > Just so I'm clear, this part of the discussions is talking about > mitigation strategies within GCC itself, right? That's right. > Can't we just > gcc_assert (x != 0) before the problematical calls? That avoids > unnecessary over-allocation and gets us a clean ICE if we were to try > and call alloca with a problematical value. gcc_assert works only in some instances (e.g., in c-ada-spec.c:191) but not in others because some actually do make the alloca(0) call at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and tree-ssa-threadedge.c:344 assert during bootstrap. After reviewing a few more of the XALLOCAVEC calls in the affected files I suspect that those to alloca(0) pointed out by the warning may be just a subset that GCC happens to see thanks to constant propagation. If that's so then changing this subset to alloca(N + !N) or some such is probably not the right approach because it will miss all the other calls where GCC doesn't see that N is zero. I think the most robust solution is to do as Bernd suggests: change XALLOCAVEC as shown above. That will let us prevent any and all unsafe assumptions about the result of alloca(0) being either non-null or distinct. The other approach would be to change XALLOCAVEC to add 1 to the byte count. That would be in line with what XMALLOC does. > I'm not sure we need to break things up. I haven't seen a good argument > for that yet. > > Is there an updated patch to post? Or has it already been posted? Given the above I suggest going with one of the attached two patches. Martin include/ChangeLog: * libiberty.h (XALLOCAVEC): Avoid calling alloca with a zero argument. Index: include/libiberty.h =================================================================== --- include/libiberty.h (revision 242768) +++ include/libiberty.h (working copy) @@ -353,7 +353,7 @@ extern unsigned int xcrc32 (const unsigned char *, /* Array allocators. */ -#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N))) +#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N) + 1)) #define XNEWVEC(T, N) ((T *) xmalloc (sizeof (T) * (N))) #define XCNEWVEC(T, N) ((T *) xcalloc ((N), sizeof (T))) #define XDUPVEC(T, P, N) ((T *) xmemdup ((P), sizeof (T) * (N), sizeof (T) * (N)))