From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 86F7B384F039; Tue, 13 Jul 2021 04:50:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86F7B384F039 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: mWUzTas5d5ofG7az0R1NtF9idnxG0fIBmMnzCv6rGT/DuNlXNddtQ9lHh8qdbSTvdQMK6rk/fd cgQr8c59trZM6GxVb8ZNsYWXruUOzCF3JMGir4hD+C4gtl/8RjgUCEu+cFkEq/+d6mXl9pL7AA ekZahT0mk3ROaaYM8ubNxbZE4C+CCUl4wiC4w4drPOiR1tUVvbY/VpZND7p03ld+IV53MSm0RB MF2+Ofy/M7nJdFmgwZ89hmg50o9B9ALos0TkcACpLOl/l98TKnOpwraQ2FJDg9acvRsMQx60dP XhI= X-IronPort-AV: E=Sophos;i="5.84,235,1620720000"; d="scan'208";a="65885522" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 12 Jul 2021 20:50:35 -0800 IronPort-SDR: ref2RtA1/yZFLjIMOFznM3IfPPxpAeARYqupYs0LbJV5B2FbLiWz80pRBcoEAi5kNGDsV9qUvG OJa+fnF+p5/8ByVvIeZOkpGzB8W32QwPghcV0Wr0NspX4ZCg4zBQWwkYxN/SCHPGlZoEM3Mp2e 0FGZ7r4RbHKpltVNh/j9s0wvFby3ZAar2/Qpie//Wvjta65DE4hZGwKQrTeq8rZgMlwsxQWpEN vkmIvy0esqLwsE084AqLMXvPFFXovioY9Ap2UQL+4MAE/zkMPY9UdjnoJsvwSo3Okj2f7dmDq1 HJY= Subject: Re: [Patch, fortran] PR fortran/100906/100907/100911/100914/100915/100916 To: =?UTF-8?Q?Jos=c3=a9_Rui_Faustino_de_Sousa?= , , Newsgroups: gmane.comp.gcc.patches,gmane.comp.gcc.fortran References: <0653caee-14bd-6ac9-76c4-c3b09413cfa5@gmail.com> From: Sandra Loosemore Message-ID: <99230983-8796-51b5-7444-6e036388ef51@codesourcery.com> Date: Mon, 12 Jul 2021 22:50:27 -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: <0653caee-14bd-6ac9-76c4-c3b09413cfa5@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-ClientProxiedBy: SVR-ORW-MBX-05.mgc.mentorg.com (147.34.90.205) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_NUMSUBJECT, NICE_REPLY_A, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP 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: fortran@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Fortran mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2021 04:50:38 -0000 On 6/13/21 12:36 PM, José Rui Faustino de Sousa via Gcc-patches wrote: > Hi All! > > Proposed patch to: > > Bug 100906 - Bind(c): failure handling character with len/=1 > Bug 100907 - Bind(c): failure handling wide character > Bug 100911 - Bind(c): failure handling C_PTR > Bug 100914 - Bind(c): errors handling complex > Bug 100915 - Bind(c): failure handling C_FUNPTR > Bug 100916 - Bind(c): CFI_type_other unimplemented I've been playing a bit with this patch in conjunction with my TS29113 testsuite. It seems to help some things, but it causes a regression in library/section-1.f90 that I don't really understand. :-S I'm also still running up against the long double bug in PR100917; after doing some experiments on top of this patch, I suggested some possible solutions/workarounds in that issue. > Patch tested only on x86_64-pc-linux-gnu. In general, I think it is a good idea to test interoperability features on a 32-bit host as well. I've been building i686-pc-linux-gnu and testing both -m32 and -m64 multilibs. ISO_Fortran_binding.h is pretty broken on 32-bit host but I'll have a patch for that in the next day or so. Anyway, I have a few comments on the libgfortran changes, mostly cosmetic things. I'm not at all familiar with the workings of the Fortran front end code that produces descriptors, so I don't think I have anything useful to say about that part of the patch. > diff --git a/libgfortran/runtime/ISO_Fortran_binding.c b/libgfortran/runtime/ISO_Fortran_binding.c > index 20833ad..3a269d7 100644 > --- a/libgfortran/runtime/ISO_Fortran_binding.c > +++ b/libgfortran/runtime/ISO_Fortran_binding.c > @@ -36,31 +36,81 @@ export_proto(cfi_desc_to_gfc_desc); > void > cfi_desc_to_gfc_desc (gfc_array_void *d, CFI_cdesc_t **s_ptr) > { > + signed char type, kind; > + size_t size; > int n; > - index_type kind; > CFI_cdesc_t *s = *s_ptr; > > if (!s) > return; > > + /* Verify descriptor. */ > + switch(s->attribute) GNU coding standards: space before the open paren. > + { > + case CFI_attribute_pointer: > + case CFI_attribute_allocatable: > + break; > + case CFI_attribute_other: > + if (s->base_addr) > + break; > + /* FALL THROUGH */ > + default: > + internal_error (NULL, "INVALID CFI DESCRIPTOR"); We could have a better message here that doesn't SHOUT. Maybe "Invalid attribute in CFI descriptor"? > > + kind = _CFI_DECODE_KIND (s->type); > + switch(type) Space before the paren again. > + { > + case BT_INTEGER: > + case BT_LOGICAL: > + case BT_REAL: > + size = (size_t)kind; > + break; > + case BT_COMPLEX: > + size = (size_t)(kind << 1); > + break; > + case BT_DERIVED: > + case BT_CHARACTER: > + case BT_VOID: > + size = s->elem_len; > + break; > + default: > + if (type != CFI_type_other) > + internal_error(NULL, "TYPE ERROR"); Space before the paren and better error message again. Section 18.5.4 of the Fortran 2018 standard says: "If a C type is not interoperable with a Fortran type and kind supported by the Fortran processor, its macro shall evaluate to a negative value." And in fact I'll have a patch for PR101305 soon that will use a negative value distinct from CFI_type_other for that. If you construct a descriptor for such an object in C and try passing it to Fortran, I think that's a user error, not an internal error. > + if (size <= 0) > + internal_error(NULL, "SIZE ERROR"); Same issues here with fixing formatting and better message. > + GFC_DESCRIPTOR_SIZE (d) = size; > + > d->dtype.version = s->version; > + > + if ((s->rank < 0) || (s->rank > CFI_MAX_RANK)) You could delete the inner parentheses here. > + internal_error(NULL, "Rank out of range."); Space before the paren again. It also looks like other error messages in libgfortran don't add punctuation at the end. There are several more instances of space-before-paren problems and fixing the errpr messages, I'm not going to point the rest of them out individually. > @@ -74,14 +124,19 @@ cfi_desc_to_gfc_desc (gfc_array_void *d, CFI_cdesc_t **s_ptr) > } > > d->offset = 0; > - for (n = 0; n < GFC_DESCRIPTOR_RANK (d); n++) > - { > - GFC_DESCRIPTOR_LBOUND(d, n) = (index_type)s->dim[n].lower_bound; > - GFC_DESCRIPTOR_UBOUND(d, n) = (index_type)(s->dim[n].extent > - + s->dim[n].lower_bound - 1); > - GFC_DESCRIPTOR_STRIDE(d, n) = (index_type)(s->dim[n].sm / s->elem_len); > - d->offset -= GFC_DESCRIPTOR_STRIDE(d, n) * GFC_DESCRIPTOR_LBOUND(d, n); > - } > + if (GFC_DESCRIPTOR_DATA (d)) > + for (n = 0; n < GFC_DESCRIPTOR_RANK (d); n++) > + { > + CFI_index_t lb = 1; > + > + if (s->attribute != CFI_attribute_other) > + lb = s->dim[n].lower_bound; > + > + GFC_DESCRIPTOR_LBOUND(d, n) = (index_type)lb; > + GFC_DESCRIPTOR_UBOUND(d, n) = (index_type)(s->dim[n].extent + lb - 1); > + GFC_DESCRIPTOR_STRIDE(d, n) = (index_type)(s->dim[n].sm / s->elem_len); > + d->offset -= GFC_DESCRIPTOR_STRIDE(d, n) * GFC_DESCRIPTOR_LBOUND(d, n); > + } > } You might as well fix the space-before-paren issues in the code block you reindented as well. > @@ -92,32 +147,87 @@ gfc_desc_to_cfi_desc (CFI_cdesc_t **d_ptr, const gfc_array_void *s) > { > int n; > CFI_cdesc_t *d; > + signed char type, kind; > > /* Play it safe with allocation of the flexible array member 'dim' > by setting the length to CFI_MAX_RANK. This should not be necessary > but valgrind complains accesses after the allocated block. */ > if (*d_ptr == NULL) > - d = malloc (sizeof (CFI_cdesc_t) > + d = calloc (1, sizeof (CFI_cdesc_t) > + (CFI_type_t)(CFI_MAX_RANK * sizeof (CFI_dim_t))); This change screwed up the indentation of that last line; the "+" should line up with "sizeof". IIRC the GNU coding standards also recommend parenthesizing that binary expression so Emacs will indent it properly automatically. > + > d->rank = (CFI_rank_t)GFC_DESCRIPTOR_RANK (s); > + if ((d->rank < 0) || (d->rank > CFI_MAX_RANK)) > + internal_error(NULL, "Rank out of range."); Another too-many-parentheses instance. > + switch (d->type) > + { > + case CFI_type_Integer: > + case CFI_type_Logical: > + case CFI_type_Real: > + kind = (signed char)d->elem_len; > + break; > + case CFI_type_Complex: > + kind = (signed char)(d->elem_len >> 1); > + break; > + case CFI_type_Character: > + kind = 1; > + break; > + case CFI_type_struct: > + case CFI_type_cptr: > + case CFI_type_other: > + kind = 0; > + break; > + default: > + internal_error(NULL, "TYPE ERROR"); > + } case CFI_type_cfunptr: ??? -Sandra