Re: [Patch] Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings
Hi Tobias!
On 2023-02-23T17:42:08+0100, Tobias Burnus wrote:
> (Side note: this patch has been committed to OG12 as
> http://gcc.gnu.org/g:55a18d47442 )
I see og12 commit 55a18d4744258e3909568e425f9f473c49f9d13f
"Fortran/OpenMP: Fix mapping of array descriptors and deferred-length strings"
regress existing test cases as follows:
[-PASS:-]{+FAIL:+} gfortran.dg/goacc/finalize-1.f -O
scan-tree-dump-times gimple "(?n)#pragma omp target oacc_exit_data
map\\(delete:MEM <[^>]+> \\[\\(integer\\(kind=.\\)\\[0:\\] \\*\\)_[0-9]+\\]
\\[len: [^\\]]+\\]\\) map\\(to:del_f_p \\[pointer set, len: [0-9]+\\]\\)
map\\(alloc:del_f_p\\.data \\[pointer assign, bias: [^\\]]+\\]\\) finalize$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times gimple
"(?n)#pragma omp target oacc_exit_data map\\(delete:del_f \\[len: [0-9]+\\]\\)
finalize$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times gimple
"(?n)#pragma omp target oacc_exit_data map\\(force_from:MEM <[^>]+>
\\[\\(integer\\(kind=.\\)\\[0:\\] \\*\\)_[0-9]+\\] \\[len: [^\\]]+\\]\\)
map\\(to:cpo_f_p \\[pointer set, len: [0-9]+\\]\\) map\\(alloc:cpo_f_p\\.data
\\[pointer assign, bias: [^\\]]+\\]\\) finalize$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times gimple
"(?n)#pragma omp target oacc_exit_data map\\(force_from:cpo_f \\[len:
[0-9]+\\]\\) finalize$" 1
@@ -54679,7 +54679,7 @@ PASS: gfortran.dg/goacc/finalize-1.f -O
scan-tree-dump-times gimple "(?n)#pr
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times original
"(?n)#pragma acc exit data map\\(from:\\*\\(integer\\(kind=.\\)\\[0:\\] \\*\\)
parm\\.1\\.data \\[len: [^\\]]+\\]\\) map\\(to:cpo_f_p \\[pointer set, len:
[0-9]+\\]\\) map\\(alloc:\\(integer\\(kind=1\\)\\[0:\\] \\* restrict\\)
cpo_f_p\\.data \\[pointer assign, bias: \\(.*int.*\\) parm\\.1\\.data -
\\(.*int.*\\) cpo_f_p\\.data\\]\\) finalize;$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times original
"(?n)#pragma acc exit data map\\(from:cpo_f\\) finalize;$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times original
"(?n)#pragma acc exit data map\\(from:cpo_r\\);$" 1
[-PASS:-]{+FAIL:+} gfortran.dg/goacc/finalize-1.f -O
scan-tree-dump-times original "(?n)#pragma acc exit data
map\\(release:\\*\\(integer\\(kind=.\\)\\[0:\\] \\*\\) parm\\.0\\.data \\[len:
[^\\]]+\\]\\) map\\(to:del_f_p \\[pointer set, len: [0-9]+\\]\\)
map\\(alloc:\\(integer\\(kind=1\\)\\[0:\\] \\* restrict\\) del_f_p\\.data
\\[pointer assign, bias: \\(.*int.*\\) parm\\.0\\.data - \\(.*int.*\\)
del_f_p\\.data\\]\\) finalize;$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times original
"(?n)#pragma acc exit data map\\(release:del_f\\) finalize;$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O scan-tree-dump-times original
"(?n)#pragma acc exit data map\\(release:del_r\\);$" 1
PASS: gfortran.dg/goacc/finalize-1.f -O (test for excess errors)
[-PASS:-]{+FAIL:+} gfortran.dg/gomp/pr78260-2.f90 -O
scan-tree-dump-times original "#pragma omp target data
map\\(tofrom:\\*\\(integer\\(kind=4\\)\\[0:\\] \\* restrict\\) __result->data
\\[len: D.[0-9]+ \\* 4\\]\\) map\\(to:\\*__result \\[pointer set, len: ..\\]\\)
map\\(alloc:\\(integer\\(kind=4\\)\\[0:\\] \\* restrict\\) __result->data
\\[pointer assign, bias: 0\\]\\) map\\(alloc:__result \\[pointer assign, bias:
0\\]\\)" 1
[-PASS:-]{+FAIL:+} gfortran.dg/gomp/pr78260-2.f90 -O
scan-tree-dump-times original "#pragma omp target data
map\\(tofrom:\\*\\(integer\\(kind=4\\)\\[0:\\] \\* restrict\\) arr.data \\[len:
D.[0-9]+ \\* 4\\]\\) map\\(to:arr \\[pointer set, len: ..\\]\\)
map\\(alloc:\\(integer\\(kind=4\\)\\[0:\\] \\* restrict\\) arr.data \\[pointer
assign, bias: 0\\]\\)" 1
PASS: gfortran.dg/gomp/pr78260-2.f90 -O scan-tree-dump-times original
"#pragma omp target data map\\(tofrom:\\*__result.0\\) map\\(alloc:__result.0
\\[pointer assign, bias: 0\\]\\)" 2
PASS: gfortran.dg/gomp/pr78260-2.f90 -O scan-tree-dump-times original
"#pragma omp target data map\\(tofrom:__result_f1\\)" 1
PASS: gfortran.dg/gomp/pr78260-2.f90 -O scan-tree-dump-times original
"#pragma omp target update to\\(\\*\\(integer\\(kind=4\\)\\[0:\\] \\*
restrict\\) __result->data \\[len: D.[0-9]+ \\* 4\\]\\)" 1
Do to the scan patterns need adjusting, or is something wrong?
Grüße
Thomas
-
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: Reuse associated_dummy memory if previously allocated [PR108923]
Hello,
Harald found a testcase with memory still leaking despite my previous
patch for PR108923.
That patch was fixing a leak caused by absence of memory release, the
attached patch fixes a leak caused by pointer overwrite.
I haven't investigated why sort_actual is called several times( which
causes the memory leak) nor tried to avoid that. Theoretically, one
could assert that the previous associated_dummy value is the same as the
one to be written (it should be the same at each sort_actual
invocation), but I have preferred to silently overwrite, and fix just
the memory problem.
Manually tested on Harald's testcase (predcom-1.f) and ran the full
fortran testsuite on x86_64-pc-linux-gnu.
OK for master and 12 and 11?
From 9b88208ec4130712d33d5c7ed74fc17466624a0c Mon Sep 17 00:00:00 2001
From: Mikael Morin
Date: Sat, 25 Feb 2023 16:27:24 +0100
Subject: [PATCH] fortran: Reuse associated_dummy memory if previously
allocated [PR108923]
This avoids making the associted_dummy field point to a new memory chunk
if it's already pointing somewhere, in which case doing so would leak the
previously allocated chunk.
gcc/fortran/ChangeLog:
* intrinsic.cc (get_intrinsic_dummy_arg,
set_intrinsic_dummy_arg): Rename the former to the latter.
Remove the return value, add a reference to the lhs as argument,
and do the pointer assignment inside the function. Don't do
it if the pointer is already non-NULL.
(sort_actual): Update caller.
---
gcc/fortran/intrinsic.cc | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc
index 17ee999c3b9..e69e541efe0 100644
--- a/gcc/fortran/intrinsic.cc
+++ b/gcc/fortran/intrinsic.cc
@@ -4259,15 +4259,14 @@ remove_nullargs (gfc_actual_arglist **ap)
}
-static gfc_dummy_arg *
-get_intrinsic_dummy_arg (gfc_intrinsic_arg *intrinsic)
+static void
+set_intrinsic_dummy_arg (gfc_dummy_arg *&dummy_arg, gfc_intrinsic_arg *intrinsic)
{
- gfc_dummy_arg * const dummy_arg = gfc_get_dummy_arg ();
+ if (dummy_arg == NULL)
+dummy_arg = gfc_get_dummy_arg ();
dummy_arg->intrinsicness = GFC_INTRINSIC_DUMMY_ARG;
dummy_arg->u.intrinsic = intrinsic;
-
- return dummy_arg;
}
@@ -4430,7 +4429,7 @@ do_sort:
if (a == NULL)
a = gfc_get_actual_arglist ();
- a->associated_dummy = get_intrinsic_dummy_arg (f);
+ set_intrinsic_dummy_arg (a->associated_dummy, f);
if (actual == NULL)
*ap = a;
--
2.39.1
Re: fortran: Reuse associated_dummy memory if previously allocated [PR108923]
Hi Mikael, Am 25.02.23 um 17:35 schrieb Mikael Morin: Hello, Harald found a testcase with memory still leaking despite my previous patch for PR108923. That patch was fixing a leak caused by absence of memory release, the attached patch fixes a leak caused by pointer overwrite. I haven't investigated why sort_actual is called several times( which causes the memory leak) nor tried to avoid that. Theoretically, one could assert that the previous associated_dummy value is the same as the one to be written (it should be the same at each sort_actual invocation), but I have preferred to silently overwrite, and fix just the memory problem. Manually tested on Harald's testcase (predcom-1.f) and ran the full fortran testsuite on x86_64-pc-linux-gnu. OK for master and 12 and 11? LGTM. OK for master and 12-branch. It appears that 11-branch is significantly different in the respective places. get_intrinsic_dummy_arg does not exist there, so this patch seems to not apply there. Or am I missing something? Thanks for the patch! Harald
Re: fortran: Reuse associated_dummy memory if previously allocated [PR108923]
Le 25/02/2023 à 18:20, Harald Anlauf a écrit : Hi Mikael, Am 25.02.23 um 17:35 schrieb Mikael Morin: Hello, Harald found a testcase with memory still leaking despite my previous patch for PR108923. That patch was fixing a leak caused by absence of memory release, the attached patch fixes a leak caused by pointer overwrite. I haven't investigated why sort_actual is called several times( which causes the memory leak) nor tried to avoid that. Theoretically, one could assert that the previous associated_dummy value is the same as the one to be written (it should be the same at each sort_actual invocation), but I have preferred to silently overwrite, and fix just the memory problem. Manually tested on Harald's testcase (predcom-1.f) and ran the full fortran testsuite on x86_64-pc-linux-gnu. OK for master and 12 and 11? LGTM. OK for master and 12-branch. It appears that 11-branch is significantly different in the respective places. get_intrinsic_dummy_arg does not exist there, so this patch seems to not apply there. Or am I missing something? No you're right. I had forgotten that a simplified patch had been used to fix pr97896 on the 11 branch. Thanks.
[PATCH, committed] Fortran: fix memory leak with real to integer conversion warning
Dear all,
while checking f951 for memory leaks on testcases that appeared
relevant during work on pr108924, I found that the conversion
warning triggered by do_subscript_6.f90 uses a code path that
forgot to mpfr_clear a used variable.
The attached obvious patch fixes this - verified by valgrind.
Pushed to mainline as r13-6344-g03c60e525bea13 .
Thanks,
Harald
From 03c60e525bea13c15edd2f64cd582f168fe80bfb Mon Sep 17 00:00:00 2001
From: Harald Anlauf
Date: Sat, 25 Feb 2023 19:05:38 +0100
Subject: [PATCH] Fortran: fix memory leak with real to integer conversion
warning
gcc/fortran/ChangeLog:
* arith.cc (gfc_real2int): Clear mpfr variable after use.
---
gcc/fortran/arith.cc | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
index d0d1c0b03d2..37aeaf1b186 100644
--- a/gcc/fortran/arith.cc
+++ b/gcc/fortran/arith.cc
@@ -2257,6 +2257,7 @@ gfc_real2int (gfc_expr *src, int kind)
gfc_typename (&result->ts), &src->where);
did_warn = true;
}
+ mpfr_clear (f);
}
if (!did_warn && warn_conversion_extra)
{
--
2.35.3
drop -fdump-fortran-global ? [was: Re: [PATCH,FORTRAN] Fix memory leak of gsymbol]
On Sun, 31 Oct 2021 21:25:44 +0100 Bernhard Reutner-Fischer wrote: > On Sun, 31 Oct 2021 20:46:07 +0100 > Harald Anlauf wrote: > > > Am 30.10.21 um 23:51 schrieb Bernhard Reutner-Fischer via Fortran: > > >>> The only caller is translate_all_program_units. > > >>> Since we free only module gsyms, even -fdump-fortran-global is > > >>> unaffected by this, fwiw. > > > > > > AFAICS we do not have a test for -fdump-fortran-global > > > Do we want to add one, would the attached be OK? > > > > This doesn't seem to test anything new or changed, or a bug fixed. > > I get the same result for all version from 9 to 12-mainline. > > So as is it seems pointless. > > Yes indeed, it just adds coverage to that functionality which we did not > exercise before. > TBH i only found that option when looking around > translate_all_program_units. I've never actually used that option > myself and cannot imagine how it is useful at all :) > > Dropped the testcase. > Thanks for your comment! The rest of 5c6aa9a8919cbf0dcf3c375f51012720bfb5f3a1 is fine, but should we really keep the option, if we don't even test basics and if it was more a specific debug dump, from the looks? Thomas? thanks,
Re: drop -fdump-fortran-global ? [was: Re: [PATCH, FORTRAN] Fix memory leak of gsymbol]
On 25.02.23 22:55, Bernhard Reutner-Fischer wrote: but should we really keep the option, if we don't even test basics and if it was more a specific debug dump, from the looks? It is always possible to add test cases if testing is required, but I don't think this is necessary. We also do not have test cases for -fdump-fortran-original, which is also quite handy and which can find a lot of bugs. And yes, it is rather useful when debugging issues with global identifiers, which are a bit special in Fortran. As I wrote in my mail when submitting the patch, > While debugging it, I also put in an option to dump the global > symbol table to standard output. I have included this in this > patch because I think this may not be the last bug in that > area 😄 That hasn't changed, IMHO. Best regards Thomas
