From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) by sourceware.org (Postfix) with ESMTPS id 648DF3858C74; Sun, 6 Feb 2022 21:04:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 648DF3858C74 X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from [192.168.178.29] ([79.251.6.124]) by mail.gmx.net (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MDysg-1nQg451CGS-009uV3; Sun, 06 Feb 2022 22:04:33 +0100 Message-ID: <24718b24-981a-730c-4cd3-b6f4727797a0@gmx.de> Date: Sun, 6 Feb 2022 22:04:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] PR/101135 - Load of null pointer when passing absent assumed-shape array argument for an optional dummy argument Content-Language: en-US To: Mikael Morin , fortran , gcc-patches Newsgroups: gmane.comp.gcc.patches,gmane.comp.gcc.fortran References: <3fd50892-dbef-d43a-8efe-148a8ffa94a9@orange.fr> From: Harald Anlauf In-Reply-To: <3fd50892-dbef-d43a-8efe-148a8ffa94a9@orange.fr> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:zQlabyUBynh5YMT+o/ZTKW2rlJWGVd+iLltA54UVAA+phVJzzak nEv0hBgswpdT77fOpChS0VQmiYLb39KU+ClURr3vLl2AfhTTk4Lf2mei85d04j1RvULylLY W5xS1PajVE1AWHmASkF8Sm0l77vLMIvQES39criPFggjjZQi3FywoSv4k1+u7KBTZ9j/SV5 yUnvj+2IafZakz1Ir8O5w== X-UI-Out-Filterresults: notjunk:1;V03:K0:CoDNJXlKby0=:RntYes0Eecb/j+lQ+Lv9ng 2qSYyQRDPgFlFUDw3A0ukIMznCye4oy49f9NOgIAM4gY3FhxiTQ43DEoS4KorscvHt0iJTQmq NmqyxCAbTvm7c7OPInDoF6TG76+gDqKZ3GTJRmcpxPlOBOo4c3mlxd3pP+bhfAUelHysiNicV RHlEDyNIuGqbdVMnvI3GfuyoQgj0JC6kutQUul1oV5gpvRSsB3VUG9dpP8T+i5v2VIBiw/SjX QkIA7pbZ+2Z1X1UmDT5ZWBcb8WXwtHPIQ0ySDFemOjPU1L3rsIXGM0YeK6Pa4Wgj0nv5tyOIT ZGJk0T9Xnc+RWG+cuG3U/r3mGB499tVwP3C77Txi/s9l0MqnYe/llzCL3uzioxXWDblmonCQ6 L10BsxH/BSrAcnPPnklnPFSsTxg7ovKquBmn1A0geYQHXoGW5rUkr29QDZmFLndQQYju4b+6A miCXc65vEoB7CpoQaZbpCth5IvPCb4y8eCI2kWS7w0MFDXmeF0V1WApzKCcVTo9J3+4yHGxv2 9PDFiN9S3KmO9ukKxfn+Ou9I/iHqtvqXWO1INnF1C0LL4g2pBI1ucqNDOPSFRENyapewrIJO8 FREvHAnGiT72Qh3Zlg7iKtkAZBGXhYt7bg0GDZzpB6Uu09n1gE2MLiFhOAK6dOWRV0yBDFHox m8az/T9+VEIOTi/Pe6y3ZNszGrZJXgN9TKKmCIIjhjh9GrEknzPKMBUtnDuO6Pw/dNiHyeKxX 18IBZwDEu0Iypt4tzsUrRDZaV+qkVObASJeAAl7nuIPsxbbT42IJ0F7Ic7Kqsrf5W9ZnSNGEL 4GvAgq6aMmBacQtS2zSXS1oGkXLpY1JOc1iVo0AzOnXu3DCmm8oNB/sBW1hV88vRDks6e7oaJ beII3HxNlRWuH6KGxmUHOrumLfOGBbvjHX6HPuWc+il6gaB3WD0lf32Wa5pP0AzqLnInk0PM4 TqsgC/+bOzH8MBB7rMAPvw7jm3zK/2oCHK8+9oPAyC1JUlO75k9BqyEyTh4VTFKxwZmYA0K94 GQiscCiLzj0T+bzK9+G5WxYMDeAHcH0pkb6g4MGb0q+pVFYm2sqadnrOh5liETVcUHXGtLxy8 e65HCMo7NO0Pj4= X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Sun, 06 Feb 2022 21:04:37 -0000 Hi Mikael, Am 04.02.22 um 11:45 schrieb Mikael Morin: > Hello, > > Le 29/01/2022 =C3=A0 22:41, Harald Anlauf via Fortran a =C3=A9crit=C2=A0= : >> The least invasive change - already pointed out by the reporter - is >> to check the presence of the argument before dereferencing the data >> pointer after the offset calculation.=C2=A0 This requires adjusting the >> checking pattern for gfortran.dg/missing_optional_dummy_6a.f90. >> >> Regtesting reminded me that procedures with bind(c) attribute are doing >> their own stuff, which is why they need to be excluded here, otherwise >> testcase bind-c-contiguous-4.f90 would regress on the expected output. only after submitting the patch I figured that the patch is incomplete. When we have a call chain of procedures with and without bind(c), there are still cases left where the failure with the sanitizer is not fixed. Just add "bind(c)" to subroutine test_wrapper only in the original PR. I have added a corresponding comment in the PR. >> There is a potential alternative solution which I did not pursue, as I >> think it is more invasive, but also that I didn't succeed to implement: >> A non-present dummy array argument should not need to get its descripto= r >> set up.=C2=A0 Pursuing this is probably not the right thing to do durin= g the >> current stage of development and could be implemented later.=C2=A0 If s= omebody >> believes this is important, feel free to open a PR for this. >> > I have an other (equally unimportant) concern that it may create an > unnecessary conditional when passing a subobject of an optional > argument.=C2=A0 In that case we can assume that the optional is present. > It=E2=80=99s not a correctness issue, so let=E2=80=99s not bother at thi= s stage. Judging from the dump tree of the cases I looked at I did not see anything that would pose a problem to the optimizer. >> Regtested on x86_64-pc-linux-gnu.=C2=A0 OK for mainline? >> > OK. Given my latest observations I'd rather withdraw the current version of the patch and rethink. I also did not see an issue with bind(c) procedures calling alikes. It would help if one would not only know the properties of the actual argument, but also of the formal one, which is not available at that point in the code. I'll have another look and resubmit. > Thanks. > Thanks for the review! Harald From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ciao.gmane.io (ciao.gmane.io [116.202.254.214]) by sourceware.org (Postfix) with ESMTPS id EC13E3858C74 for ; Sun, 6 Feb 2022 21:04:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EC13E3858C74 Received: from list by ciao.gmane.io with local (Exim 4.92) (envelope-from ) id 1nGoi4-0007bf-Lm for gcc-patches@gcc.gnu.org; Sun, 06 Feb 2022 22:04:36 +0100 X-Injected-Via-Gmane: http://gmane.org/ To: gcc-patches@gcc.gnu.org From: Harald Anlauf Subject: Re: [PATCH] PR/101135 - Load of null pointer when passing absent assumed-shape array argument for an optional dummy argument Date: Sun, 6 Feb 2022 22:04:31 +0100 Message-ID: <24718b24-981a-730c-4cd3-b6f4727797a0@gmx.de> References: <3fd50892-dbef-d43a-8efe-148a8ffa94a9@orange.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: en-US In-Reply-To: <3fd50892-dbef-d43a-8efe-148a8ffa94a9@orange.fr> Cc: fortran@gcc.gnu.org X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: Sun, 06 Feb 2022 21:04:39 -0000 Message-ID: <20220206210431.8DIwJbGpUH_5g49yroxJD7ATtrCwwpoHZkcm6E3nTL8@z> Hi Mikael, Am 04.02.22 um 11:45 schrieb Mikael Morin: > Hello, > > Le 29/01/2022 à 22:41, Harald Anlauf via Fortran a écrit : >> The least invasive change - already pointed out by the reporter - is >> to check the presence of the argument before dereferencing the data >> pointer after the offset calculation.  This requires adjusting the >> checking pattern for gfortran.dg/missing_optional_dummy_6a.f90. >> >> Regtesting reminded me that procedures with bind(c) attribute are doing >> their own stuff, which is why they need to be excluded here, otherwise >> testcase bind-c-contiguous-4.f90 would regress on the expected output. only after submitting the patch I figured that the patch is incomplete. When we have a call chain of procedures with and without bind(c), there are still cases left where the failure with the sanitizer is not fixed. Just add "bind(c)" to subroutine test_wrapper only in the original PR. I have added a corresponding comment in the PR. >> There is a potential alternative solution which I did not pursue, as I >> think it is more invasive, but also that I didn't succeed to implement: >> A non-present dummy array argument should not need to get its descriptor >> set up.  Pursuing this is probably not the right thing to do during the >> current stage of development and could be implemented later.  If somebody >> believes this is important, feel free to open a PR for this. >> > I have an other (equally unimportant) concern that it may create an > unnecessary conditional when passing a subobject of an optional > argument.  In that case we can assume that the optional is present. > It’s not a correctness issue, so let’s not bother at this stage. Judging from the dump tree of the cases I looked at I did not see anything that would pose a problem to the optimizer. >> Regtested on x86_64-pc-linux-gnu.  OK for mainline? >> > OK. Given my latest observations I'd rather withdraw the current version of the patch and rethink. I also did not see an issue with bind(c) procedures calling alikes. It would help if one would not only know the properties of the actual argument, but also of the formal one, which is not available at that point in the code. I'll have another look and resubmit. > Thanks. > Thanks for the review! Harald