gfortran.dg/PR82376.f90: Avoid matching a file-path.
I had a file-path to sources with the substring "new" in it,
and (only) this test regressed compared to results from
another build without "new" in the name.
The test does
! { dg-final { scan-tree-dump-times "new" 4 "original" } }
i.e. the contents of the tree-dump-file .original needs to match
the undelimited string "new" exactly four times. Very brittle.
In the dump-file, there are three lines with calls to new:
D.908 = new ((integer(kind=4) *) data);
integer(kind=4) * new (integer(kind=4) & data)
static integer(kind=4) * new (integer(kind=4) &);
But, there's also a line, which for me and cris-elf looked like:
_gfortran_runtime_error_at (&"At line 46 of file
/X/xyzzynewfrob/gcc/testsuite/gfortran.dg/PR82376.f90"[1]{lb: 1 sz: 1},
&"Pointer actual argument \'new\' is not associated"[1]{lb: 1 sz: 1});
The fourth match is obviously intended to match this line, but only
with *one* match, whereas the path can as above yield another hit.
With Tcl, the regexp for matching the " " *and* the "'"
*and* the "\" gets a bit unsightly, so I suggest just
matching the "new" calls, which according to the comment in
the test is the key point. You can't have a file-path with
spaces and parentheses in a gcc build. I'm also making use
of {} rather than "" needing one level of quoting; the "\("
is needed because the matched string is a regexp.
Ok to commit?
testsuite:
* gfortran.dg/PR82376.f90: Robustify match.
---
gcc/testsuite/gfortran.dg/PR82376.f90 | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/gcc/testsuite/gfortran.dg/PR82376.f90
b/gcc/testsuite/gfortran.dg/PR82376.f90
index 07143ab7e82e..b99779ce9d8a 100644
--- a/gcc/testsuite/gfortran.dg/PR82376.f90
+++ b/gcc/testsuite/gfortran.dg/PR82376.f90
@@ -2,7 +2,8 @@
! { dg-options "-fdump-tree-original -fcheck=pointer" }
!
! Test the fix for PR82376. The pointer check was doubling up the call
-! to new. The fix reduces the count of 'new' from 5 to 4.
+! to new. The fix reduces the count of 'new' from 5 to 4, or to 3, when
+! counting only calls.
!
! Contributed by José Rui Faustino de Sousa
!
@@ -56,4 +57,4 @@ contains
end subroutine set
end program main_p
-! { dg-final { scan-tree-dump-times "new" 4 "original" } }
+! { dg-final { scan-tree-dump-times { new \(} 3 "original" } }
--
2.11.0
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
On Wed, Aug 11, 2021 at 05:55:39AM -0500, Segher Boessenkool wrote: > Hi! > > On Tue, Aug 10, 2021 at 04:46:11PM -0600, Sandra Loosemore wrote: > > OK. I used your wording verbatim for the first one. For the second > > one, I'm still pretty confused as I think it is at least theoretically > > possible on PowerPC to have a target with 64-bit long double (AIX?) that > > Some embedded and embedded-like subtargets use 64-bit long double by > default. You can also configure this on any Power target (not that it > will necessarily work ;-) ) It will work on Linux LE systems with glibc 2.32 (it may work with earlier glibcs). I've built parallel toolchains with all 3 long double formats. There are some tests in the test suite that fail if you configure 64-bit long doubles. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
*PING* [PATCH, part2] PR fortran/98411 [10/11/12 Regression] Pointless: Array larger than ‘-fmax-stack-var-size=’, ...
*Ping* > Gesendet: Mittwoch, 04. August 2021 um 23:09 Uhr > Von: "Harald Anlauf" > An: "fortran" , "gcc-patches" > Betreff: [PATCH, part2] PR fortran/98411 [10/11/12 Regression] Pointless: > Array larger than ‘-fmax-stack-var-size=’, ... > > Dear all, > > here's the second part that should fix this regression for good. > The patch also adjusts the warning message to make it easier to > understand, using the suggestion by Tobias (see PR). > > Since F2018 in principle makes RECURSIVE the default, which might > conflict with the purpose of the testcase, I chose to change the > options to include -std=f2008, and to verify that implicit SAVE > works the same as explicit SAVE. > > Regtested on x86_64-pc-linux-gnu. OK for affected branches? > > Thanks, > Harald > > > Fortran: fix pointless warning for static variables > > gcc/fortran/ChangeLog: > > PR fortran/98411 > * trans-decl.c (gfc_finish_var_decl): Adjust check to handle > implicit SAVE as well as variables in the main program. Improve > warning message text. > > gcc/testsuite/ChangeLog: > > PR fortran/98411 > * gfortran.dg/pr98411.f90: Adjust testcase options to restrict to > F2008, and verify case of implicit SAVE. > >
*PING* Re: [PATCH] PR fortran/100950 - ICE in output_constructor_regular_field, at varasm.c:5514
*Ping*
> Gesendet: Dienstag, 03. August 2021 um 23:17 Uhr
> Von: "Harald Anlauf"
> An: "Harald Anlauf"
> Cc: "Tobias Burnus" , "Bernhard Reutner-Fischer"
> , "Harald Anlauf via Gcc-patches"
> , "fortran"
> Betreff: Re: [PATCH] PR fortran/100950 - ICE in
> output_constructor_regular_field, at varasm.c:5514
>
> Here's now my third attempt to fix this PR, taking into account
> the comments by Tobias and Bernhard.
>
> > > On 10.06.21 20:52, Harald Anlauf via Fortran wrote:
> > > > +static bool
> > > > +substring_has_constant_len (gfc_expr *e)
> > > > +{
> > > > + ptrdiff_t istart, iend;
> > > > + size_t length;
> > > > + bool equal_length = false;
> > > > +
> > > > + if (e->ts.type != BT_CHARACTER
> > > > + || !e->ref
> > > > + || e->ref->type != REF_SUBSTRING
> > >
> > > Is there a reason why you do not handle:
> > >
> > > type t
> > >character(len=5) :: str1
> > >character(len=:), allocatable :: str2
> > > end type
> > > type(t) :: x
> > >
> > > allocate(x%str2, source="abd")
> > > if (len (x%str)) /= 1) ...
> > > if (len (x%str2(1:2) /= 2) ...
> > > etc.
> > >
> > > Namely: Search the last_ref = expr->ref->next->next ...?
> > > and then check that lastref?
>
> The mentioned search is now implemented.
>
> Note, however, that gfc_simplify_len still won't handle neither
> deferred strings nor their substrings.
>
> I think there is nothing to simplify at compile time here. Otherwise
> there would be a conflict/inconsistency with type parameter inquiry,
> see F2018:9.4.5(2):
>
> "A deferred type parameter of a pointer that is not associated or
> of an unallocated allocatable variable shall not be inquired about."
>
> > >* * *
> > >
> > > Slightly unrelated: I think the following does not violate
> > > F2018's R916 / C923 – but is rejected, namely:
> > >R916 type-param-inquiry is designator % type-param-name
> > > the latter is 'len' or 'kind' for intrinsic types. And:
> > >R901 designator is ...
> > > or substring
> > > But
> > >
> > > character(len=5) :: str
> > > print *, str(1:3)%len
> > > end
> > >
> > > fails with
> > >
> > > 2 | print *, str(1:3)%len
> > >| 1
> > > Error: Syntax error in PRINT statement at (1)
> > >
> > >
> > > Assuming you don't want to handle it, can you open a new PR?
> > > Thanks!
>
> I tried to look into this, but there appear to be several unrelated
> issues requiring a separate treatment. I therefore opened:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101735
>
> > > > + istart = gfc_mpz_get_hwi (e->ref->u.ss.start->value.integer);
> > > > + iend = gfc_mpz_get_hwi (e->ref->u.ss.end->value.integer);
> > > > + length = gfc_mpz_get_hwi
> > > > (e->ref->u.ss.length->length->value.integer);
> > > > +
> > > > + if (istart <= iend)
> > > > +{
> > > > + if (istart < 1)
> > > > + {
> > > > + gfc_error ("Substring start index (%ld) at %L below 1",
> > > > + (long) istart, >ref->u.ss.start->where);
> > >
> > > As mentioned by Bernhard, you could use HOST_WIDE_INT_PRINT_DEC.
> > >
> > > (It probably only matters on Windows which uses long == int = 32bit for
> > > strings longer than INT_MAX.)
>
> Done.
>
> The updated patch regtests fine. OK?
>
> Thanks,
> Harald
>
>
> Fortran - simplify length of substring with constant bounds
>
> gcc/fortran/ChangeLog:
>
> PR fortran/100950
> * simplify.c (substring_has_constant_len): New.
> (gfc_simplify_len): Handle case of substrings with constant
> bounds.
>
> gcc/testsuite/ChangeLog:
>
> PR fortran/100950
> * gfortran.dg/pr100950.f90: New test.
>
>
[PATCH] PR fortran/99351 - ICE in gfc_finish_var_decl, at fortran/trans-decl.c:695
Dear all,
the checks for the STAT= and ERRMSG= arguments to the coarray SYNC statements
did not properly handle several cases, such as named constants (parameters).
While fixing this, I adjusted the code similarly to what was recently done
for (DE)ALLOCATE. We now also accept function references with data pointer
result. (See also PR101652).
Regtested on x86_64-pc-linux-gnu. OK for mainline?
Thanks,
Harald
Fortran: fix checks for STAT= and ERRMSG= arguments of SYNC ALL/SYNC IMAGES
gcc/fortran/ChangeLog:
PR fortran/99351
* match.c (sync_statement): Replace %v code by %e in gfc_match to
allow for function references as STAT and ERRMSG arguments.
* resolve.c (resolve_sync): Adjust checks of STAT= and ERRMSG= to
being definable arguments. Function references with a data
pointer result are accepted.
* trans-stmt.c (gfc_trans_sync): Adjust assertion.
gcc/testsuite/ChangeLog:
PR fortran/99351
* gfortran.dg/coarray_sync.f90: New test.
* gfortran.dg/coarray_3.f90: Adjust to change error messages.
diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
index b1105481099..16502da001d 100644
--- a/gcc/fortran/match.c
+++ b/gcc/fortran/match.c
@@ -3855,7 +3855,7 @@ sync_statement (gfc_statement st)
for (;;)
{
- m = gfc_match (" stat = %v", );
+ m = gfc_match (" stat = %e", );
if (m == MATCH_ERROR)
goto syntax;
if (m == MATCH_YES)
@@ -3875,7 +3875,7 @@ sync_statement (gfc_statement st)
break;
}
- m = gfc_match (" errmsg = %v", );
+ m = gfc_match (" errmsg = %e", );
if (m == MATCH_ERROR)
goto syntax;
if (m == MATCH_YES)
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index 592364689f9..959f0bed4fb 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -10236,19 +10236,27 @@ resolve_sync (gfc_code *code)
/* Check STAT. */
gfc_resolve_expr (code->expr2);
- if (code->expr2
- && (code->expr2->ts.type != BT_INTEGER || code->expr2->rank != 0
- || code->expr2->expr_type != EXPR_VARIABLE))
-gfc_error ("STAT= argument at %L must be a scalar INTEGER variable",
- >expr2->where);
+ if (code->expr2)
+{
+ if (code->expr2->ts.type != BT_INTEGER || code->expr2->rank != 0)
+ gfc_error ("STAT= argument at %L must be a scalar INTEGER variable",
+ >expr2->where);
+ else
+ gfc_check_vardef_context (code->expr2, false, false, false,
+ _("STAT variable"));
+}
/* Check ERRMSG. */
gfc_resolve_expr (code->expr3);
- if (code->expr3
- && (code->expr3->ts.type != BT_CHARACTER || code->expr3->rank != 0
- || code->expr3->expr_type != EXPR_VARIABLE))
-gfc_error ("ERRMSG= argument at %L must be a scalar CHARACTER variable",
- >expr3->where);
+ if (code->expr3)
+{
+ if (code->expr3->ts.type != BT_CHARACTER || code->expr3->rank != 0)
+ gfc_error ("ERRMSG= argument at %L must be a scalar CHARACTER variable",
+ >expr3->where);
+ else
+ gfc_check_vardef_context (code->expr3, false, false, false,
+ _("ERRMSG variable"));
+}
}
diff --git a/gcc/fortran/trans-stmt.c b/gcc/fortran/trans-stmt.c
index 7cbdef7a304..11df1863bad 100644
--- a/gcc/fortran/trans-stmt.c
+++ b/gcc/fortran/trans-stmt.c
@@ -1226,7 +1226,8 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type)
if (code->expr2)
{
- gcc_assert (code->expr2->expr_type == EXPR_VARIABLE);
+ gcc_assert (code->expr2->expr_type == EXPR_VARIABLE
+ || code->expr2->expr_type == EXPR_FUNCTION);
gfc_init_se (, NULL);
gfc_conv_expr_val (, code->expr2);
stat = argse.expr;
@@ -1236,7 +1237,8 @@ gfc_trans_sync (gfc_code *code, gfc_exec_op type)
if (code->expr3 && flag_coarray == GFC_FCOARRAY_LIB)
{
- gcc_assert (code->expr3->expr_type == EXPR_VARIABLE);
+ gcc_assert (code->expr3->expr_type == EXPR_VARIABLE
+ || code->expr3->expr_type == EXPR_FUNCTION);
gfc_init_se (, NULL);
argse.want_pointer = 1;
gfc_conv_expr (, code->expr3);
diff --git a/gcc/testsuite/gfortran.dg/coarray_3.f90 b/gcc/testsuite/gfortran.dg/coarray_3.f90
index d152ce1b2bd..1c294cd0189 100644
--- a/gcc/testsuite/gfortran.dg/coarray_3.f90
+++ b/gcc/testsuite/gfortran.dg/coarray_3.f90
@@ -11,11 +11,11 @@ character(len=30) :: str(2)
critical fkl ! { dg-error "Syntax error in CRITICAL" }
end critical fkl ! { dg-error "Expecting END PROGRAM" }
-sync all (stat=1) ! { dg-error "Syntax error in SYNC ALL" }
+sync all (stat=1) ! { dg-error "Non-variable expression" }
sync all ( stat = n,stat=k) ! { dg-error "Redundant STAT" }
sync memory (errmsg=str) ! { dg-error "must be a scalar CHARACTER variable" }
sync memory (errmsg=n) ! { dg-error "must be a scalar CHARACTER variable" }
-sync images (*, stat=1.0) ! { dg-error "Syntax error in SYNC IMAGES" }
+sync images (*, stat=1.0) ! { dg-error "must be a scalar INTEGER variable" }
sync images (-1) ! { dg-error "must between 1 and num_images" }
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
On 8/11/21 2:05 AM, Tobias Burnus wrote: On 11.08.21 00:46, Sandra Loosemore wrote: On 8/10/21 2:29 AM, Tobias Burnus wrote: [snip] To conclude: I like the code changes (LGTM); the '__float128' -> 'TFmode' comment change also matches the code. However, I think both longer comments need to be updated. OK. I used your wording verbatim for the first one. For the second one, I'm still pretty confused as I think it is at least theoretically possible on PowerPC to have a target with 64-bit long double (AIX?) that also supports the __ibm128 format, and it would be wrong to assume that *any* 128-bit mode that's not long double is IEEE. So I decided the best thing is just to replace the FIXME with a pointer to the issue I opened yesterday https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101835 LGTM – but ... + /* See PR101835. */ ... I wonder whether your PR reference should have a TODO or FIXME prefix – or a "for some issue" suffix. Currently, it can be read as if the PR describes why the code was added – and not for questioning the code. OK, thank you. I've pushed the patch with the addition of "TODO" to that comment. -Sandra
Re: [PATCH][v2] Adjust volatile handling of the operand scanner
On Wed, 11 Aug 2021, Eric Botcazou wrote:
> > So I'm leaning towards leaving build3 alone and fixing up frontends
> > as issues pop up.
>
> FWIW fine with me.
OK, so I pushed the original change (reposted below).
Bootstrapped / tested on x86_64-unknown-linux-gnu.
Richard.
>From e5a23d54d189f3d160c82f770683288a15c3645e Mon Sep 17 00:00:00 2001
From: Richard Biener
Date: Mon, 9 Aug 2021 13:12:08 +0200
Subject: [PATCH] Adjust volatile handling of the operand scanner
To: gcc-patc...@gcc.gnu.org
The GIMPLE SSA operand scanner handles COMPONENT_REFs that are
not marked TREE_THIS_VOLATILE but have a TREE_THIS_VOLATILE
FIELD_DECL as volatile. That's inconsistent in how TREE_THIS_VOLATILE
testing on GENERIC refs works which requires operand zero of
component references to mirror TREE_THIS_VOLATILE to the ref
so that testing TREE_THIS_VOLATILE on the outermost reference
is enough to determine the volatileness.
The following patch thus removes FIELD_DECL scanning from
the GIMPLE SSA operand scanner, possibly leaving fewer stmts
marked as gimple_has_volatile_ops.
It shows we miss at least one case in the fortran frontend, though
there's a suspicious amount of COMPONENT_REF creation compared
to little setting of TREE_THIS_VOLATILE. This fixes the FAIL
of gfortran.dg/volatile11.f90 that would otherwise occur.
Visually inspecting fortran/ reveals a bunch of likely to fix
cases but I don't know the constraints of 'volatile' uses in
the fortran language to assess whether some of these are not
necessary.
2021-08-09 Richard Biener
gcc/
* tree-ssa-operands.c (operands_scanner::get_expr_operands):
Do not look at COMPONENT_REF FIELD_DECLs TREE_THIS_VOLATILE
to determine has_volatile_ops.
gcc/fortran/
* trans-common.c (create_common): Set TREE_THIS_VOLATILE on the
COMPONENT_REF if the field is volatile.
---
gcc/fortran/trans-common.c | 9 +
gcc/tree-ssa-operands.c| 7 +--
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/gcc/fortran/trans-common.c b/gcc/fortran/trans-common.c
index a11cf4c839e..7bcf18dc475 100644
--- a/gcc/fortran/trans-common.c
+++ b/gcc/fortran/trans-common.c
@@ -759,10 +759,11 @@ create_common (gfc_common_head *com, segment_info *head,
bool saw_equiv)
else
gfc_add_decl_to_function (var_decl);
- SET_DECL_VALUE_EXPR (var_decl,
- fold_build3_loc (input_location, COMPONENT_REF,
- TREE_TYPE (s->field),
- decl, s->field, NULL_TREE));
+ tree comp = build3_loc (input_location, COMPONENT_REF,
+ TREE_TYPE (s->field), decl, s->field, NULL_TREE);
+ if (TREE_THIS_VOLATILE (s->field))
+ TREE_THIS_VOLATILE (comp) = 1;
+ SET_DECL_VALUE_EXPR (var_decl, comp);
DECL_HAS_VALUE_EXPR_P (var_decl) = 1;
GFC_DECL_COMMON_OR_EQUIV (var_decl) = 1;
diff --git a/gcc/tree-ssa-operands.c b/gcc/tree-ssa-operands.c
index c15575416dd..ebf7eea3b04 100644
--- a/gcc/tree-ssa-operands.c
+++ b/gcc/tree-ssa-operands.c
@@ -834,12 +834,7 @@ operands_scanner::get_expr_operands (tree *expr_p, int
flags)
get_expr_operands (_OPERAND (expr, 0), flags);
if (code == COMPONENT_REF)
- {
- if (!(flags & opf_no_vops)
- && TREE_THIS_VOLATILE (TREE_OPERAND (expr, 1)))
- gimple_set_has_volatile_ops (stmt, true);
- get_expr_operands (_OPERAND (expr, 2), uflags);
- }
+ get_expr_operands (_OPERAND (expr, 2), uflags);
else if (code == ARRAY_REF || code == ARRAY_RANGE_REF)
{
get_expr_operands (_OPERAND (expr, 1), uflags);
--
2.31.1
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
Hi! On Wed, Aug 11, 2021 at 12:29:06PM +0100, Iain Sandoe wrote: > > On 11 Aug 2021, at 11:55, Segher Boessenkool > > wrote: > > On Tue, Aug 10, 2021 at 04:46:11PM -0600, Sandra Loosemore wrote: > >> OK. I used your wording verbatim for the first one. For the second > >> one, I'm still pretty confused as I think it is at least theoretically > >> possible on PowerPC to have a target with 64-bit long double (AIX?) that > > > > Some embedded and embedded-like subtargets use 64-bit long double by > > default. You can also configure this on any Power target (not that it > > will necessarily work ;-) ) > > > > I don't know if any subtarget with default 64-bit long double supports > > Fortran. > > I realize that this is not directly relevant to unscrambling the PPC 128bit > stuff, > but aarch64-apple-darwin2x has only 64b long double and supports gfortran. > (on both the new M1 desktop macOS and embedded iOS) Yes, but aarch64-apple-darwin2x is not an rs6000 subtarget :-) There certainly are many targets with a 64b long double. > - it is not clear to me yet if there will at some point be a transition to a > 128b >long double for at least the desktop version. Yeah. I recommend any new target (or target for which this is new) to use an IEEE QP float as long double, even if just as soft float -- the advantages are just too great. > So the permutation definitely exists for at least one non-legacy, non-embedded > platform (and gfortran is very much in demand from the new M1 users). M1 is not embedded? :-) Segher
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
Hi Folks > On 11 Aug 2021, at 11:55, Segher Boessenkool > wrote: > On Tue, Aug 10, 2021 at 04:46:11PM -0600, Sandra Loosemore wrote: >> OK. I used your wording verbatim for the first one. For the second >> one, I'm still pretty confused as I think it is at least theoretically >> possible on PowerPC to have a target with 64-bit long double (AIX?) that > > Some embedded and embedded-like subtargets use 64-bit long double by > default. You can also configure this on any Power target (not that it > will necessarily work ;-) ) > > I don't know if any subtarget with default 64-bit long double supports > Fortran. I realize that this is not directly relevant to unscrambling the PPC 128bit stuff, but aarch64-apple-darwin2x has only 64b long double and supports gfortran. (on both the new M1 desktop macOS and embedded iOS) - it is not clear to me yet if there will at some point be a transition to a 128b long double for at least the desktop version. So the permutation definitely exists for at least one non-legacy, non-embedded platform (and gfortran is very much in demand from the new M1 users). Iain >> also supports the __ibm128 format, and it would be wrong to assume that >> *any* 128-bit mode that's not long double is IEEE. > > Absolutely. Modes are not types, and types are not modes. There are > 128-bit floating point modes that are not IEEE, there are that are, and > either can be used for long double, or neither. > > > Segher
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
Hi! On Tue, Aug 10, 2021 at 04:46:11PM -0600, Sandra Loosemore wrote: > OK. I used your wording verbatim for the first one. For the second > one, I'm still pretty confused as I think it is at least theoretically > possible on PowerPC to have a target with 64-bit long double (AIX?) that Some embedded and embedded-like subtargets use 64-bit long double by default. You can also configure this on any Power target (not that it will necessarily work ;-) ) I don't know if any subtarget with default 64-bit long double supports Fortran. > also supports the __ibm128 format, and it would be wrong to assume that > *any* 128-bit mode that's not long double is IEEE. Absolutely. Modes are not types, and types are not modes. There are 128-bit floating point modes that are not IEEE, there are that are, and either can be used for long double, or neither. Segher
Re: [PATCH][v2] Adjust volatile handling of the operand scanner
> So I'm leaning towards leaving build3 alone and fixing up frontends > as issues pop up. FWIW fine with me. -- Eric Botcazou
Re: [PATCH][v2] Adjust volatile handling of the operand scanner
On Wed, 11 Aug 2021, Richard Biener wrote:
> On Tue, 10 Aug 2021, Eric Botcazou wrote:
>
> > > The question is whether we instead want to amend build3 to
> > > set TREE_THIS_VOLATILE automatically when the FIELD_DECL has
> > > it set. At least for the Fortran FE cases the gimplifier
> > > fails to see some volatile references and thus can generate
> > > wrong code which is a latent issue.
> >
> > What do we do for other similar flags, e.g. TREE_READONLY?
>
> build3 currently does no special processing for the FIELD_DECL operand,
> it just sets TREE_THIS_VOLATILE from operand zero for tcc_references.
>
> The C and C++ frontends have repeated patterns like
>
> ref = build3 (COMPONENT_REF, subtype, datum, subdatum,
> NULL_TREE);
> SET_EXPR_LOCATION (ref, loc);
> if (TREE_READONLY (subdatum)
> || (use_datum_quals && TREE_READONLY (datum)))
> TREE_READONLY (ref) = 1;
> if (TREE_THIS_VOLATILE (subdatum)
> || (use_datum_quals && TREE_THIS_VOLATILE (datum)))
> TREE_THIS_VOLATILE (ref) = 1;
>
> Leaving out TREE_READONLY shouldn't have any correctness issue. It's
> just that when adjusting the SSA operand scanner to correctly interpret
> GENERIC that this uncovers pre-existing issues in the Fortran frontend
> (one manifests in a testsuite FAIL - otherwise I wouldn't have noticed).
>
> I'm fine with just plugging the Fortran FE holes as we discover them
> but I did not check other frontends and testsuite coverage is weak.
>
> Now - I wonder if there's a reason a frontend might _not_ want to
> set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has
> TREE_THIS_VOLATILE set.
>
> I guess I'll do one more experiment and add verification that
> TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent
> and see where that trips.
It trips for
struct X { volatile int i; };
void foo ()
{
struct X x = (struct X){ .i = 0 };
}
where the gimplifier in gimplify_init_ctor_eval does
gcc_assert (TREE_CODE (purpose) == FIELD_DECL);
cref = build3 (COMPONENT_REF, TREE_TYPE (purpose),
unshare_expr (object), purpose, NULL_TREE);
producing
x.i = 0;
that is not volatile qualified. This manifests itself during the
build of libasan. I'm not sure whether the gimplifiers doing is
correct or not. Changing build3 would alter the behavior here.
Then there's a case where the COMPONENT_REF is TREE_THIS_VOLATILE
but neither the FIELD_DECL nor the base reference is. This
trips during libtsan build and again is from gimplification/folding,
this time gimplify_modify_expr_rhs doing
case INDIRECT_REF:
{
/* If we have code like
*(const A*)(A*)
where the type of "x" is a (possibly cv-qualified variant
of "A"), treat the entire expression as identical to "x".
This kind of code arises in C++ when an object is bound
to a const reference, and if "x" is a TARGET_EXPR we want
to take advantage of the optimization below. */
bool volatile_p = TREE_THIS_VOLATILE (*from_p);
tree t = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p,
0));
if (t)
{
if (TREE_THIS_VOLATILE (t) != volatile_p)
{
if (DECL_P (t))
t = build_simple_mem_ref_loc (EXPR_LOCATION
(*from_p),
build_fold_addr_expr
(t));
if (REFERENCE_CLASS_P (t))
TREE_THIS_VOLATILE (t) = volatile_p;
I suppose that's OK, it's folding volatile
*(void (*__sanitizer_sighandler_ptr) (int) *) >D.5368.handler
to act->D.5368.handler which wouldn't be volatile.
The opposite could happen, too, of course - casting away volatileness
for an access but letting that slip through verification would make
it moot. So ...
With those cases fixed bootstrap runs through and testing reveals
no additional issues apart from the already known
gfortran.dg/volatile11.f90
So I'm leaning towards leaving build3 alone and fixing up frontends
as issues pop up.
Ricahrd.
Re: [PATCH][v2] Adjust volatile handling of the operand scanner
> build3 currently does no special processing for the FIELD_DECL operand, > it just sets TREE_THIS_VOLATILE from operand zero for tcc_references. > > The C and C++ frontends have repeated patterns like > > ref = build3 (COMPONENT_REF, subtype, datum, subdatum, > NULL_TREE); > SET_EXPR_LOCATION (ref, loc); > if (TREE_READONLY (subdatum) > > || (use_datum_quals && TREE_READONLY (datum))) > > TREE_READONLY (ref) = 1; > if (TREE_THIS_VOLATILE (subdatum) > > || (use_datum_quals && TREE_THIS_VOLATILE (datum))) > > TREE_THIS_VOLATILE (ref) = 1; Likewise in the Ada front-end (gigi). > Now - I wonder if there's a reason a frontend might _not_ want to > set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has > TREE_THIS_VOLATILE set. This would be weird semantics in my opinion. > I guess I'll do one more experiment and add verification that > TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent > and see where that trips. Sounds good to me. -- Eric Botcazou
Re: [Patch v3 Fortran] Fix c_float128 and c_float128_complex on targets with 128-bit long double.
On 11.08.21 00:46, Sandra Loosemore wrote: On 8/10/21 2:29 AM, Tobias Burnus wrote: [snip] To conclude: I like the code changes (LGTM); the '__float128' -> 'TFmode' comment change also matches the code. However, I think both longer comments need to be updated. OK. I used your wording verbatim for the first one. For the second one, I'm still pretty confused as I think it is at least theoretically possible on PowerPC to have a target with 64-bit long double (AIX?) that also supports the __ibm128 format, and it would be wrong to assume that *any* 128-bit mode that's not long double is IEEE. So I decided the best thing is just to replace the FIXME with a pointer to the issue I opened yesterday https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101835 LGTM – but ... + /* See PR101835. */ ... I wonder whether your PR reference should have a TODO or FIXME prefix – or a "for some issue" suffix. Currently, it can be read as if the PR describes why the code was added – and not for questioning the code. Tobias PS: I added some more notes to the PR + extended the subject to make it easier to find. - 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] Adjust volatile handling of the operand scanner
On Tue, 10 Aug 2021, Eric Botcazou wrote: > > The question is whether we instead want to amend build3 to > > set TREE_THIS_VOLATILE automatically when the FIELD_DECL has > > it set. At least for the Fortran FE cases the gimplifier > > fails to see some volatile references and thus can generate > > wrong code which is a latent issue. > > What do we do for other similar flags, e.g. TREE_READONLY? build3 currently does no special processing for the FIELD_DECL operand, it just sets TREE_THIS_VOLATILE from operand zero for tcc_references. The C and C++ frontends have repeated patterns like ref = build3 (COMPONENT_REF, subtype, datum, subdatum, NULL_TREE); SET_EXPR_LOCATION (ref, loc); if (TREE_READONLY (subdatum) || (use_datum_quals && TREE_READONLY (datum))) TREE_READONLY (ref) = 1; if (TREE_THIS_VOLATILE (subdatum) || (use_datum_quals && TREE_THIS_VOLATILE (datum))) TREE_THIS_VOLATILE (ref) = 1; Leaving out TREE_READONLY shouldn't have any correctness issue. It's just that when adjusting the SSA operand scanner to correctly interpret GENERIC that this uncovers pre-existing issues in the Fortran frontend (one manifests in a testsuite FAIL - otherwise I wouldn't have noticed). I'm fine with just plugging the Fortran FE holes as we discover them but I did not check other frontends and testsuite coverage is weak. Now - I wonder if there's a reason a frontend might _not_ want to set TREE_THIS_VOLATILE on a COMPONENT_REF when the FIELD_DECL has TREE_THIS_VOLATILE set. I guess I'll do one more experiment and add verification that TREE_THIS_VOLATILE on COMPONENT_REFs and FIELD_DECLs is consistent and see where that trips. Richard.
