From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 49C963858018 for ; Wed, 20 Jan 2021 08:18:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 49C963858018 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 13C81AB7A; Wed, 20 Jan 2021 08:18:08 +0000 (UTC) Date: Wed, 20 Jan 2021 09:18:08 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] builtins: Fix up two bugs in access_ref::inform_access [PR98721] In-Reply-To: <20210120075211.GM4020736@tucnak> Message-ID: References: <20210120075211.GM4020736@tucnak> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jan 2021 08:18:11 -0000 On Wed, 20 Jan 2021, Jakub Jelinek wrote: > Hi! > > The following patch fixes two bugs in the access_ref::inform_access function > (plus some formatting nits). > > The first problem is that ref can be various things, e.g. *_DECL, or > SSA_NAME, or IDENTIFIER_NODE. And allocfn is non-NULL only if ref is > (at least originally) an SSA_NAME initialized to the result of some > allocator function (but not e.g. __builtin_alloca_with_align which is > handled differently). > > A few lines above the last hunk of this patch in builtins.c, the code uses > if (mode == access_read_write || mode == access_write_only) > { > if (allocfn == NULL_TREE) > { > if (*offstr) > inform (loc, "at offset %s into destination object %qE of size %s", > offstr, ref, sizestr); > else > inform (loc, "destination object %qE of size %s", ref, sizestr); > return; > } > > if (*offstr) > inform (loc, > "at offset %s into destination object of size %s " > "allocated by %qE", offstr, sizestr, allocfn); > else > inform (loc, "destination object of size %s allocated by %qE", > sizestr, allocfn); > return; > } > so if allocfn is NULL, it prints whatever ref is, if it is non-NULL, > it prints instead the allocation function. But strangely the hunk > a few lines below wasn't consistent with that and instead printed the > first form only if DECL_P (ref) and would ICE if ref wasn't a decl but > still allocfn was NULL. Fixed by making it consistent what the code does > earlier. > > Another bug is that the code earlier contains an ugly hack for VLAs and was > assuming that SSA_NAME_IDENTIFIER must be non-NULL on the lhs of > __builtin_alloca_with_align. While that is likely true for the cases where > the compiler emits this builtin for VLAs (and it will also be true that > the name of the VLA in that case can be taken from that identifier up to the > first .), the builtin is user accessible as the testcase shows, so one can > have any other SSA_NAME in there. I think it would be better to add some > more reliable way how to identify VLA names corresponding to > __builtin_alloca_with_align allocations, perhaps internal fn or whatever, > but that is beyond the scope of this patch. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Thanks, Richard. > 2021-01-20 Jakub Jelinek > > PR tree-optimization/98721 > * builtins.c (access_ref::inform_access): Don't assume > SSA_NAME_IDENTIFIER must be non-NULL. Print messages about > object whenever allocfn is NULL, rather than only when DECL_P > is true. Use %qE instead of %qD for that. Formatting fixes. > > * gcc.dg/pr98721-1.c: New test. > * gcc.dg/pr98721-2.c: New test. > > --- gcc/builtins.c.jj 2021-01-18 19:07:16.022895507 +0100 > +++ gcc/builtins.c 2021-01-19 11:56:52.247070923 +0100 > @@ -4414,8 +4414,8 @@ access_ref::inform_access (access_mode m > MAXREF on which the result is based. */ > const offset_int orng[] = > { > - offrng[0] - maxref.offrng[0], > - wi::smax (offrng[1] - maxref.offrng[1], offrng[0]), > + offrng[0] - maxref.offrng[0], > + wi::smax (offrng[1] - maxref.offrng[1], offrng[0]), > }; > > /* Add the final PHI's offset to that of each of the arguments > @@ -4493,12 +4493,15 @@ access_ref::inform_access (access_mode m > /* Strip the SSA_NAME suffix from the variable name and > recreate an identifier with the VLA's original name. */ > ref = gimple_call_lhs (stmt); > - ref = SSA_NAME_IDENTIFIER (ref); > - const char *id = IDENTIFIER_POINTER (ref); > - size_t len = strcspn (id, ".$"); > - if (!len) > - len = strlen (id); > - ref = get_identifier_with_length (id, len); > + if (SSA_NAME_IDENTIFIER (ref)) > + { > + ref = SSA_NAME_IDENTIFIER (ref); > + const char *id = IDENTIFIER_POINTER (ref); > + size_t len = strcspn (id, ".$"); > + if (!len) > + len = strlen (id); > + ref = get_identifier_with_length (id, len); > + } > } > else > { > @@ -4557,13 +4560,13 @@ access_ref::inform_access (access_mode m > return; > } > > - if (DECL_P (ref)) > + if (allocfn == NULL_TREE) > { > if (*offstr) > - inform (loc, "at offset %s into source object %qD of size %s", > + inform (loc, "at offset %s into source object %qE of size %s", > offstr, ref, sizestr); > else > - inform (loc, "source object %qD of size %s", ref, sizestr); > + inform (loc, "source object %qE of size %s", ref, sizestr); > > return; > } > --- gcc/testsuite/gcc.dg/pr98721-1.c.jj 2021-01-19 12:15:03.825600828 +0100 > +++ gcc/testsuite/gcc.dg/pr98721-1.c 2021-01-19 12:14:24.730045488 +0100 > @@ -0,0 +1,14 @@ > +/* PR tree-optimization/98721 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +int > +foo (int n) > +{ > + if (n <= 0) > + { > + char vla[n]; /* { dg-message "source object 'vla' of size 0" } */ > + return __builtin_strlen (vla); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */ > + } > + return -1; > +} > --- gcc/testsuite/gcc.dg/pr98721-2.c.jj 2021-01-19 12:00:16.005742548 +0100 > +++ gcc/testsuite/gcc.dg/pr98721-2.c 2021-01-19 11:59:29.372275423 +0100 > @@ -0,0 +1,8 @@ > +/* PR tree-optimization/98721 */ > +/* { dg-do compile } */ > + > +int > +foo (void) > +{ > + return __builtin_strlen (__builtin_alloca_with_align (0, 16)); /* { dg-warning "'__builtin_strlen' reading 1 or more bytes from a region of size 0" } */ > +} /* { dg-message "source object '' of size 0" "" { target *-*-* } .-1 } */ > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)