Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169
Hi Harald,
On 26.07.21 23:55, Harald Anlauf wrote:
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.
Thanks.
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.
I concur – I just wanted to quickly check how many %v are there –
besides %v, there are also direct calls to gfc_match_variable.
Can you open a PR?
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.
The loops are clear – but the
'!stat->ref || (...ref->type != ARRAY || ref->type != COMPONENT))'
is still not clear to me.
* * *
Can you add the (working) test:
allocate (A, stat=y%stat%kind) ! { dg-error "cannot be a constant" }
deallocate (A, stat=y%stat%kind) ! { dg-error "cannot be a constant" }
to your testcase gcc/testsuite/gfortran.dg/allocate_stat_3.f90 ?
And also the following one, which does not get diagnosed and, hence,
later gives an ICE during gimplification.
type tc
character (len=:), allocatable :: str
end type tc
...
type(tc) :: z
...
allocate(character(len=13) :: z%str)
allocate (A, stat=z%str%len)
deallocate (A, stat=z%str%len)
To fix it, I think the solution is to do the following:
* In gfc_check_vardef_context, handle also REF_INQUIRY; in the
for (ref = e->ref; ref && check_intentin; ref = ref->next)
loop, I think there should be a
if (ref->type == REF_INQUIRY)
{
if (context)
gfc_error ("Type parameter inquiry for %qs in "
"variable definition context (%s) at %L",
name, context, &e->where);
return false;
}
(untested)
I assume (but have not tested it) that will give
two error messages for:
allocate (A, errmsg=z%str%len)
deallocate (A, errmsg=z%str%len)
one for the new type-param-inquiry check and one for
!= BT_CHARACTER
if you want to prevent the double error, consider to
replace
gfc_check_vardef_context (...);
by
if (!gfc_check_vardef_context (...))
goto done_errmsg;
Regtested on x86_64-pc-linux-gnu. OK?
LGTM - except for the two testcase additions proposed above
and fixing the ICE. If you are happy with my changes and they
work, feel free add them and commit without further review.
In either case, I have the feeling we are nearly there. :-)
Thanks for the patch and the review-modification-review-... patience!
Tobias
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.
-
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: [r12-2511 Regression] FAIL: gfortran.dg/PR93963.f90 -Os execution test on Linux/x86_64
The automatic regression test of Sunil wrote: On 26.07.21 19:27, sunil.k.pandey wrote: commit 0cbf03689e3e7d9d6002b8e5d159ef3716d0404c PR fortran/93308/93963/94327/94331/97046 problems raised by descriptor handling caused FAIL: gfortran.dg/PR93963.f90 -O2 execution test ... (That's on x86-64-gnu-linux but (only) with -m32.) I have filled: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101635 Let's see how soon we can fix this – otherwise, we have to XFAIL this testcase. I don't completely understand why this only occurred with -m32, but I believe it is an alias issue. The whole handling of the conversion (prep code, library call, post-library handling) looks extremely fragile and this is not the first issue in that this code causes. I think the proper solution – having tons of advantages - is to move the library code to the compiler itself and making use of the compile-time known type, rank, etc. knowledge. 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 v2, Fortran] TS 29113 testsuite
Hi Sandra, hi Thomas, hi all, @Thomas K: Comments about the following - and of course to the testsuite itself - are highly welcome. In my opinion, the testsuite LGTM and can be committed. @Sandra: - Thoughts on the directory name? (cf. below) - Give others/Thomas a chance to comment on this, before committing it. (And remove the now passing xfails.) Thanks for the testsuite! Regarding: * XFAILS - as discussed before, I think having some XFAILS is not ideal but fine, especially if the XFAIL/PASS ratio is low and there are plans to fix the known fails, some posted patches for those, and open PRs for the issues. (I think there is one patch pending review and two patches pending committal with some modifications by Sandra - plus several patches by José which still need to be reviewed.) * Naming of the directory + .exp file: ts29113/ts29113.exp is okay. Given that 'select rank' (in F2018 but not in TS29113) is also tested, there was some controversy regarding the name and the coverage; additionally, TS29113 is a name which is not immediately clear. Thus, we could use some other name like: c-interop/c-interop.exp or (suggestions?). In any case, I do not feel strong about either name. * I had a closer look at earlier versions of the testsuite, I did browse through the current one + looked at the diff to previous version, but it is big enough and the spec is complex enough that I have likely missed something. Thus: Additional reviews are highly welcome! Other than that: On 25.07.21 21:47, Sandra Loosemore wrote: Here is an updated version of my TS29113 testsuite. ... And this approved but not-yet-committed patch from Jose https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572725.html fixes 6 more. The patch was committed as r12-2511-g0cbf03689e3e7d9d6002b8e5d159ef3716d0404c [see also PR fortran/101635 for an open issue]: I can confirm that gfortran.dg/ts29113/interoperability/cf-descriptor-2.f90 now shows XPASS (6 times for -m64, 6 times for -m32). Overall, I see (x86-64-gnu-linux): === gfortran Summary for unix === # of expected passes1041 # of unexpected successes 6 # of expected failures 269 # of unresolved testcases 30 === gfortran Summary for unix/-m32 === # of expected passes1029 # of unexpected successes 6 # of expected failures 257 # of unresolved testcases 30 # of unsupported tests 12 Unexpected success -> see XPASS above Unsupported = dg-require-effective-target, unsupported for -m32. Unresolved = dg-do run, but failed to compile * * * Regarding: gfortran.dg/ts29113/interoperability/contiguous-2.f90 ! FIXME: is this correct? "the Fortran processor will handle the ! difference in contiguity" may not mean that it's required to make ! the array contiguous, just that it can access it correctly? I think the latter is permitted - and makes sense when the contiguity is not needed. For instance, arg = 5 ! -> implicit loop + array-element access does not require a contiguous array, even though assuming sm = sizeof(elem) might generate faster code, e.g. by permitting vector ops. And for size(arg) it does not really matter whether 'arg' is contiguous or not. But for any call which needs a contiguous argument, contiguity is required - which then implies that the compiler has to make the argument contiguous (copy into a contiguous temporary). The simplest is to always do the copy-in-into-temp (when not contiguous), but of course there is room for optimization - like always. In terms of the testsuite, I think the test as written is fine: if later more optimizations are done in GCC, the GCC developer can still inspect the FAIL, read your comment, and modify the testcase accordingly. 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, Fortran] [PR libfortran/101310] Bind(c): Fix bugs in CFI_section
On 18.07.21 06:36, Sandra Loosemore wrote:
This patch fixes bugs I observed in tests for the CFI_section function
-- it turns out both the function and test cases had bugs. :-(
The bugs in CFI_section itself had to do with incorrect computation of
the base address for the result descriptor, plus an ordering problem
that caused it not to work if the source and result descriptors are
the same. I fixed this by rewriting the loop to fill in the dimension
info for the result array and reordering it with respect to the base
address computation.
Note that the old version of CFI_section attempted to preserve the
lower bounds of the section passed in as an argument.
Namely: for CFI_section(result, source, /*lower_bound */ {5, 7}, ...),
the result was: 5 and 6 for i=1,2 in result->dim[i].lower_bound.
With the attached patch, it is == 0. The latter is closer to the
generic Fortran behavior, where in Fortran: lbound(A(5:,7:)) == [1,1].
And CFI arrays passed to nonallocatable, nonpointer dummies have [0,0] as
lower bound.
This is not actually required by the Fortran standard, which specifies
only the shape of the result array, not its bounds. My rewritten
version produces an array with zero lower bounds, similar to what
CFI_establish produces given the shape as input.
I think something has to set a lower bound. For
CFI_establish (dv, /* base_addr = */ NULL, ...)
it is not mandated to set dv->dim[i].lower_bound (as base_addr=NULL).
On the other hand, CFI_section(...) also does not explicitly require it
to be set.
Hence, lower_bound can be unset – but that does not work well, especially
not when the uninitialized lower_bound either exceeds 'int' or even
'ptrdiff_t'.
This issue shows up when not setting lower_bound in CFI_section, e.g.
at
ts29113/interoperability/cf-descriptor-6-c.c's ctest()
(submitted v2 testsuite, not yet committed),
which uses 'int' and overflows.
And for
gcc/testsuite/gfortran.dg/ISO_Fortran_binding_10.c
which assumed in the CFI_address call that lower_bound == 0.
* * *
Thus: I think we really should set it in CFI_section – and the choice
to set it to 0 is in my opinion better than to set it to lower_bound.
I wonder whether we need to add a comment before
result->dim[o].lower_bound = 0;
Maybe:
/* If result was established with base_addr = NULL, its lower bound it
unset; while Fortran 2018 does not specify that CFI_section updates
the lower_bound, setting it to zero is sensible and in line with
Fortran subsections having a lower bound of 1. */
The bug in the older ISO_Fortran_binding_1.c testcase was an incorrect
assertion about the lower bound behavior,
Concur.
while the bugs in the not-yet-committed TS29113 testsuite were due to
me having previously lost track of having fixed this already and just
failing to save the fix before I posted the testsuite patch. As with
the other patches I've been posting for TS29113 testsuite issues, I
can refactor the testsuite changes to lump them all in with the base
testsuite patch depending on the order that things get reviewed/committed.
I tried it with the 'dim[i].lower_bound = 0;' line commented out;
in any case, I see the following XPASS with the such-modified patch
applied:
gfortran.dg/ts29113/library/section-1p.f90
gfortran.dg/ts29113/library/section-2p.f90
gfortran.dg/ts29113/library/section-3p.f90
gfortran.dg/ts29113/interoperability/cf-descriptor-2.f90
gfortran.dg/ts29113/interoperability/fc-out-descriptor-7.f90
[PR libfortran/101310] Bind(c): Fix bugs in CFI_section
CFI_section was incorrectly adjusting the base pointer for the result
array twice in different ways. It was also overwriting the array
dimension info in the result descriptor before computing the base
address offset from the source descriptor, which caused problems if
the two descriptors are the same. This patch fixes both problems and
makes the code simpler, too.
A consequence of this patch is that the result array is now 0-based in
all dimensions instead of starting at the numbering to match the first
element of the source array. The Fortran standard only specifies the
shape of the result array, not its lower bounds, so this is permitted
and probably less confusing for users as well as implementors.
2021-07-17 Sandra Loosemore
PR libfortran/101310
libgfortran/
* runtime/ISO_Fortran_binding.c (CFI_section): Fix the base
address computation and simplify the code.
gcc/testsuite/
* gfortran.dg/ISO_Fortran_binding_1.c (section_c): Remove
incorrect assertions.
* gfortran.dg/ts29113/library/section-3.f90: Fix indexing bugs.
* gfortran.dg/ts29113/library/section-3p.f90: Likewise.
* gfortran.dg/ts29113/library/section-4-c.c: New file.
* gfortran.dg/ts29113/library/section-4.f90: New file.
LGTM – thanks for the patch!
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfs
Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169
Hi Tobias,
> > We should rather open a separate PR on auditing the related uses
> > of gfc_match.
>
> I concur – I just wanted to quickly check how many %v are there –
> besides %v, there are also direct calls to gfc_match_variable.
>
> Can you open a PR?
this is now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101652
> The loops are clear – but the
> '!stat->ref || (...ref->type != ARRAY || ref->type != COMPONENT))'
> is still not clear to me.
Ah, I was really missing the point and looking in the wrong place.
Actually, I also do not understand the reason for this version of
the check, and it also leads to a strange accepts-invalid for
certain non-integer STAT variables. Removing the stat->ref part
fixes that without introducing any regression in the testsuite.
(There was an analogous part in the check for ERRMSG.)
> Can you add the (working) test:
>allocate (A, stat=y%stat%kind) ! { dg-error "cannot be a constant" }
>deallocate (A, stat=y%stat%kind) ! { dg-error "cannot be a constant" }
> to your testcase gcc/testsuite/gfortran.dg/allocate_stat_3.f90 ?
Done.
> And also the following one, which does not get diagnosed and, hence,
> later gives an ICE during gimplification.
>
>type tc
> character (len=:), allocatable :: str
>end type tc
> ...
>type(tc) :: z
> ...
>allocate(character(len=13) :: z%str)
>allocate (A, stat=z%str%len)
>deallocate (A, stat=z%str%len)
>
> To fix it, I think the solution is to do the following:
> * In gfc_check_vardef_context, handle also REF_INQUIRY; in the
> for (ref = e->ref; ref && check_intentin; ref = ref->next)
>loop, I think there should be a
> if (ref->type == REF_INQUIRY)
>{
> if (context)
> gfc_error ("Type parameter inquiry for %qs in "
> "variable definition context (%s) at %L",
> name, context, &e->where);
> return false;
>}
> (untested)
This almost worked, needing only a restriction to %KIND and %LEN.
Note that %RE and %IM are usually definable.
> I assume (but have not tested it) that will give
> two error messages for:
>allocate (A, errmsg=z%str%len)
>deallocate (A, errmsg=z%str%len)
> one for the new type-param-inquiry check and one for
>!= BT_CHARACTER
> if you want to prevent the double error, consider to
> replace
> gfc_check_vardef_context (...);
> by
> if (!gfc_check_vardef_context (...))
> goto done_errmsg;
Yes, that is reasonable. Done.
> > Regtested on x86_64-pc-linux-gnu. OK?
> LGTM - except for the two testcase additions proposed above
> and fixing the ICE. If you are happy with my changes and they
> work, feel free add them and commit without further review.
> In either case, I have the feeling we are nearly there. :-)
I have added the updated "final" version of the patch to give
everybody another 24h to have a look, and will commit if nobody
complains.
> Thanks for the patch and the review-modification-review-... patience!
Well, I believe this was really a worthwile review process,
with fixing a few issues on the way before Gerhard finds them...
Thanks,
Harald
Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
gcc/fortran/ChangeLog:
PR fortran/101564
* expr.c (gfc_check_vardef_context): Add check for KIND and LEN
parameter inquiries.
* 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. Remove bogus parts of checks for STAT and ERRMSG.
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: Likewise.
* gfortran.dg/inquiry_type_ref_3.f90: Likewise.
diff --git a/gcc/fortran/expr.c b/gcc/fortran/expr.c
index b11ae7ce5c5..35563a78697 100644
--- a/gcc/fortran/expr.c
+++ b/gcc/fortran/expr.c
@@ -6199,6 +6199,16 @@ gfc_check_vardef_context (gfc_expr* e, bool pointer, bool alloc_obj,
if (!pointer)
check_intentin = false;
}
+ if (ref->type == REF_INQUIRY
+ && (ref->u.i == INQUIRY_KIND || ref->u.i == INQUIRY_LEN))
+ {
+ if (context)
+ gfc_error ("%qs parameter inquiry for %qs in "
+ "variable definition context (%s) at %L",
+ ref->u.i == INQUIRY_KIND ? "KIND" : "LEN",
+ sym->name, context, &e->where);
+ return false;
+ }
}
if (check_intentin
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 i
Re: [PATCH 3/3] [PR libfortran/101305] Fix ISO_Fortran_binding.h paths in gfortran testsuite
On 7/26/21 2:13 PM, Sandra Loosemore wrote:
On 7/26/21 3:45 AM, Tobias Burnus wrote:
[snip]
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.
Unfortunately, I could not get this to work. For installed-tree
testing, this resulted in diagnostics about a nonexistent directory on
the include path. In my i686-pc-linux-gnu build I was having other
problems when I tried build-tree testing using
make check-gfortran RUNTESTFLAGS="--target-board=localhost/m64"
or similar variants of --target-board (it seemed to be ignoring all the
xfails?) so I did not think that could be the way people who normally
test in the build tree can be doing it. And when I tried a recipe like
make check-gfortran RUNTESTFLAGS="ts29113.exp --tool_opts='-m64'"
it found the ISO_Fortran_binding.h via the correct /64-specific path via
a -B option, same as for installed-tree testing.
Since the patch was already approved without additional hacks to include
file paths, I went ahead and pushed it as-is. If somebody can provide
me with an exact recipe for reproducing failures to find the include
file, I'll take another stab at it, but TBH this is far from my area of
expertise. :-(
BTW, I can't find any documentation for what get_multilibs is supposed
to do. It seems to be part of Dejagnu itself rather than the gcc test
support?
-Sandra
