From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id 8E2B63848428; Wed, 21 Jul 2021 18:01:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8E2B63848428 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com IronPort-SDR: sqtWM91RbKPcwZUWUWlFCXJq0bdM5tL7aXsDb69XWM6R82DZONG/EcEbdFOA4+AU7N260urK72 XuzW/ikPn06DnKAmeDBUm2wOf/ru3Z3PyTzEql9lr2YbMxU6TJ0Dg5qXK0Zvavm3y1OilEu2bW 0XHhmDg0kUzyE1nXvhfzPz1iCxqtkCkwNuFi7NllEuTIOnjpQ3fPmR+J9ginrLySyAhV49xqMA DOtRgZA2O1TyS8d7RFroVH5kZdeTHIOznFHVcTsqJJcmJh/EzVJgfJEuLj+pmZnGzRGWHe/uRX qySFxZ3jJSSFSgiqG+CTaxlJ X-IronPort-AV: E=Sophos;i="5.84,258,1620720000"; d="scan'208";a="63780996" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 21 Jul 2021 10:01:48 -0800 IronPort-SDR: 6RFHX4/wFuhF2O19n0+B0ykXVGNLjExVjVPlHLVb45XZJM/KzXsceRzar+jOPRj5u9KGrKoKCc 71AJwqBKNH0NdFby/LaNL/4hjU7Z45rT/ljiYq04pN22lB7xlYjrdq5zQalimzS1QFLL/+Eryq QWBGiulWyYoldhRTAbT7VlltY3eVxNfinIYao84EpEw106Ck6/qGOnmHciLo44XmeGOdN8p7kO 3jc6b7PZIbZv3GlH576OuEntpehKm5fvVUIARTIRmX8nFAtVCNp12p+qVD6nSSxQ7pNmgVXpsx JQM= Subject: Re: [PATCH, Fortran] [PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions To: Tobias Burnus , "fortran@gcc.gnu.org" , "gcc-patches@gcc.gnu.org" References: <8fc3b97c-b122-9c2c-4657-4f98cf82973e@codesourcery.com> From: Sandra Loosemore Message-ID: <1fd11074-9c32-37a6-a957-3fe3e329eb9e@codesourcery.com> Date: Wed, 21 Jul 2021 12:01:40 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-ClientProxiedBy: svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00, BODY_8BITS, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP 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: Wed, 21 Jul 2021 18:01:55 -0000 On 7/21/21 11:26 AM, Tobias Burnus wrote: > On 17.07.21 02:49, Sandra Loosemore wrote: > >> This patch is for PR101317, one of the bugs uncovered by the TS29113 >> testsuite.  Here I'd observed that CFI_establish, etc was not >> diagnosing some invalid-argument situations documented in the >> standard, although it was properly catching others.  After fixing >> those I discovered a couple small mistakes in the test cases and fixed >> those too. > > Some first comments – I think I have to read though the file > ISO_Fortran_binding.c itself and not only your patch. > >> --- a/libgfortran/runtime/ISO_Fortran_binding.c >> +++ b/libgfortran/runtime/ISO_Fortran_binding.c >> @@ -232,7 +232,16 @@ CFI_allocate (CFI_cdesc_t *dv, const CFI_index_t >> lower_bounds[], >>     /* If the type is a Fortran character type, the descriptor's element >>        length is replaced by the elem_len argument. */ >>     if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char) >> -    dv->elem_len = elem_len; >> +    { >> +      if (unlikely (compile_options.bounds_check) && elem_len == 0) >> +    { >> +      fprintf ("CFI_allocate: The supplied elem_len must be " >> +           "greater than zero (elem_len = %d).\n", >> +           (int) elem_len); > > I think there is no need to use '(elem_len = %d)' given that it is > always zero as stated in the error message itself. Yeah, I could fix this. I'd initially forgotten that elem_len was an unsigned type and was trying to test it by passing a negative value. :-P > > (Appears twice) > > However, the check itself is also wrong – cf. below. Hmmm. CFI_establish explicitly says that the elem_len has to be greater than zero. It seems somewhat confusing that it's inconsistent with the other functions that take an elem_len argument. > Talking about CFI_allocatable, there is also another bug in that function, > untouched by your patch: > >  /* If the type is a character, the descriptor's element length is > replaced >      by the elem_len argument. */ >   if (dv->type == CFI_type_char || dv->type == CFI_type_ucs4_char || >       dv->type == CFI_type_signed_char) >     dv->elem_len = elem_len; > > The bug is that CFI_type_signed_char is not a character type. Ha! I noticed the same thing and already posted a separate patch for that. :-P https://gcc.gnu.org/pipermail/fortran/2021-July/056243.html >> +  else if (unlikely (compile_options.bounds_check) >> +       && type < 0) > Pointless line break. >> +          fprintf (stderr, "CFI_establish: Extents must be nonnegative " >> +               "(extents[%d] = %d).\n", i, (int)extents[i]); >> +          return CFI_INVALID_EXTENT; >> +        } > > How about PRIiPTR + ptrdiff_t instead of %d + (int) cast? At least as > positive value, extent may exceed INT_MAX. Hmmm, there are similar problems in existing code in other functions in this file (e.g., CFI_section). >> +      if (source->attribute == CFI_attribute_other >> +          && source->rank > 0 >> +          && source->dim[source->rank - 1].extent == -1) >> +        { >> +          fprintf (stderr, "CFI_setpointer: The source is a " >> +               "nonallocatable nonpointer object that is an " >> +               "assumed-size array.\n"); > > I think you could just check for assumed rank – without > CFI_attribute_other in the 'if' and 'nonallocatable nonpointer' in the > error message. Only nonallocatable nonpointer variables can be of > assumed size (in Fortran); I think that makes the message simpler > (focusing on the issue) and if the C user passes an allocatable/pointer, > which is assumed rank, it is also a bug. The wording of the message reflects the language of the standard: "source shall be a null pointer or the address of a C descriptor for an allocated allocatable object, a data pointer object, or a nonallocatable nonpointer data object that is not an assumed-size array. -Sandra