Re: [PATCH v2, Fortran] [PR libfortran/101317] Bind(c): Improve error checking in CFI_* functions
Hi Sandra, On 25.07.21 06:11, Sandra Loosemore wrote: Congratulation – we have found a bug in the spec, which is also present in the current draft (21-007). I have now written to J3: https://mailman.j3-fortran.org/pipermail/j3/2021-July/013189.html That discussion seems to have wandered off into some other direction so I'm not sure whether it really clarifies this problem. I concur. I do hope that it will be at some point discussed and clarified. But for now: For the purposes of this patch I have left in the test for elem_len > 0 in CFI_establish where the standard explicitly has that requirement and removed it from the other functions where I'd added it just to be consistent. I think that makes sense. OK, I have done that throughout the file, and also made the wording change you asked for. While I was at it, I went through all the diagnostic messages in the file and simplified the wording of a few other messages as well, fixed typos and inconsistent capitalization and missing punctuation and things like that. Thanks! Here's a new patch. LGTM. Thanks, Tobias - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Re: Add f2008 to description
Hi Vivek, On 24.07.21 16:19, Vivek Rao via Fortran wrote: The gfortran page https://gcc.gnu.org/wiki/GFortran says, "Gfortran is the name of the GNU Fortran project, developing a free Fortran 95/2003/2008 compiler for GCC, the GNU Compiler Collection." Since Fortran 2018 features are being added, I suggest that 95/2003/2008 be replaced by 95/2003/2008/2018. Done now. While still a lot of 2018 is missing, the development surely includes 2018 features. Thanks, Tobias - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Re: [PATCH 3/3] [PR libfortran/101305] Fix ISO_Fortran_binding.h paths in gfortran testsuite
Hi Sandra,
On 23.07.21 22:43, Sandra Loosemore wrote:
On 7/23/21 8:15 AM, Tobias Burnus wrote:
On 21.07.21 12:17, Tobias Burnus wrote:
On 13.07.21 23:28, Sandra Loosemore wrote:
ISO_Fortran_binding.h is now generated in the libgfortran build
directory where it is on the default include path. Adjust includes
[...]
Unfortunately, that does not help.
It seems as if the following works in the *.exp file:
[...]
I'm not seeing the include path failures Tobias is seeing, [...] why
this isn't working for Tobias. :-S
I also do not have any idea – I did bootstrap before into an empty directory
and I don't think I had other patches applied.
I have no idea why it did not work – nor why it now works. I did now (again?):
* Reset all patches + re-apply your three patches
* Bootstrap into an empty directory with
$ /configure --prefix=... --enable-multiarch
--enable-languages=c,c++,fortran,lto,objc
$ make -j12 && make install
$ cd gcc
$ make check-fortran RUNTESTFLAGS="dg.exp=ISO_Fortran_binding_1.f90
--target_board=unix\{,-m32\}"
and now I got:
=== gfortran Summary for unix ===
# of expected passes12
=== gfortran Summary for unix/-m32 ===
# of expected passes12
Thus, it (mostly) works.
(I also did a more complete 'make check-fortran' run.)
* * *
I did say that it mostly works because of:
$ find x86_64-pc-linux-gnu/ -name ISO_Fortran_binding.h
x86_64-pc-linux-gnu/libgfortran/ISO_Fortran_binding.h
x86_64-pc-linux-gnu/32/libgfortran/ISO_Fortran_binding.h
And when looking at the -B lines, I see for the '' alias '-m64' run:
-B.../build/gcc/testsuite/gfortran/../../
-B.../build/x86_64-pc-linux-gnu/./libgfortran/
-B.../build/x86_64-pc-linux-gnu/./libgfortran/.libs
-B.../build/x86_64-pc-linux-gnu/./libquadmath/.libs
which is fine (second line ensures the ISO*.h file is found.)
But for -m32, I see:
-B.../build/gcc/testsuite/gfortran/../../
-B.../build/x86_64-pc-linux-gnu/./libgfortran/
-B.../build/x86_64-pc-linux-gnu/32/libgfortran/.libs
-B.../build/x86_64-pc-linux-gnu/32/libquadmath/.libs
That also works, but it uses again the same directory for ISO*.h,
such that the -m64 header file is used instead of the -m32 one.
* * *
I am not sure whether it really matters – the differences between
the header files is (on x86-64-gnu-linux):
-#define CFI_type_int128_t (CFI_type_Integer + (16 << CFI_type_kind_shift))
-#define CFI_type_int_least128_t (CFI_type_Integer + (16 <<
CFI_type_kind_shift))
-#define CFI_type_int_fast128_t (CFI_type_Integer + (16 << CFI_type_kind_shift))
+#define CFI_type_int128_t -2
+#define CFI_type_int_least128_t -2
+#define CFI_type_int_fast128_t -2
There might be larger differences on other multi-arch systems, but at least
for x86-64, it seems to be harmless. (-2 = not available).
For instance, there might be an issue on Windows. (I keep forgetting what
sizeof(long) is with -m64 – is is the same as sizeof(int) as with -m32?
Or is it 64bit with -m64?)
* * *
Thus, I am puzzled why it failed before and not longer. But given that it
works now:
LGTM, thanks for the patch and sorry for the confusion!
Tobias
PS: Still, it would be nice if the proper multi-lib ISO*.h could be found;
while it usually does not matter, it could do so in some cases.
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
Committed: Re: [Patch, fortran V2] PR fortran/93308/93963/94327/94331/97046 problems raised by descriptor handling
I have now committed José's patch with the two nits fixed (cf. my on-top patch to which I just replied) r12-2511-g0cbf03689e3e7d9d6002b8e5d159ef3716d0404c Note: I have slightly reworded the error message compared to both the original patch and to my on-top suggestion. Reason: When calling a BIND(C) function from Fortran, it might happen that a actual or effective argument is an allocatable or pointer that is no allocatated/associated (→ base_addr == NULL) but whose dtype.attribute is 'other' as the dummy argument is nonallocatable/nonpointer. Likewise, when passing a base_addr == NULL from C to a Fortran-written BIND(C) procedure where attribute == CFI_attribute_other. Those errors are much more likely than having some other bug. Thus, those get now an error on their own instead of a generic error, even though the reason can be the same as for: On the other hand, if the attribute != 0, 1, 2 it is invalid, which either is a bug in the compiler, random/uninitialized memory or a bug in the C code setting up the descriptor. Thus, the error message is now different. Comments to the new wording + comments/remarks to this commit (or any new or existing code) are welcome :-) Thanks, Tobias PS: I wrote: On 22.06.21 09:11, Tobias Burnus wrote: On 21.06.21 22:29, Tobias Burnus wrote: However, that's independent from the patch you had submitted and which is fine except for the two tiny nits. As I just did run into a test, which does trigger the error, I think it would be useful to have something like the following on top of your patch – what do you think? (Two of the changes are the nit changes I mentioned in the LGTM approval.) - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 commit 0cbf03689e3e7d9d6002b8e5d159ef3716d0404c Author: Tobias Burnus Date: Mon Jul 26 14:20:46 2021 +0200 PR fortran/93308/93963/94327/94331/97046 problems raised by descriptor handling Fortran: Fix attributes and bounds in ISO_Fortran_binding. 2021-07-26 José Rui Faustino de Sousa Tobias Burnus PR fortran/93308 PR fortran/93963 PR fortran/94327 PR fortran/94331 PR fortran/97046 gcc/fortran/ChangeLog: * trans-decl.c (convert_CFI_desc): Only copy out the descriptor if necessary. * trans-expr.c (gfc_conv_gfc_desc_to_cfi_desc): Updated attribute handling which reflect a previous intermediate version of the standard. Only copy out the descriptor if necessary. libgfortran/ChangeLog: * runtime/ISO_Fortran_binding.c (cfi_desc_to_gfc_desc): Add code to verify the descriptor. Correct bounds calculation. (gfc_desc_to_cfi_desc): Add code to verify the descriptor. gcc/testsuite/ChangeLog: * gfortran.dg/ISO_Fortran_binding_1.f90: Add pointer attribute, this test is still erroneous but now it compiles. * gfortran.dg/bind_c_array_params_2.f90: Update regex to match code changes. * gfortran.dg/PR93308.f90: New test. * gfortran.dg/PR93963.f90: New test. * gfortran.dg/PR94327.c: New test. * gfortran.dg/PR94327.f90: New test. * gfortran.dg/PR94331.c: New test. * gfortran.dg/PR94331.f90: New test. * gfortran.dg/PR97046.f90: New test. --- gcc/fortran/trans-decl.c | 32 +-- gcc/fortran/trans-expr.c | 24 +- .../gfortran.dg/ISO_Fortran_binding_1.f90 | 2 +- gcc/testsuite/gfortran.dg/PR93308.f90 | 52 + gcc/testsuite/gfortran.dg/PR93963.f90 | 150 gcc/testsuite/gfortran.dg/PR94327.c| 70 ++ gcc/testsuite/gfortran.dg/PR94327.f90 | 195 gcc/testsuite/gfortran.dg/PR94331.c| 73 ++ gcc/testsuite/gfortran.dg/PR94331.f90 | 252 + gcc/testsuite/gfortran.dg/PR97046.f90 | 58 + .../gfortran.dg/bind_c_array_params_2.f90 | 2 +- libgfortran/runtime/ISO_Fortran_binding.c | 56 - 12 files changed, 933 insertions(+), 33 deletions(-) diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index bf8783a35f8..784f7b61ce1 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -4539,22 +4539,28 @@ convert_CFI_desc (gfc_wrapped_block * block, gfc_symbol *sym) gfc_add_expr_to_block (&outer_block, incoming); incoming = gfc_finish_block (&outer_block); - /* Convert the gfc descriptor back to the CFI type before going out of scope, if the CFI type was present at entry. */ - gfc_i
Re: [PATCH 3/3] [PR libfortran/101305] Fix ISO_Fortran_binding.h paths in gfortran testsuite
On 7/26/21 3:45 AM, Tobias Burnus wrote:
[snip]
I did say that it mostly works because of:
$ find x86_64-pc-linux-gnu/ -name ISO_Fortran_binding.h
x86_64-pc-linux-gnu/libgfortran/ISO_Fortran_binding.h
x86_64-pc-linux-gnu/32/libgfortran/ISO_Fortran_binding.h
And when looking at the -B lines, I see for the '' alias '-m64' run:
-B.../build/gcc/testsuite/gfortran/../../
-B.../build/x86_64-pc-linux-gnu/./libgfortran/
-B.../build/x86_64-pc-linux-gnu/./libgfortran/.libs
-B.../build/x86_64-pc-linux-gnu/./libquadmath/.libs
which is fine (second line ensures the ISO*.h file is found.)
But for -m32, I see:
-B.../build/gcc/testsuite/gfortran/../../
-B.../build/x86_64-pc-linux-gnu/./libgfortran/
-B.../build/x86_64-pc-linux-gnu/32/libgfortran/.libs
-B.../build/x86_64-pc-linux-gnu/32/libquadmath/.libs
That also works, but it uses again the same directory for ISO*.h,
such that the -m64 header file is used instead of the -m32 one.
I did some more experiments and I see that too. :-S It's finding a .h
file, but not the right one. :-(
PS: Still, it would be nice if the proper multi-lib ISO*.h could be found;
while it usually does not matter, it could do so in some cases.
I think I ought to fix this now instead of just sweeping it under the
rug. The suggestion you made previously to add
# Flags for finding libgfortran ISO*.h files.
if [info exists TOOL_OPTIONS] {
set specpath [get_multilibs ${TOOL_OPTIONS}]
} else {
set specpath [get_multilibs]
}
set options "-I $specpath/libgfortran/"
to the .exp files looks consistent with what I see elsewhere for adding
things to the include path, so I will give it a try and see how it works.
-Sandra
Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169
Hi Tobias,
> > This works as expected with Intel and AOCC, but gives a
> > syntax error with every gfortran tested because of match.c:
> >
> > alloc_opt_list:
> >m = gfc_match (" stat = %v", &tmp);
>
> I think we can simply change that one to %e; the definable
> check should ensure that any non variable (in the Fortran sense)
> is rejected.
>
> And we should update the comment for %v / match_variable to state
> that it does not include function references.
I've updated this for ALLOCATE/DEALLOCATE and STAT/ERRMSG, see
attached patch. This required updating the error messages of
two existing files in the testsuite.
> Also affected: Some I/O items, a bunch of other stat=%v and
> errmsg=%v.
We should rather open a separate PR on auditing the related uses
of gfc_match.
> Talking about errmsg: In the same function, the same check is
> done for errmsg as for stat – hence, the patch should update
> also errmsg.
Done.
> >> Additionally, I have to admit that I do not understand the
> >> following existing condition, which you did not touch:
> >>
> >> if ((stat->ts.type != BT_INTEGER
> >> && !(stat->ref && (stat->ref->type == REF_ARRAY
> >> || stat->ref->type == REF_COMPONENT)))
> >> || stat->rank > 0)
> >> gfc_error ("Stat-variable at %L must be a scalar INTEGER "
> >> "variable", &stat->where);
> >>
> >> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
> >> but what's the reason for the refs?
> > Well, that needs to be answered by Steve (see commit 3759634).
>
> (https://gcc.gnu.org/g:3759634f3208cbc1226bec19d22cbff989a287c3 (svn
> r145331))
>
> The reason for the ref checks is unclear and seem to be wrong. The added
> testcases also only use 'x' (real) and n or i (integer) as input, i.e.
> they do not exercise this. I did not look for the patch email for reasoning.
Well, there is some text in the standard that I added in front of
the for loops, and this code is now exercised in the new testcase.
Regtested on x86_64-pc-linux-gnu. OK?
Thanks,
Harald
Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
gcc/fortran/ChangeLog:
PR fortran/101564
* match.c (gfc_match): Fix comment for %v code.
(gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
by %e in gfc_match to allow for function references as STAT and
ERRMSG arguments.
* resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
dereferences and shortcut for bad STAT and ERRMSG argument to
(DE)ALLOCATE.
gcc/testsuite/ChangeLog:
PR fortran/101564
* gfortran.dg/allocate_stat_3.f90: New test.
* gfortran.dg/allocate_stat.f90: Adjust error messages.
* gfortran.dg/implicit_11.f90: Adjust error messages.
diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
index d148de3e3b5..b1105481099 100644
--- a/gcc/fortran/match.c
+++ b/gcc/fortran/match.c
@@ -1109,7 +1109,8 @@ gfc_match_char (char c)
%t Matches end of statement.
%o Matches an intrinsic operator, returned as an INTRINSIC enum.
%l Matches a statement label
- %v Matches a variable expression (an lvalue)
+ %v Matches a variable expression (an lvalue, except function references
+ having a data pointer result)
% Matches a required space (in free form) and optional spaces. */
match
@@ -4405,7 +4406,7 @@ gfc_match_allocate (void)
alloc_opt_list:
- m = gfc_match (" stat = %v", &tmp);
+ m = gfc_match (" stat = %e", &tmp);
if (m == MATCH_ERROR)
goto cleanup;
if (m == MATCH_YES)
@@ -4434,7 +4435,7 @@ alloc_opt_list:
goto alloc_opt_list;
}
- m = gfc_match (" errmsg = %v", &tmp);
+ m = gfc_match (" errmsg = %e", &tmp);
if (m == MATCH_ERROR)
goto cleanup;
if (m == MATCH_YES)
@@ -4777,7 +4778,7 @@ gfc_match_deallocate (void)
dealloc_opt_list:
- m = gfc_match (" stat = %v", &tmp);
+ m = gfc_match (" stat = %e", &tmp);
if (m == MATCH_ERROR)
goto cleanup;
if (m == MATCH_YES)
@@ -4799,7 +4800,7 @@ dealloc_opt_list:
goto dealloc_opt_list;
}
- m = gfc_match (" errmsg = %v", &tmp);
+ m = gfc_match (" errmsg = %e", &tmp);
if (m == MATCH_ERROR)
goto cleanup;
if (m == MATCH_YES)
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 45c3ad387ac..809a4ad86d1 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -8165,6 +8165,12 @@ resolve_allocate_deallocate (gfc_code *code, const char *fcn)
gfc_error ("Stat-variable at %L must be a scalar INTEGER "
"variable", &stat->where);
+ if (stat->expr_type == EXPR_CONSTANT || stat->symtree == NULL)
+ goto done_stat;
+
+ /* F2018:9.7.4: The stat-variable shall not be allocated or deallocated
+ * within the ALLOCATE or DEALLOCATE statement in which it appears ...
+ */
for (p = code->ext.alloc.list; p; p = p->next)
if (
non_recursive vs recursive procedures
In the old days, before F2018, a programmer was required to explicitly declared a procedure as RECURSIVE if the programmer wanted a recursive behavior. Fast-forward to F2018, and J3 has changed the requirements and introduced a new NON_RECURSIVE prefix; and, by defaults procedures are considered recursive. The old RECURSIVE is still around for compatibility. gfortran does not support the NON_RECURSIVE prefix, so I created https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101632 And to everyone's delight, I have attached a patch to implement the new world order. Hopefully, someone can find the time to commit the patch, so that it does not fester in bugzilla like the other dozen or patches I've attached to other PRs. -- Steve
