Re: [PATCH] Fortran: deferred-length character optional dummy arguments [PR93762,PR100651]
Hi FX,
On 11/28/23 18:07, FX Coudert wrote:
Hi Harald,
The patch looks OK to me. Probably wait a bit for another opinion, since I’m
not that active and I may have missed something.
Thanks,
FX
thanks for having a look.
In the meantime I got an automated mail from the Linaro testers.
According to it there is a runtime failure of the testcase on
aarch64. I couldn't see any useful traceback or else.
I tried the testcase on x86 with different options and found
an unexpected result only with -fsanitize=undefined and only
for the case of a rank-1 dummy when there is no actual argument
and the passed to another subroutine. (valgrind is happy.)
Reduced reproducer:
! this fails with -fsanitize=undefined
program main
call test_rank1 ()
contains
subroutine test_rank1 (msg1)
character(:), optional, allocatable :: msg1(:)
if (present (msg1)) stop 77
call assert_rank1 ()! <- no problem here
call assert_rank1 (msg1)! <- problematic code path
end
subroutine assert_rank1 (msg2)
character(:), optional, allocatable :: msg2(:)
if (present (msg2)) stop 99 ! <- no problem if commented
end
end
As far as I can tell, this could be a pre-existing (latent)
issue. By looking at the tree-dump, the only thing that
appears fishy has been there before. But then I am only
guessing that this is the problem observed on aarch64.
I have disabled the related call in the testcase of the
attached revised version. As I do not see anything else,
I wonder if one could proceed with the current version
but open a PR for the reduced case above, unless someone
can pinpoint the place that is responsible for the above
failure. (Is it the caller, or rather the function entry
code in the callee?)
Cheers,
Harald
From 63879942b491e23eefc6da4d80c5492434e42ec8 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Tue, 28 Nov 2023 20:19:14 +0100
Subject: [PATCH] Fortran: deferred-length character optional dummy arguments
[PR93762,PR100651]
gcc/fortran/ChangeLog:
PR fortran/93762
PR fortran/100651
* trans-expr.cc (gfc_conv_missing_dummy): The character length for
deferred-length dummy arguments is passed by reference, so that its
value can be returned. Adjust handling for optional dummies.
gcc/testsuite/ChangeLog:
PR fortran/93762
PR fortran/100651
* gfortran.dg/optional_deferred_char_1.f90: New test.
---
gcc/fortran/trans-expr.cc | 22 +++-
.../gfortran.dg/optional_deferred_char_1.f90 | 100 ++
2 files changed, 118 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/optional_deferred_char_1.f90
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index bfe9996ced6..c90c7bbf936 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -2116,10 +2116,24 @@ gfc_conv_missing_dummy (gfc_se * se, gfc_expr * arg, gfc_typespec ts, int kind)
if (ts.type == BT_CHARACTER)
{
- tmp = build_int_cst (gfc_charlen_type_node, 0);
- tmp = fold_build3_loc (input_location, COND_EXPR, gfc_charlen_type_node,
- present, se->string_length, tmp);
- tmp = gfc_evaluate_now (tmp, >pre);
+ /* Handle deferred-length dummies that pass the character length by
+ reference so that the value can be returned. */
+ if (ts.deferred && INDIRECT_REF_P (se->string_length))
+ {
+ tmp = gfc_build_addr_expr (NULL_TREE, se->string_length);
+ tmp = fold_build3_loc (input_location, COND_EXPR, TREE_TYPE (tmp),
+ present, tmp, null_pointer_node);
+ tmp = gfc_evaluate_now (tmp, >pre);
+ tmp = build_fold_indirect_ref_loc (input_location, tmp);
+ }
+ else
+ {
+ tmp = build_int_cst (gfc_charlen_type_node, 0);
+ tmp = fold_build3_loc (input_location, COND_EXPR,
+ gfc_charlen_type_node,
+ present, se->string_length, tmp);
+ tmp = gfc_evaluate_now (tmp, >pre);
+ }
se->string_length = tmp;
}
return;
diff --git a/gcc/testsuite/gfortran.dg/optional_deferred_char_1.f90 b/gcc/testsuite/gfortran.dg/optional_deferred_char_1.f90
new file mode 100644
index 000..0fb0fb5fea1
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/optional_deferred_char_1.f90
@@ -0,0 +1,100 @@
+! { dg-do run }
+! PR fortran/93762
+! PR fortran/100651 - deferred-length character as optional dummy argument
+
+program main
+ implicit none
+ character(:), allocatable :: err_msg, msg3(:)
+ character(:), pointer :: err_msg2 => NULL()
+
+ ! Subroutines with optional arguments
+ call to_int ()
+ call to_int_p ()
+! call test_rank1 ()! this fails with -fsanitize=undefined
+ call assert_code ()
+ call assert_p ()
+ call assert_rank1 ()
+
+ ! Test passing of optional arguments
+ call to_int (err_msg)
+ if (.not. allocated (err_msg)) stop 1
+ if (len (err_msg) /= 7)stop 2
+ if (err_msg(1:7) /= "foo bar") stop 3
+
+ call to_int2 (err_msg)
+ if (.not. allocated (err_msg)) stop 4
+ if (len (err_msg) /= 7)stop 5
+ if (err_msg(1:7) /= "foo bar") stop 6
+
Re: [PATCH] Fortran: deferred-length character optional dummy arguments [PR93762,PR100651]
Hi Harald, The patch looks OK to me. Probably wait a bit for another opinion, since I’m not that active and I may have missed something. Thanks, FX
Re: [PATCH v2] Fortran: fix reallocation on assignment of polymorphic variables [PR110415]
Hi Andrew, On 27.11.23 18:35, Andrew Jenner wrote: This is the second version of the patch - previous discussion at: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636671.html This patch adds the testcase from PR110415 and fixes the bug. The problem is that in a couple of places in trans_class_assignment in trans-expr.cc, we need to get the run-time size of the polymorphic object from the vtbl, but we are currently getting that vtbl from the lhs of the assignment rather than the rhs. This gives us the old value of the size but we need to pass the new size to __builtin_malloc and __builtin_realloc. I'm fixing this by adding a parameter to trans_class_vptr_len_assignment to retrieve the tree corresponding the vptr from the object on the rhs of the assignment, and then passing this where it is needed. In the case where trans_class_vptr_len_assignment returns NULL_TREE for the rhs vptr we use the lhs vptr as before. To get this to work I also needed to change the implementation of trans_class_vptr_len_assignment to create a temporary for the assignment in more circumstances. Currently, the "a = func()" assignment in MAIN__ doesn't hit the "Create a temporary for complication expressions" case on line 9951 because "DECL_P (rse->expr)" is true - the expression has already been placed into a temporary. That means we don't hit the "if (temp_rhs ..." case on line 10038 and go on to get the vptr_expr from "gfc_lval_expr_from_sym (gfc_find_vtab (>ts))" on line 10057 which is the vtbl of the static type rather than the dynamic one from the rhs. So with this fix we create an extra temporary, but that should be optimised away in the middle-end so there should be no run-time effect. I'm not sure if this is the best way to fix this (the Fortran front-end is new territory for me) but I've verified that the testcase passes with this change, fails without it, and that the change does not introduce any FAILs when running the gfortran testcases on x86_64-pc-linux-gnu. After the previous submission, Tobias Burnus found a closely related problem and contributed testcases and a fix for it, which I have incorporated into this version of the patch. The problem in this case is with the __builtin_realloc call that is executed if one polymorphic variable is replaced by another. The return value of this call was being ignored rather than used to replace the pointer being reallocated. Is this OK for mainline, GCC 13 and OG13? LGTM – Thanks! OK for mainline and after some grace period for GCC 13. OG13 does not require approval, hence, either cherry pick directly or wait until it is in GCC 13 and then "git merge" GCC 13 to OG13. Thanks, Tobias gcc/fortran/ PR fortran/110415 * trans-expr.cc (trans_class_vptr_len_assignment): Add from_vptrp parameter. Populate it. Don't check for DECL_P when deciding whether to create temporary. (trans_class_pointer_fcn, gfc_trans_pointer_assignment): Add NULL argument to trans_class_vptr_len_assignment calls. (trans_class_assignment): Get rhs_vptr from trans_class_vptr_len_assignment and use it for determining size for allocation/reallocation. Use return value from realloc. gcc/testsuite/ PR fortran/110415 * gfortran.dg/pr110415.f90: New test. * gfortran.dg/asan/pr110415-2.f90: New test. * gfortran.dg/asan/pr110415-3.f90: New test. - 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
[Patch] OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'
I stumbled over this omission when looking at Sandra's patch. It turned out
that this is
a new OpenMP 5.2 feature - probably added to simplify/unify the syntax. I guess
the reason
that release/acquire wasn't added before is that it cannot be universally be
used - read/write
do only accept one of them.
However, as a compilation unit might only/mostly use read (and update) or write
(and update),
that can be fine - especially as overriding the default clause is still
possible.
It is not quite clear to me why, but the current patch also fixes a bug
regarding the
diagnostic message for gfortran.dg/gomp/requires-5.f90. (I think I could find
out, but
as it changed to the better...)
Comments, suggestions?
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
OpenMP: Support acquires/release in 'omp require atomic_default_mem_order'
This is an OpenMP 5.2 feature.
gcc/c/ChangeLog:
* c-parser.cc (c_parser_omp_requires): Handle acquires/release
in atomic_default_mem_order clause.
(c_parser_omp_atomic): Update.
gcc/cp/ChangeLog:
* parser.cc (cp_parser_omp_requires): Handle acquires/release
in atomic_default_mem_order clause.
(cp_parser_omp_atomic): Update.
gcc/fortran/ChangeLog:
* gfortran.h (enum gfc_omp_requires_kind): Add
OMP_REQ_ATOMIC_MEM_ORDER_ACQUIRE and OMP_REQ_ATOMIC_MEM_ORDER_RELEASE.
(gfc_namespace): Add a 7th bit to omp_requires.
* module.cc (enum ab_attribute): Add AB_OMP_REQ_MEM_ORDER_ACQUIRE
and AB_OMP_REQ_MEM_ORDER_RELEASE
(mio_symbol_attribute): Handle it.
* openmp.cc (gfc_omp_requires_add_clause): Update for acquire/release.
(gfc_match_omp_requires): Likewise.
(gfc_match_omp_atomic): Handle them for atomic_default_mem_order.
* parse.cc: Likewise.
gcc/testsuite/ChangeLog:
* c-c++-common/gomp/requires-3.c: Update for now valid code.
* gfortran.dg/gomp/requires-3.f90: Likewise.
* gfortran.dg/gomp/requires-2.f90: Update dg-error.
* gfortran.dg/gomp/requires-5.f90: Likewise.
* c-c++-common/gomp/requires-5.c: New test.
* c-c++-common/gomp/requires-6.c: New test.
* c-c++-common/gomp/requires-7.c: New test.
* c-c++-common/gomp/requires-8.c: New test.
* gfortran.dg/gomp/requires-10.f90: New test.
* gfortran.dg/gomp/requires-11.f90: New test.
gcc/c/c-parser.cc | 32 +++-
gcc/cp/parser.cc | 32 +++-
gcc/fortran/gfortran.h | 22 ++-
gcc/fortran/module.cc | 19 +
gcc/fortran/openmp.cc | 53 +-
gcc/fortran/parse.cc | 8
gcc/testsuite/c-c++-common/gomp/requires-3.c | 8 ++--
gcc/testsuite/c-c++-common/gomp/requires-5.c | 23 +++
gcc/testsuite/c-c++-common/gomp/requires-6.c | 23 +++
gcc/testsuite/c-c++-common/gomp/requires-7.c | 11 ++
gcc/testsuite/c-c++-common/gomp/requires-8.c | 14 +++
gcc/testsuite/gfortran.dg/gomp/requires-10.f90 | 36 +
gcc/testsuite/gfortran.dg/gomp/requires-11.f90 | 31 +++
gcc/testsuite/gfortran.dg/gomp/requires-2.f90 | 2 +-
gcc/testsuite/gfortran.dg/gomp/requires-3.f90 | 7 ++--
gcc/testsuite/gfortran.dg/gomp/requires-5.f90 | 2 +-
16 files changed, 291 insertions(+), 32 deletions(-)
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index df9a07928b5..5700c49 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -20896,6 +20896,28 @@ c_parser_omp_atomic (location_t loc, c_parser *parser, bool openacc)
case OMP_MEMORY_ORDER_SEQ_CST:
memory_order = OMP_MEMORY_ORDER_SEQ_CST;
break;
+ case OMP_MEMORY_ORDER_ACQUIRE:
+ if (code == NOP_EXPR) /* atomic write */
+ {
+ error_at (loc, "%<#pragma omp atomic write%> incompatible with "
+ "% clause implicitly provided by a "
+ "% directive");
+ memory_order = OMP_MEMORY_ORDER_SEQ_CST;
+ }
+ else
+ memory_order = OMP_MEMORY_ORDER_ACQUIRE;
+ break;
+ case OMP_MEMORY_ORDER_RELEASE:
+ if (code == OMP_ATOMIC_READ)
+ {
+ error_at (loc, "%<#pragma omp atomic read%> incompatible with "
+ "% clause implicitly provided by a "
+ "% directive");
+ memory_order = OMP_MEMORY_ORDER_SEQ_CST;
+ }
+ else
+ memory_order = OMP_MEMORY_ORDER_RELEASE;
+ break;
case OMP_MEMORY_ORDER_ACQ_REL:
switch (code)
{
@@ -25724,15 +25746,21 @@ c_parser_omp_requires (c_parser *parser)
else if (!strcmp (p, "relaxed"))
this_req
= (enum omp_requires) OMP_MEMORY_ORDER_RELAXED;
+ else if (!strcmp (p, "release"))
+ this_req
+ = (enum omp_requires) OMP_MEMORY_ORDER_RELEASE;
else if (!strcmp (p, "acq_rel"))
this_req
= (enum omp_requires) OMP_MEMORY_ORDER_ACQ_REL;
+
