From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id 41B173858C2B; Tue, 24 Oct 2023 16:13:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 41B173858C2B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 41B173858C2B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=68.232.137.252 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698163985; cv=none; b=oW/foCouQKAe+jcSY64G60fsOXNVrB+fr4P9x3lPb7kq1Sori8Uw/aanxCKbIOHVmjmH+rpSBNym1FdfKqeB7S0hojvI2vZsJXF5tSyfo/n03BSKuxAjPayv/tPbqV/Gxr6YYynO9a1NbwOdIongIAnDGXm1aVjBZv3T6OsPgQQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698163985; c=relaxed/simple; bh=QUULrcs0TcYKb9wCsIhdDCw3WgXIwSvCZAsS2Mi6uGA=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=wKfbBx8csw1omy2lcyzbXbS6qOPzXrDOB/SwJaJO034TVJ+/W45/18IMXJY77X2RVeq4ucKzRKPmqM5t8kKu34LthAX5DIUCCJ0LcATyS+qeIAUCBz5BoqzotzZ/rtzG6iXfpo2kevr8twyEwDN/mL/Zn9CtVZNlq+BvJkYw1IE= ARC-Authentication-Results: i=1; server2.sourceware.org X-CSE-ConnectionGUID: ilANKE6JSbiRAry2Ga08zw== X-CSE-MsgGUID: +9p48UlTQEOupNswlYwjhQ== X-IronPort-AV: E=Sophos;i="6.03,248,1694764800"; d="diff'?scan'208";a="20604769" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa4.mentor.iphmx.com with ESMTP; 24 Oct 2023 08:12:57 -0800 IronPort-SDR: vCyw/jTiRseqCsMTbAQ0rWhutMVvMXuklpeDsG4yIsbc6lLr0YWnLwW1KSBpfHLsNd1CrrjFGU 1wNafSPkRezaKUzG68QzUsh55htqQK9QVuiPRbrmh1q40jk4Wy9Jhf2xyOqv98G6Rl36pfrRbl yU5Pr0Hns4VQoddEiOEOmnjvc/bPDU1ca2cIakoxLdaXKW0iN81rEURSOKJZKalXCGupTHtt8Y g9OJ76RhOnY++24GSuIVLaUENn0s5sOjXJ/aYOpy4wsbUDibAUbOFINWFquBI5xoy12Vli3KQF 5tw= Content-Type: multipart/mixed; boundary="------------P601K0Eg8j6Kq7TOzN8P2dql" Message-ID: <78236b34-a5bd-4ccf-a197-94bee00b8a2b@codesourcery.com> Date: Tue, 24 Oct 2023 18:12:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and TYPE(c_ptr) Content-Language: en-US To: , gcc-patches , Paul-Antoine Arras References: <6fc6a877-2dc7-4551-b141-fd117c66ecfa@codesourcery.com> From: Tobias Burnus In-Reply-To: <6fc6a877-2dc7-4551-b141-fd117c66ecfa@codesourcery.com> X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP 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: --------------P601K0Eg8j6Kq7TOzN8P2dql Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable Hi PA, hello all, First, I hesitate to review/approve a patch I am involved in; Thus, I would= like if someone could have a second look. Regarding the patch itself: On 20.10.23 16:02, Paul-Antoine Arraswrote: > Hi all, > > The attached patch fixes a bug that causes valid OpenMP declare > variant directive > and functions to be rejected with the following error (see testcase): > > [...] > Error: variant =E2=80=98foo_variant=E2=80=99 and base =E2=80=98foo=E2=80= =99 at (1) have incompatible > types: Type mismatch in argument 'c_bv' (INTEGER(8)/TYPE(c_ptr)) > > The fix consists in special-casing this situation in gfc_compare_types(). > OK for mainline? ... > Subject: [PATCH] Fortran: Fix incompatible types between INTEGER(8) and > TYPE(c_ptr) > > In the context of an OpenMP declare variant directive, arguments of type = C_PTR > are sometimes recognised as C_PTR in the base function and as INTEGER(8) = in the > variant - or the other way around, depending on the parsing order. > This patch prevents such situation from turning into a compile error. > > 2023-10-20 Paul-Antoine Arras > Tobias Burnus > > gcc/fortran/ChangeLog: > > * interface.cc (gfc_compare_types): Return true in this situation. That's a bad description. It makes sense when reading the commit log but if= you only read gcc/fortran/ChangeLog, 'this situation' is a dangling reference. > gcc/fortran/ChangeLog.omp | 5 ++ > gcc/testsuite/ChangeLog.omp | 4 ++ On mainline, the ChangeLog not ChangeLog.omp is used. This changelog is aut= omatically filled by the data in the commit log. Thus, no need to include it in the pa= tch. (Besides: It keeps getting outdated by any other commit to that file.) As you have a commit, running the following, possibly with the commit hash = as argument (unless it is the last commit) will show that the nightly script w= ill use: ./contrib/gcc-changelog/git_check_commit.py -v -p It is additionally a good check whether you got the syntax right. (This is = run as pre-commit hook.) * * * Regarding the patch, I think it will work, but I wonder whether we can do better - esp. regarding c_ptr vs. c_funptr. I started by looking why the current code fails: > index e9843e9549c..8bd35fd6d22 100644 > --- a/gcc/fortran/interface.cc > +++ b/gcc/fortran/interface.cc > @@ -705,12 +705,17 @@ gfc_compare_types (gfc_typespec *ts1, gfc_typespec = *ts2) > - > - if (((ts1->type =3D=3D BT_INTEGER && ts2->type =3D=3D BT_DERIVED) > - || (ts1->type =3D=3D BT_DERIVED && ts2->type =3D=3D BT_INTEGER)) > - && ts1->u.derived && ts2->u.derived > - && ts1->u.derived =3D=3D ts2->u.derived) This does not work because the pointers to the derived type are different: (gdb) p *ts1 $10 =3D {type =3D BT_INTEGER, kind =3D 8, u =3D {derived =3D 0x30c66b0, cl = =3D 0x30c66b0, pad =3D 51144368}, interface =3D 0x0, is_c_interop =3D 1, is= _iso_c =3D 0, f90_type =3D BT_VOID, deferred =3D false, interop_kind =3D 0x0} (gdb) p *ts2 $11 =3D {type =3D BT_DERIVED, kind =3D 0, u =3D {derived =3D 0x30c2930, cl = =3D 0x30c2930, pad =3D 51128624}, interface =3D 0x0, is_c_interop =3D 0, is= _iso_c =3D 0, f90_type =3D BT_UNKNOWN, deferred =3D false, interop_kind =3D 0x0} The reason seems to be that they are freshly created in different namespaces. Consequently, attr.use_assoc =3D 1 and the namespace is different, i.e. (gdb) p ts1->u.derived->ns->proc_name->name $18 =3D 0x7ffff6ff4138 "foo" (gdb) p ts2->u.derived->ns->proc_name->name $19 =3D 0x7ffff6ffc260 "foo_variant" * * * Having said this, I think we can combine the current and the modified version, i.e. > + if ((ts1->type =3D=3D BT_INTEGER && ts2->type =3D=3D BT_DERIVED > + && ts1->f90_type =3D=3D BT_VOID > + && ts2->u.derived->ts.is_iso_c > + && ts2->u.derived->ts.u.derived->ts.f90_type =3D=3D BT_VOID) > + || (ts2->type =3D=3D BT_INTEGER && ts1->type =3D=3D BT_DERIVED > + && ts2->f90_type =3D=3D BT_VOID > + && ts1->u.derived->ts.is_iso_c > + && ts1->u.derived->ts.u.derived->ts.f90_type =3D=3D BT_VOID)) See attached patch for a combined version, which checks now whether from_intmod =3D=3D INTMOD_ISO_C_BINDING and then compares the names (to distinguish c_ptr and c_funptr). Those are unaffected by 'use' renames, hence, we should be fine. While in this example, the name pointers are identical, I fear that won't hold in some more complex indirect use via module-use. Thus, strcmp is used. (gdb) p ts1->u.derived->name $13 =3D 0x7ffff6ff4100 "c_ptr" (gdb) p ts2->u.derived->name $14 =3D 0x7ffff6ff4100 "c_ptr" * * * Additionally, I think it would be good to have a testcase which checks for c_funptr vs. c_ptr mismatch. Just changing c_ptr to c_funptr in the testcase (+ commenting the c_f_point= er) prints: Error: variant =E2=80=98foo_variant=E2=80=99 and base =E2=80=98foo=E2=80= =99 at (1) have incompatible types: Type mismatch in argument 'c_bv' (INTEG= ER(8)/TYPE(c_funptr)) I think that would be a good invalid testcase besides the valid one. But with a tweak to get better messages to give: Error: variant =E2=80=98foo_variant=E2=80=99 and base =E2=80=98foo=E2=80= =99 at (1) have incompatible types: Type mismatch in argument 'c_bv' (TYPE(= c_ptr)/TYPE(c_funptr)) cf. misc.cc in the attached proposal for the *.cc files, only. Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --------------P601K0Eg8j6Kq7TOzN8P2dql Content-Type: text/x-patch; charset="UTF-8"; name="proposed.diff" Content-Disposition: attachment; filename="proposed.diff" Content-Transfer-Encoding: base64 IGdjYy9mb3J0cmFuL2ludGVyZmFjZS5jYyB8IDE2ICsrKysrKysrKysrKy0tLS0KIGdjYy9m b3J0cmFuL21pc2MuY2MgICAgICB8ICA3ICsrKysrKy0KIDIgZmlsZXMgY2hhbmdlZCwgMTgg aW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9nY2MvZm9ydHJh bi9pbnRlcmZhY2UuY2MgYi9nY2MvZm9ydHJhbi9pbnRlcmZhY2UuY2MKaW5kZXggYzAxZGYw NDYwZDcuLjhjNDU3MWUwYWE2IDEwMDY0NAotLS0gYS9nY2MvZm9ydHJhbi9pbnRlcmZhY2Uu Y2MKKysrIGIvZ2NjL2ZvcnRyYW4vaW50ZXJmYWNlLmNjCkBAIC03MzYsMTAgKzczNiwxOCBA QCBnZmNfY29tcGFyZV90eXBlcyAoZ2ZjX3R5cGVzcGVjICp0czEsIGdmY190eXBlc3BlYyAq dHMyKQogICAgICBiZXR0ZXIgd2F5IG9mIGRvaW5nIHRoaXMuICBXaGVuIElTTyBDIGJpbmRp bmcgaXMgY2xlYXJlZCB1cCwKICAgICAgdGhpcyBjYW4gcHJvYmFibHkgYmUgcmVtb3ZlZC4g IFNlZSBQUiA1NzA0OC4gICovCiAKLSAgaWYgKCgodHMxLT50eXBlID09IEJUX0lOVEVHRVIg JiYgdHMyLT50eXBlID09IEJUX0RFUklWRUQpCi0gICAgICAgfHwgKHRzMS0+dHlwZSA9PSBC VF9ERVJJVkVEICYmIHRzMi0+dHlwZSA9PSBCVF9JTlRFR0VSKSkKLSAgICAgICYmIHRzMS0+ dS5kZXJpdmVkICYmIHRzMi0+dS5kZXJpdmVkCi0gICAgICAmJiB0czEtPnUuZGVyaXZlZCA9 PSB0czItPnUuZGVyaXZlZCkKKyAgaWYgKCh0czEtPnR5cGUgPT0gQlRfSU5URUdFUgorICAg ICAgICYmIHRzMi0+dHlwZSA9PSBCVF9ERVJJVkVECisgICAgICAgJiYgdHMxLT5mOTBfdHlw ZSA9PSBCVF9WT0lECisgICAgICAgJiYgdHMyLT51LmRlcml2ZWQtPmZyb21faW50bW9kID09 IElOVE1PRF9JU09fQ19CSU5ESU5HCisgICAgICAgJiYgdHMxLT51LmRlcml2ZWQKKyAgICAg ICAmJiBzdHJjbXAgKHRzMS0+dS5kZXJpdmVkLT5uYW1lLCB0czItPnUuZGVyaXZlZC0+bmFt ZSkgPT0gMCkKKyAgICAgIHx8ICh0czItPnR5cGUgPT0gQlRfSU5URUdFUgorCSAgJiYgdHMx LT50eXBlID09IEJUX0RFUklWRUQKKwkgICYmIHRzMi0+ZjkwX3R5cGUgPT0gQlRfVk9JRAor CSAgJiYgdHMxLT51LmRlcml2ZWQtPmZyb21faW50bW9kID09IElOVE1PRF9JU09fQ19CSU5E SU5HCisJICAmJiB0czItPnUuZGVyaXZlZAorCSAgJiYgc3RyY21wICh0czEtPnUuZGVyaXZl ZC0+bmFtZSwgdHMyLT51LmRlcml2ZWQtPm5hbWUpID09IDApKQogICAgIHJldHVybiB0cnVl OwogCiAgIC8qIFRoZSBfZGF0YSBjb21wb25lbnQgaXMgbm90IGFsd2F5cyBwcmVzZW50LCB0 aGVyZWZvcmUgY2hlY2sgZm9yIGl0cwpkaWZmIC0tZ2l0IGEvZ2NjL2ZvcnRyYW4vbWlzYy5j YyBiL2djYy9mb3J0cmFuL21pc2MuY2MKaW5kZXggYmFlNmQyOTJkYzUuLmVkZmZiYTA3MDEz IDEwMDY0NAotLS0gYS9nY2MvZm9ydHJhbi9taXNjLmNjCisrKyBiL2djYy9mb3J0cmFuL21p c2MuY2MKQEAgLTEzOCw3ICsxMzgsMTIgQEAgZ2ZjX3R5cGVuYW1lIChnZmNfdHlwZXNwZWMg KnRzLCBib29sIGZvcl9oYXNoKQogICBzd2l0Y2ggKHRzLT50eXBlKQogICAgIHsKICAgICBj YXNlIEJUX0lOVEVHRVI6Ci0gICAgICBzcHJpbnRmIChidWZmZXIsICJJTlRFR0VSKCVkKSIs IHRzLT5raW5kKTsKKyAgICAgIGlmICh0cy0+ZjkwX3R5cGUgPT0gQlRfVk9JRAorCSAgJiYg dHMtPnUuZGVyaXZlZAorCSAgJiYgdHMtPnUuZGVyaXZlZC0+ZnJvbV9pbnRtb2QgPT0gSU5U TU9EX0lTT19DX0JJTkRJTkcpCisJc3ByaW50ZiAoYnVmZmVyLCAiVFlQRSglcykiLCB0cy0+ dS5kZXJpdmVkLT5uYW1lKTsKKyAgICAgIGVsc2UKKwlzcHJpbnRmIChidWZmZXIsICJJTlRF R0VSKCVkKSIsIHRzLT5raW5kKTsKICAgICAgIGJyZWFrOwogICAgIGNhc2UgQlRfUkVBTDoK ICAgICAgIHNwcmludGYgKGJ1ZmZlciwgIlJFQUwoJWQpIiwgdHMtPmtpbmQpOwo= --------------P601K0Eg8j6Kq7TOzN8P2dql--