From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.smtpout.orange.fr (smtp-23.smtpout.orange.fr [80.12.242.23]) by sourceware.org (Postfix) with ESMTPS id 146503858C2B for ; Mon, 6 Nov 2023 19:19:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 146503858C2B Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=orange.fr Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=orange.fr ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 146503858C2B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=80.12.242.23 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699298351; cv=none; b=b0cRTxZuPXQAEKMgUYVFGGx++juL13q+2NFlPqrnZmyT/Ew/Mch81A8wWv1eZD7Oe2BBtHAWoLeRCRoZFkbnyZENlh01iwxJcLxhYQW3ntI9vVg3Ad7PT9wZk3m/fnX1lM8O5Ts6N8U1P3p/X0gQrHH9QztsRo68nA32Lp90G14= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699298351; c=relaxed/simple; bh=6Xa9HDTjBr1ZLJYlSztGOU2uqd5vuL4veDqf4S5NRU8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=mQ6/fQWzHYQWEJyBzz4WfpnP5zANE+Qq+zyObDmUq7fUAk8sSJaMBtvBBneSU1s76mDJRi0TU6kzvQJTeyEi/O15e7pr8F2IIwcSvGx/9iS5mQ6DZOUi5FGZYYuWa9uHZBWWcC3Zwp+iNN/2tv4ibQHVC8IqG9i/4/tjLRocKc8= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [192.168.1.13] ([86.215.161.51]) by smtp.orange.fr with ESMTPA id 057rrZI7pp5Gd057rrqSyI; Mon, 06 Nov 2023 20:19:08 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=orange.fr; s=t20230301; t=1699298348; bh=fReHH7GborEGdGnaZ0Top8da97EHg1tRbgJAp1kySOo=; h=Date:Subject:To:References:From:In-Reply-To; b=RgCV2fhDbm2I79QXxhxUBmupwBcAC8Cp9Enfm2pQ3D6pZsaTazcS6eyD0zNtDQARt NE+r6hCxuaByb8nP8cUgAo1b7KTR9JD/Hq+BdwIzeJHp3hfSMKrBjWDgjXGfGw+FZ+ L1EYyfSy3uD7B+GjT6D0xOlDmfnkx3GIDGl5KfR5LUhJoIMNTt02xwSHktzWxSqaUw rKSA7PaKZZTQMwJTwapyWG9SdYfVFNTZGbHK2Zb5v08ZMtkDCUt6JWild2jgHRS+1c jbhdPTTYG1jEx/wpd83JHJooSnhF5JJrtUlwemYTVZZvLBujhvyhhPNX2QtzXLaS7r ERwfLkNsqhCxw== X-ME-Helo: [192.168.1.13] X-ME-Auth: bW9yaW4tbWlrYWVsQG9yYW5nZS5mcg== X-ME-Date: Mon, 06 Nov 2023 20:19:08 +0100 X-ME-IP: 86.215.161.51 Message-ID: <9fd8d832-51b6-4967-abad-510860ddafd0@orange.fr> Date: Mon, 6 Nov 2023 20:19:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/2] libgfortran: Remove empty array descriptor first dimension overwrite [PR112371] Content-Language: fr To: Harald Anlauf , Mikael Morin , gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org References: <20231106114325.828968-1-mikael@gcc.gnu.org> <20231106114325.828968-3-mikael@gcc.gnu.org> <5562f6b7-1b0d-4ff7-9797-8f1f58edc1f5@gmx.de> From: Mikael Morin In-Reply-To: <5562f6b7-1b0d-4ff7-9797-8f1f58edc1f5@gmx.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Le 06/11/2023 à 19:20, Harald Anlauf a écrit : > Hi Mikael, > > Am 06.11.23 um 12:43 schrieb Mikael Morin: >> Remove the forced overwrite of the first dimension of the result array >> descriptor to set it to zero extent, in the function templates for >> transformational functions doing an array reduction along a >> dimension.  This >> overwrite, which happened before early returning in case the result array >> was empty, was wrong because an array may have a non-zero extent in the >> first dimension and still be empty if it has a zero extent in a higher >> dimension.  Overwriting the dimension was resulting in wrong array result >> upper bound for the first dimension in that case. >> >> The offending piece of code was present in several places, and this >> removes >> them all.  More precisely, there is only one case to fix for logical >> reduction functions, and there are three cases for other reduction >> functions, corresponding to non-masked reduction, reduction with array >> mask, >> and reduction with scalar mask.  The impacted m4 files are >> ifunction_logical.m4 for logical reduction functions, ifunction.m4 for >> regular functions and types, ifunction-s.m4 for character minloc and >> maxloc, >> ifunction-s2.m4 for character minval and maxval, and ifindloc1.m4 for >> findloc. > > while your fix seems mechanical and correct, I wonder if you looked > at the following pre-existing irregularity which can be seen in > this snippet: > >> diff --git a/libgfortran/m4/ifunction.m4 b/libgfortran/m4/ifunction.m4 >> index 480649cf691..abc15b430ab 100644 >> --- a/libgfortran/m4/ifunction.m4 >> +++ b/libgfortran/m4/ifunction.m4 >> @@ -96,12 +96,7 @@ name`'rtype_qual`_'atype_code` ('rtype` * const >> restrict retarray, >> >>         retarray->base_addr = xmallocarray (alloc_size, sizeof >> (rtype_name)); >>         if (alloc_size == 0) >> -    { >> -      /* Make sure we have a zero-sized array.  */ >> -      GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1); >> -      return; >> - >> -    } >> +    return; >>       } >>     else >>       { > > This is all enclosed in a block which has >   if (retarray->base_addr == NULL) > but allocates and sets retarray->base_addr, while > >> @@ -290,11 +285,7 @@ m'name`'rtype_qual`_'atype_code` ('rtype` * const >> restrict retarray, >>         retarray->dtype.rank = rank; >> >>         if (alloc_size == 0) >> -    { >> -      /* Make sure we have a zero-sized array.  */ >> -      GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1); >> -      return; >> -    } >> +    return; >>         else >>       retarray->base_addr = xmallocarray (alloc_size, sizeof >> (rtype_name)); >> > > and > >> @@ -454,11 +445,7 @@ void >>         alloc_size = GFC_DESCRIPTOR_STRIDE(retarray,rank-1) * >> extent[rank-1]; >> >>         if (alloc_size == 0) >> -    { >> -      /* Make sure we have a zero-sized array.  */ >> -      GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1); >> -      return; >> -    } >> +    return; >>         else >>       retarray->base_addr = xmallocarray (alloc_size, sizeof >> (rtype_name)); >>       } > > do not set retarray->base_addr to non-NULL for alloc_size == 0. > > Do you know if the first snippet can be safely rewritten to avoid > the (hopefully pointless) xmallocarray for alloc_size == 0? > This change to the testcase: diff --git a/gcc/testsuite/gfortran.dg/bound_11.f90 b/gcc/testsuite/gfortran.dg/bound_11.f90 index 170eba4ddfd..2e96f843476 100644 --- a/gcc/testsuite/gfortran.dg/bound_11.f90 +++ b/gcc/testsuite/gfortran.dg/bound_11.f90 @@ -88,6 +88,7 @@ contains m4 = .false. i = 1 r = sum(a, dim=i) + if (.not. allocated(r)) stop 210 if (any(shape(r) /= (/ 3, 0, 7 /))) stop 211 if (any(ubound(r) /= (/ 3, 0, 7 /))) stop 212 i = 2 @@ -104,6 +105,7 @@ contains if (any(ubound(r) /= (/ 9, 3, 0 /))) stop 218 i = 1 r = sum(a, dim=i, mask=m1) + if (.not. allocated(r)) stop 220 if (any(shape(r) /= (/ 3, 0, 7 /))) stop 221 if (any(ubound(r) /= (/ 3, 0, 7 /))) stop 222 i = 2 @@ -120,6 +122,7 @@ contains if (any(ubound(r) /= (/ 9, 3, 0 /))) stop 228 i = 1 r = sum(a, dim=i, mask=m4) + if (.not. allocated(r)) stop 230 if (any(shape(r) /= (/ 3, 0, 7 /))) stop 231 if (any(ubound(r) /= (/ 3, 0, 7 /))) stop 232 i = 2 gives me a FAIL with STOP 220 (or STOP 230 if the STOP 220 line is commented); the first one with STOP 210 passes. So it is the first snippet with the xmallocarray (which supports zero values see memory.c) call that is the correct one. Good catch, I will open a separate PR. Mikael