Re: [PATCH] [PR125747] Fix shadow-variable bugs for DO, CONCURRENT/FORALL type-spec
Jerry D <[email protected]> Sun, 14 Jun 2026 13:45:31 -0700
| Newsgroups | gmane.comp.gcc.patches,gmane.comp.gcc.fortran |
|---|---|
| Message-ID | <[email protected]> |
On 6/14/26 1:15 AM, Thomas Koenig wrote: > Hi Jerry, > > >> This patch fixes a problem with nested DO_CONCURRENT loops. In short, shadow >> variables were not unique between the nested levels. >> >> Regression tested on x86_64. > >> OK for mainline? > > I have a couple of questions. > > + int nunderscore = 1; > + name = (char *) alloca (strlen (v->symtree->name) + 32); > > This restricts the number of underscores to 32, and will break if there > are more. I would suggest declaring a static counter which adds the > number to a string, like n_vars in frontend-passes.cc , which is then > used in create_vars. > I have updated the patch, attached, to remove the limitation on underscores and use a counter to generate unique names. > The test case do_concurrent_typesspec_f90 has > > + s2 = 0 > + total = 0 > + do concurrent (integer :: i = 1:2) > + s2 = s2 + i > + do concurrent (integer(2) :: i = 1:3) > + total = total + int (i, kind (total)) > + end do > + end do > > As far as I read the Fortran standard, the > > do concurrent (integer :: i = 1:2) > s2 = s2 + i > > is illegal because s2 has unspecified locality, and > 11.1.7.5 Additional semantics for DO CONCURRENT constructs > states (paragraph 7) > Agree, I reviewed my copy of the 2023 draft standard as well. I think it is on the user to not do this. I updated the test cases to used reduce to ensure validity for all the test cases If we want to diagnose someone doing the wrong things I think that needs to be for later effort. > If a variable has unspecified locality > > if it is referenced in an iteration it shall either be previously > defined during that iteration, or shall not be defined or become > undefined during any other iteration; if it is defined or becomes > undefined by more than one iteration it becomes undefined when the loop > terminates; > > [...] > > Would the patch also work for the (slightly nonsensical, but also > legal, AFAIK) > > do concurrent (integer :: i = 1:2) > do concurrent (integer(2) :: i = 1:3) > do concurrent (integer(8) :: i = 1:3) > Yes and a similar testcase is included. > And finally, I didn't dig deep enough into the standard to see if > > do concurrent (integer :: i = 1:2) > do concurrent (integer(2) :: i = 1:3) > do concurrent (integer :: i = 1:3) > > is legal or not, and what the compiler should do in this case. > Yes the above I think is legal and will compile. > Best regards > > Thomas >
PR125747-do-concurrent-forall-typespec-shadow.patch
(text/x-patch, 11.7 KB)
From 423deda167c480f902a9278bbbd96eb9209cd03e Mon Sep 17 00:00:00 2001 From: Jerry DeLisle <[email protected]> Date: Thu, 11 Jun 2026 12:05:15 -0700 Subject: [PATCH] fortran: [PR125747] Fix shadow-variable bugs for DO CONCURRENT/FORALL type-spec When a DO CONCURRENT or FORALL construct gives its index-name an explicit type-spec (F2018 19.4(6)) and that name already exists in the enclosing scope, the front end creates a "shadow" variable with the requested type and substitutes references to the outer-scope variable with the shadow inside the construct's body. Three related bugs in this mechanism could cause an ICE or wrong code: 1. The shadow variable was always named "_<name>", so two sibling constructs reusing the same index-name with different type-specs ended up sharing (and mutating) the same shadow symbol, corrupting already-resolved bounds and triggering an ICE in gfc_add_modify_loc. 2. gfc_resolve_forall derived the shadowed outer-scope symbol by stripping a single leading underscore from the shadow variable's name. Once shadow names were uniquified (fix 1) this derivation broke for "__name", "___name", etc., causing the body substitution to target the wrong symbol and produce wrong runtime values. 3. The generic block-recursion in replace_in_code_recursive unconditionally descended into nested FORALL/DO CONCURRENT bodies during an outer construct's substitution pass, prematurely rewriting references to the shared index-name before the nested construct (which shadows the same name itself) performed its own substitution. Fix: uniquify shadow-variable names against the current namespace using a monotonically increasing counter, record the shadowed outer-scope symbol directly in a new shadow_outer_sym field instead of deriving it from the shadow name, and skip block-recursion into nested FORALL/DO CONCURRENT constructs that shadow the same outer symbol. Assisted by: Claude Sonnet 4.6 PR fortran/125747 gcc/fortran/ChangeLog: * gfortran.h (gfc_forall_iterator): Add shadow_outer_sym field. * match.cc (apply_typespec_to_iterator): Uniquify the shadow variable name against the current namespace using a monotonically increasing counter, and record the shadowed outer-scope symbol in shadow_outer_sym. * resolve.cc (replace_in_code_recursive): Skip recursing into a nested FORALL/DO CONCURRENT body that itself shadows old_sym. (gfc_resolve_forall): Use shadow_outer_sym directly instead of deriving the shadowed symbol from the shadow variable's name. gcc/testsuite/ChangeLog: * gfortran.dg/do_concurrent_typespec_2.f90: New test. * gfortran.dg/do_concurrent_typespec_3.f90: New test. * gfortran.dg/do_concurrent_typespec_4.f90: New test. --- gcc/fortran/gfortran.h | 2 + gcc/fortran/match.cc | 21 +++++++-- gcc/fortran/resolve.cc | 46 ++++++++++--------- .../gfortran.dg/do_concurrent_typespec_2.f90 | 32 +++++++++++++ .../gfortran.dg/do_concurrent_typespec_3.f90 | 32 +++++++++++++ .../gfortran.dg/do_concurrent_typespec_4.f90 | 35 ++++++++++++++ 6 files changed, 142 insertions(+), 26 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/do_concurrent_typespec_2.f90 create mode 100644 gcc/testsuite/gfortran.dg/do_concurrent_typespec_3.f90 create mode 100644 gcc/testsuite/gfortran.dg/do_concurrent_typespec_4.f90 diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h index ef6239cd667..4c9eeda43a2 100644 --- a/gcc/fortran/gfortran.h +++ b/gcc/fortran/gfortran.h @@ -3179,6 +3179,8 @@ typedef struct gfc_forall_iterator gfc_loop_annot annot; /* index-name shadows a variable from outer scope. */ bool shadow; + /* The shadowed outer-scope symbol, set when SHADOW is true. */ + gfc_symbol *shadow_outer_sym; struct gfc_forall_iterator *next; } gfc_forall_iterator; diff --git a/gcc/fortran/match.cc b/gcc/fortran/match.cc index ef6be2d2496..6ff3a4a01db 100644 --- a/gcc/fortran/match.cc +++ b/gcc/fortran/match.cc @@ -2819,6 +2819,10 @@ cleanup: } +/* Counter used to uniquify shadow-variable names created below. */ + +static int shadow_var_num = 0; + /* Apply type-spec to iterator and create shadow variable if needed. */ static void @@ -2846,14 +2850,23 @@ apply_typespec_to_iterator (gfc_forall_iterator *iter, gfc_typespec *ts, else { /* Variable exists in outer scope - must create shadow to comply - with F2018 19.4(6) scoping rules. */ - name = (char *) alloca (strlen (v->symtree->name) + 2); - strcpy (name, "_"); - strcat (name, v->symtree->name); + with F2018 19.4(6) scoping rules. A sibling DO CONCURRENT/FORALL + construct using the same index-name may already have created a + shadow with the same name; uniquify against a monotonically + increasing counter so that distinct constructs do not share (and + mutate) the same shadow symbol. */ + name = (char *) alloca (GFC_MAX_SYMBOL_LEN + 1); + do + snprintf (name, GFC_MAX_SYMBOL_LEN + 1, "_%s_%d", + v->symtree->name, shadow_var_num++); + while (gfc_find_symtree (gfc_current_ns->sym_root, name) != NULL); + if (gfc_get_sym_tree (name, NULL, &st, false) != 0) gfc_internal_error ("Failed to create shadow variable symtree for " "DO CONCURRENT type-spec at %L", loc); + iter->shadow_outer_sym = iter->var->symtree->n.sym; + v = gfc_get_expr (); v->where = gfc_current_locus; v->expr_type = EXPR_VARIABLE; diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc index 8d2a0349881..c326bc4f3d1 100644 --- a/gcc/fortran/resolve.cc +++ b/gcc/fortran/resolve.cc @@ -12767,8 +12767,22 @@ replace_in_code_recursive (gfc_code *code, gfc_symbol *old_sym, gfc_symtree *new break; } - /* Recurse into blocks */ - if (c->block) + /* Recurse into blocks, unless this is a nested FORALL/DO CONCURRENT + that itself shadows OLD_SYM with its own type-spec; such a nested + construct replaces references to OLD_SYM in its own body with its + own shadow variable, so this outer substitution must not touch + them first. */ + bool skip_nested_shadow = false; + if (c->op == EXEC_FORALL || c->op == EXEC_DO_CONCURRENT) + for (gfc_forall_iterator *fa = c->ext.concur.forall_iterator; + fa; fa = fa->next) + if (fa->shadow && fa->shadow_outer_sym == old_sym) + { + skip_nested_shadow = true; + break; + } + + if (c->block && !skip_nested_shadow) replace_in_code_recursive (c->block->next, old_sym, new_st); } } @@ -12863,30 +12877,18 @@ gfc_resolve_forall (gfc_code *code, gfc_namespace *ns, int forall_save) { if (fa->shadow) { - gfc_symtree *shadow_st; - const char *shadow_name_str; - char *outer_name; + gfc_symtree *shadow_st = fa->var->symtree; - /* fa->var now points to the shadow variable "_name". */ - shadow_name_str = fa->var->symtree->name; - shadow_st = fa->var->symtree; + /* The outer-scope symbol being shadowed was recorded by + apply_typespec_to_iterator; using it directly avoids + deriving it from the shadow variable's name, which is + not reliable once uniquified (e.g. "__i", "___i", ...) + to avoid colliding with sibling constructs' shadows. */ + gfc_symbol *iter_sym = fa->shadow_outer_sym; - if (shadow_name_str[0] != '_') - gfc_internal_error ("Expected shadow variable name to start with _"); - - outer_name = (char *) alloca (strlen (shadow_name_str)); - strcpy (outer_name, shadow_name_str + 1); - - /* Find the ITERATOR symbol in the current namespace. - This is the local DO CONCURRENT variable that body expressions reference. */ - gfc_symtree *iter_st = gfc_find_symtree (ns->sym_root, outer_name); - - if (!iter_st) - /* No iterator variable found - this shouldn't happen */ + if (!iter_sym) continue; - gfc_symbol *iter_sym = iter_st->n.sym; - /* Walk the FORALL/DO CONCURRENT body and replace all references. */ if (code->block && code->block->next) gfc_replace_forall_variable (&code->block->next, iter_sym, shadow_st); diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_typespec_2.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_2.f90 new file mode 100644 index 00000000000..378fd22df1d --- /dev/null +++ b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_2.f90 @@ -0,0 +1,32 @@ +! { dg-do run } +! +! PR fortran/125747 +! Three consecutive DO CONCURRENT constructs reuse the index-name 'i' +! with type-specs. The second and third constructs each need a shadow +! variable per F2018 19.4(6); they used to be assigned the same shadow +! symbol "_i", and the third construct's INTEGER(2) type-spec mutated +! the shadow symbol shared with the second construct, leaving its +! already-converted INTEGER(4) bounds inconsistent with the (now +! INTEGER(2)) loop variable and causing an ICE in gfc_add_modify_loc. + +program p + implicit none + integer :: s1, s2, s3 + + s1 = 0 + do concurrent (integer :: i = 1:3) reduce(+:s1) + s1 = s1 + i + end do + + s2 = 0 + do concurrent (integer :: i = 1:3) reduce(+:s2) + s2 = s2 + i + end do + + s3 = 0 + do concurrent (integer(2) :: i = 1:3) reduce(+:s3) + s3 = s3 + int (i, kind (s3)) + end do + + if (s1 /= 6 .or. s2 /= 6 .or. s3 /= 6) stop 1 +end program p diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_typespec_3.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_3.f90 new file mode 100644 index 00000000000..82b4648aeda --- /dev/null +++ b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_3.f90 @@ -0,0 +1,32 @@ +! { dg-do run } +! +! PR fortran/125747 +! A nested DO CONCURRENT that re-shadows the same index-name 'i' as its +! enclosing construct, with a different type-spec. The outer construct's +! body-substitution (i -> its shadow) must not also rewrite references to +! 'i' inside the nested construct's body, since the nested construct +! shadows 'i' itself and performs its own substitution. + +program p + implicit none + integer :: s1, s2, total + + ! Establish 'i' as INTEGER(4) via a prior sibling construct. + s1 = 0 + do concurrent (integer :: i = 1:3) reduce(+:s1) + s1 = s1 + i + end do + + s2 = 0 + total = 0 + do concurrent (integer :: i = 1:2) reduce(+:s2) + s2 = s2 + i + do concurrent (integer(2) :: i = 1:3) reduce(+:total) + total = total + int (i, kind (total)) + end do + end do + + if (s1 /= 6) stop 1 + if (s2 /= 3) stop 2 + if (total /= 12) stop 3 +end program p diff --git a/gcc/testsuite/gfortran.dg/do_concurrent_typespec_4.f90 b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_4.f90 new file mode 100644 index 00000000000..cacb76f5481 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/do_concurrent_typespec_4.f90 @@ -0,0 +1,35 @@ +! { dg-do run } +! +! PR fortran/125747 +! Deeper nesting of DO CONCURRENT constructs that all reuse the +! index-name 'i' with type-specs, each shadowing the same outer-scope +! symbol. Exercises three levels with three distinct kinds, and a +! non-monotonic reuse where the innermost construct repeats the +! outermost construct's type-spec. + +program p + implicit none + integer :: total1, total2 + + ! Three levels, three distinct kinds (default, kind=2, kind=8). + total1 = 0 + do concurrent (integer :: i = 1:2) reduce(+:total1) + do concurrent (integer(2) :: i = 1:2) reduce(+:total1) + do concurrent (integer(8) :: i = 1:2) reduce(+:total1) + total1 = total1 + int (i, kind (total1)) + end do + end do + end do + if (total1 /= 12) stop 1 + + ! Innermost construct reuses the outermost construct's type-spec. + total2 = 0 + do concurrent (integer :: i = 1:2) reduce(+:total2) + do concurrent (integer(2) :: i = 1:2) reduce(+:total2) + do concurrent (integer :: i = 1:2) reduce(+:total2) + total2 = total2 + i + end do + end do + end do + if (total2 /= 12) stop 2 +end program p -- 2.54.0