[Patch] Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621]
[The following is about Fortran pointers as actual argument to a CFI taking
procedure.]
The issue has been marked as 12/13 regression but the issue is just a
diagnostic one.
To disentangle:
(A) Bogus warning
[Now tracked as middle-end https://gcc.gnu.org/PR108906 ]
Assume:
nullify(p)
call bind_c_proc(p)
For some reasons, the compiler does not propagate the NULL and thus assumes that
if (addr != NULL) could be true. This leads to a may-be-uninit warning with
'-Wall -O0'.
(And no warning with -Og/-Os/-O1.)
We could silence it on the gfortran side, I think, by tweaking some tree
properties,
but I am not sure we want to to it.
(The same kind of code is in GCC 11 but as it is hidden in libgfortran; hence,
there are
no warnings when compiling user code. Since GCC 12, the compiler does the
conversion
in place when generating the code.)
(B) The attached patch:
With 'intent(out)' there is no reason to do the conversions. While for nullified
pointers the bounds-conversion loop is skipped, it may still be executed for
undefined
pointers. (Which is usually harmless.) In either case, not generating this code
makes
sense.
OK for mainline?
Regarding GCC 12: I am not really sure as it is no real regression. Besides
bogus
warnings, there might be an issue for undefined pointers and
-fsanitize=undefined, namely
if 'ubound - lbound' evaluated on random numbers overflows (such as for ubound
= huge(..)
and lbound = -huge(..)). But that looks like a rather special case. - Thoughts?
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
Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621]
When the dummy argument of the bind(C) proc is 'pointer, intent(out)', the conversion
of the GFC to the CFI bounds can be skipped: it is not needed and avoids issues with
noninit memory.
Note that the 'cfi->base_addr = gfc->addr' assignment is kept as the C code of a user
might assume that a nullified pointer arrives as NULL (or even a specific value).
For instance, gfortran.dg/c-interop/section-{1,2}.f90 assumes the value NULL.
Note 2: The PR is about a may-be-uninitialized warning with intent(out). In the PR's
testcase, the pointer was nullified and should not have produced that warning.
That is a diagnostic issue, now tracked as PR middle-end/108906 as the issue in principle
still exists (e.g. with 'intent(inout)'). [But no longer for intent(out).]
Note 3: With undefined pointers and no 'intent', accessing uninit memory is unavoidable
on the caller side as the compiler cannot know what the C function does (but this usage
determines whether the pointer is permitted be undefined or whether the bounds must be
gfc-to-cfi converted).
gcc/fortran/ChangeLog:
PR fortran/108621
* trans-expr.cc (gfc_conv_gfc_desc_to_cfi_desc):
gcc/testsuite/ChangeLog:
PR fortran/108621
* gfortran.dg/c-interop/fc-descriptor-pr108621.f90: New test.
gcc/fortran/trans-expr.cc | 6 ++
.../c-interop/fc-descriptor-pr108621.f90 | 65 ++
2 files changed, 71 insertions(+)
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index e85b53fae85..045c8b00b90 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -5673,6 +5673,9 @@ gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
gfc_add_modify (&block, tmp,
build_int_cst (TREE_TYPE (tmp), attr));
+ /* The cfi-base_addr assignment could be skipped for 'pointer, intent(out)'.
+ That is very sensible for undefined pointers, but the C code might assume
+ that the pointer retains the value, in particular, if it was NULL. */
if (e->rank == 0)
{
tmp = gfc_get_cfi_desc_base_addr (cfi);
@@ -5695,6 +5698,9 @@ gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym)
gfc_add_modify (&block, tmp2, fold_convert (TREE_TYPE (tmp2), tmp));
}
+ if (fsym->attr.pointer && fsym->attr.intent == INTENT_OUT)
+goto done;
+
/* When allocatable + intent out, free the cfi descriptor. */
if (fsym->attr.allocatable && fsym->attr.intent == INTENT_OUT)
{
diff --git a/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90 b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90
new file mode 100644
index 000..9c9062bd62d
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90
@@ -0,0 +1,65 @@
+! { dg-do compile }
+! { dg-additional-options "-fdump-tree-original" }
+!
+! PR fortran/108621
+!
+! If the bind(C) procedure's dummy argument is a POINTER with INTENT(OUT),
+! avoid converting the array bounds for the CFI descriptor before the call.
+!
+! Rational: Fewer code and, esp. for undefined pointers, there might be a
Re: Support for NOINLINE attribute
Hello Bernhard,
On Fri, Feb 24, 2023 at 9:20 AM Bernhard Reutner-Fischer
wrote:
> > > * decl.cc: Add EXT_ATTR_NOINLINE, EXT_ATTR_NORETURN, EXT_ATTR_WEAK.
> > > * gfortran.h (ext_attr_id_t): Ditto.
> >
> > We had that discussion recently here..
> > Which of these are required to be recorded to the module and why,
> > exactly? Please elaborate.
> The aforementioned discussion was here:
> https://gcc.gnu.org/pipermail/fortran/2022-November/058542.html
>
> So, again, please elaborate why you need to store each of NOINLINE,
> NORETURN, WEAK in the module?
1. In .mod files technically only NORETURN needs to be stored for
module contained procedures callsites. It should be handled the same
as the DEPRECATED attribute.
module foo
!GCC$ ATTRIBUTES noreturn :: abort_msg
contains
subroutine abort_msg(cmsg)
! ...
end subroutine)
end module
Otherwise there will be failure in already added test:
FAIL: gfortran.dg/noreturn-1.f90 -O detect falling off end of
noreturn (test for warnings, line 9)
2. The NOINLINE attribute can be excluded from on-disk .mod file
until/if there would be added a way to inline say scalar functions
that evaluate to constants at compile time when compiling with
optimizations.
3. The WEAK attribute certainly needs to be excluded from on-disk
generated .mod files, I do not see a legitimate use case where such
information would be used in module imports. Except maybe for
FL_SUBMODULE cases that I so far have not investigated how it
interacts with FL_MODULE. My attempt to mask out NOINLINE/WEAK was
with:
diff --git a/gcc/fortran/module.cc b/gcc/fortran/module.cc
index 601497e0998..1789be88b73 100644
--- a/gcc/fortran/module.cc
+++ b/gcc/fortran/module.cc
@@ -2245,7 +2245,7 @@ static void
mio_symbol_attribute (symbol_attribute *attr)
{
atom_type t;
- unsigned ext_attr,extension_level;
+ unsigned ext_attr,extension_level,ext_mask;
mio_lparen ();
@@ -2255,7 +2255,12 @@ mio_symbol_attribute (symbol_attribute *attr)
attr->if_source = MIO_NAME (ifsrc) (attr->if_source, ifsrc_types);
attr->save = MIO_NAME (save_state) (attr->save, save_status);
- ext_attr = attr->ext_attr;
+ /* Do not save EXT_ATTR_NOINLINE and EXT_ATTR_WEAK into .mod file. */
+ ext_mask = (1 << EXT_ATTR_LAST) - 1;
+ ext_mask -= 1 << EXT_ATTR_NOINLINE;
+ ext_mask -= 1 << EXT_ATTR_WEAK;
+
+ ext_attr = attr->ext_attr & ext_mask;
mio_integer ((int *) &ext_attr);
attr->ext_attr = ext_attr;
And results in failures for testcases submitted for review in part2
and part3 patches:
FAIL: gfortran.dg/noinline-2.f90 -O scan-tree-dump-times dom2 "foo (" 4
FAIL: gfortran.dg/weak-2.f90 -O scan-assembler weak[^ \\t]*[
\\t]_?__foo_MOD_abc
I am not sure if this is a correct place to do such masking and/or why
it results in these specific failures.
Regards,
Rimvydas
Re: adding attributes
I noticed other attributes have been added now. What happened to target_clones, in particular? (Thanks for working on it.)
[PATCH, committed] Fortran: frontend passes do_subscript leaks gmp memory [PR108924]
Dear all,
as reported by Richard - although without a testcase - we leak
gmp memory in do_subscript(). The attached patch was derived
by inspection of the code pointed at by valgrind and regtested
on x86_64-pc-linux-gnu.
Committed as obvious as
commit r13-6336-g45f406c4f62e516b58dcda20b5a7aa43ff0aa0f3
Author: Harald Anlauf
Date: Fri Feb 24 19:56:32 2023 +0100
Thanks,
Harald
From 45f406c4f62e516b58dcda20b5a7aa43ff0aa0f3 Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Fri, 24 Feb 2023 19:56:32 +0100
Subject: [PATCH] Fortran: frontend passes do_subscript leaks gmp memory
[PR108924]
gcc/fortran/ChangeLog:
PR fortran/108924
* frontend-passes.cc (do_subscript): Clear used gmp variable.
---
gcc/fortran/frontend-passes.cc | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/gcc/fortran/frontend-passes.cc b/gcc/fortran/frontend-passes.cc
index 02fcb41dbc4..90428982023 100644
--- a/gcc/fortran/frontend-passes.cc
+++ b/gcc/fortran/frontend-passes.cc
@@ -2883,7 +2883,10 @@ do_subscript (gfc_expr **e)
have_do_end = false;
if (!have_do_start && !have_do_end)
- return 0;
+ {
+ mpz_clear (do_step);
+ return 0;
+ }
/* No warning inside a zero-trip loop. */
if (have_do_start && have_do_end)
--
2.35.3
[pushed] fortran: Plug leak of associated_dummy memory. [PR108923]
Hello,
I have just pushed a for PR108923 (a memory leak).
It fixes the small reproducer that I pasted in bugzilla, and I have run
it through the fortran regression testsuite.
More details in the patch.From 545a7d5da5fcc338e29c5241b574ac99d03f4454 Mon Sep 17 00:00:00 2001
From: Mikael Morin
Date: Fri, 24 Feb 2023 22:11:17 +0100
Subject: [PATCH] fortran: Plug leak of associated_dummy memory. [PR108923]
This fixes a memory leak by accompanying the release of
gfc_actual_arglist elements' memory with a release of the
associated_dummy field memory (if allocated).
Actual argument copy is adjusted as well so that each copy can free
its field independently.
PR fortran/108923
gcc/fortran/ChangeLog:
* expr.cc (gfc_free_actual_arglist): Free associated_dummy
memory.
(gfc_copy_actual_arglist): Make a copy of the associated_dummy
field if it is set in the original element.
---
gcc/fortran/expr.cc | 7 +++
1 file changed, 7 insertions(+)
diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
index c295721b9d6..4662328bf31 100644
--- a/gcc/fortran/expr.cc
+++ b/gcc/fortran/expr.cc
@@ -545,6 +545,7 @@ gfc_free_actual_arglist (gfc_actual_arglist *a1)
a2 = a1->next;
if (a1->expr)
gfc_free_expr (a1->expr);
+ free (a1->associated_dummy);
free (a1);
a1 = a2;
}
@@ -565,6 +566,12 @@ gfc_copy_actual_arglist (gfc_actual_arglist *p)
new_arg = gfc_get_actual_arglist ();
*new_arg = *p;
+ if (p->associated_dummy != NULL)
+ {
+ new_arg->associated_dummy = gfc_get_dummy_arg ();
+ *new_arg->associated_dummy = *p->associated_dummy;
+ }
+
new_arg->expr = gfc_copy_expr (p->expr);
new_arg->next = NULL;
--
2.39.1
Re: [Patch] Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621]
Hi Tobias, Am 24.02.23 um 12:31 schrieb Tobias Burnus: (B) The attached patch: With 'intent(out)' there is no reason to do the conversions. While for nullified pointers the bounds-conversion loop is skipped, it may still be executed for undefined pointers. (Which is usually harmless.) In either case, not generating this code makes sense. OK for mainline? LGTM. I was pondering whether one should keep the testcase closer to the one in the PR, but the essence of the bug and the fix is well represented in the reduced version, and also the tree dump tells the whole story anyway. Regarding GCC 12: I am not really sure as it is no real regression. Besides bogus warnings, there might be an issue for undefined pointers and -fsanitize=undefined, namely if 'ubound - lbound' evaluated on random numbers overflows (such as for ubound = huge(..) and lbound = -huge(..)). But that looks like a rather special case. - Thoughts? I'd rather consider the case of undefined pointers as of practical importance. It's up to you or others to decide whether it should be backported. I would not oppose. Thanks for the patch! Harald 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: Support for WEAK attribute, part 2
Hi Rimvydas,
Am 24.02.23 um 06:16 schrieb Rimvydas Jasinskas via Gcc-patches:
On Thu, Feb 23, 2023 at 10:53 PM Harald Anlauf wrote:
the patch is mostly fine, but there is a minor style issue:
+ if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+ gfc_error ("Symbol %qs at %L has the WEAK attribute but is a %s",
+ sym->name, &sym->declared_at, sym->attr.dummy
+ ? "dummy argument" : "local variable");
+
It is my understanding that this is not translation-friendly.
Please use separate error texts for either case instead.
Interesting, I was under the impression this was fixed with OO-inlines
around the *.c rename.
if this is the case, I must have missed it.
> In any case, adjusted in v2 to use:
+ if (sym->attr.ext_attr & (1 << EXT_ATTR_WEAK))
+{
+ if (sym->attr.dummy)
+gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+ "dummy argument", sym->name, &sym->declared_at);
+ else
+gfc_error ("Symbol %qs at %L has the WEAK attribute but is a "
+ "local variable", sym->name, &sym->declared_at);
+}
This is ok.
These testcases are dg-compile and do not go through the "-O0 -O1 -O2
-O3 -Os" options like dg-run. Combining the testcases does not reduce
gfortran.sum a lot:
I wasn't thinking of gfortran.sum, it's about the total overhead of
the testsuite (dejagnu etc.). But thanks for combining the tests!
Finally, please do not forget to CC patches to gcc-patches@
so that others can see them.
Out of curiosity, what is the purpose of CC patches to gcc-patches
too? Attachments are even available in web mailing list too, like in:
https://gcc.gnu.org/pipermail/fortran/2023-February/058953.html
Well, patches should always go the gcc-patches@, see e.g.
https://gcc.gnu.org/gitwrite.html
On the other hand, many *Fortran* reviewers will ignore patches
there and look at them only when they are sent to fortran@.
Thanks for your patch, pushed as r13-6338-gbcbeebc498126c .
Harald
Regards,
Rimvydas
