Discussion:
PR fortran/87919 patch for -fno-dec-structure
Mark Eggleston
2018-11-07 15:07:04 UTC
Permalink
Please find attached the patch and a ChangeLog entry. This is my first
patch, apologies for any mistakes in the submission process.

This patch is the simple removal of an OPT_dec_structure case from a
switch statement, it was noticeable as there was no corresponding code
for the other dec specific options. I spotted this before the creation
of PR fortran/87919 so I now know that this fixes the broken behaviour
of -fno-dec-structure.

The patch contains a change to gcc/fortran/options.c and four testcases
pr87919-dec-structure-*.f where * is one of 1, 2, 3 and 4.

The testcases are specified to compile with no specific options, -fdec,
-fdec-structure and both -fdec and -fno-dec-structure.

After building the compiler on x86_64 I got the following results
aggregated from make -j 5 check-fortran:

        === gfortran Summary ===

# of expected passes        48184
# of expected failures        103
# of unsupported tests        79
--
https://www.codethink.co.uk/privacy.html
Jakub Jelinek
2018-11-07 15:22:16 UTC
Permalink
Post by Mark Eggleston
PR fortran/87919
* options.c (gfc_handle_option): Removed case OPT_fdec_structure
as it breaks the handling of -fno-dec-structure.
No entries for the tests, i.e.
* gfortran.dg/pr87919-dec-structure-1.f: New test.
* gfortran.dg/pr87919-dec-structure-2.f: New test.
* gfortran.dg/pr87919-dec-structure-3.f: New test.
* gfortran.dg/pr87919-dec-structure-4.f: New test.
Post by Mark Eggleston
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index 73f5389361d9..3b7c2d40fe8a 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -761,10 +761,6 @@ gfc_handle_option (size_t scode, const char *arg, HOST_WIDE_INT value,
/* Enable all DEC extensions. */
set_dec_flags (1);
break;
-
- flag_dec_structure = 1;
- break;
}
Fortran_handle_option_auto (&global_options, &global_options_set,
LGTM, but I'll defer the final review to Fortran maintainers.
Post by Mark Eggleston
diff --git a/gcc/testsuite/gfortran.dg/pr87919-dec-structure-1.f b/gcc/testsuite/gfortran.dg/pr87919-dec-structure-1.f
new file mode 100644
index 000000000000..4dd34082b97a
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr87919-dec-structure-1.f
@@ -0,0 +1,21 @@
+! { dg-do compile }
+!
+! PR/fortran/87919
Without the first /, i.e.
! PR fortran/87919
(in all tests).
Post by Mark Eggleston
+++ b/gcc/testsuite/gfortran.dg/pr87919-dec-structure-2.f
@@ -0,0 +1,22 @@
+! { dg-do run }
+! { dg-options "-fdec" }
+!
+! PR/fortran/87919
+!
+! Should compile anf run with the -fdec option
s/anf/and/ (in several tests).
Post by Mark Eggleston
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr87919-dec-structure-3.f
@@ -0,0 +1,22 @@
+! { dg-do run }
+! { dg-options "-fdec-structure" }
+!
+! PR/fortran/87919
+!
+! Should compile anf run with the -fdec option
s/-fdec/-fdec-structure/ in this case.
Post by Mark Eggleston
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr87919-dec-structure-4.f
@@ -0,0 +1,22 @@
+! { dg-do compile }
+! { dg-options "-fdec -fno-dec-structure" }
+!
+! PR/fortran/87919
+!
+! Should fail to compile with the -fdec and -fno-dec-structure option
s/option/options/

I'd suggest to add another test, with
! { dg-options "-fdec-structure -fno-dec-structure" }
where the options cancel each other and the result is no DEC structure
support.

Jakub
Fritz Reese
2018-11-07 22:05:13 UTC
Permalink
Post by Jakub Jelinek
Post by Mark Eggleston
PR fortran/87919
* options.c (gfc_handle_option): Removed case OPT_fdec_structure
as it breaks the handling of -fno-dec-structure.
No entries for the tests, i.e.
* gfortran.dg/pr87919-dec-structure-1.f: New test.
* gfortran.dg/pr87919-dec-structure-2.f: New test.
* gfortran.dg/pr87919-dec-structure-3.f: New test.
* gfortran.dg/pr87919-dec-structure-4.f: New test.
Post by Mark Eggleston
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index 73f5389361d9..3b7c2d40fe8a 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -761,10 +761,6 @@ gfc_handle_option (size_t scode, const char *arg,
HOST_WIDE_INT value,
/* Enable all DEC extensions. */
set_dec_flags (1);
break;
-
- flag_dec_structure = 1;
- break;
}
Fortran_handle_option_auto (&global_options, &global_options_set,
LGTM, but I'll defer the final review to Fortran maintainers.
Thanks for the patch Mark, I concur with Jakub that it is correct for
what it does. However, I have a few comments in addition to the fixes
recommended by Jakub regarding the test cases.

First, I would prefer to name these test cases as "dec_structure_*.f"
to align with the other (23) -fdec-structure test cases. Second, the
third case (*dec-structure-3.f) is unnecessary because it is identical
in function to dec_structure_1.f90. I concur with the remaining test
cases, as well as Jakub's suggestion to cover "-fdec-structure
-fno-dec-structure" with an additional test. I would name the final
four (= 4 - 1 + 1) tests as "dec_structure_[24-27].f".


I have taken the liberty of extending this patch to cover the
remainder of PR 87919. That is, to fix -fno-* for -fno-dec,
-fno-check-array-temporaries and -fno-init-local-zero. In the extended
patch, the 'value' set for the aforementioned options is no longer
ignored, so that value=1 truly means set and value=0 truly means
"unset". Previously, the aforementioned flags effectively ignored the
value=0 condition. Similarly to the tests Mark provided with
-fdec-structure, I've provided new tests for the various facets of
-fno-dec, -fno-check-array-temporaries, and -fno-init-local-zero.

Below is the changelog. Bootstraps and regtests fine for me on
x86_64-redhat-linux. If it looks OK I'll commit to trunk (and probably
backport to 8-branch and 7-branch since the affected code appears to
be the same for those branches).


From 2d9e39bbf4a179ae433f33f4e7039b85078ba72f Mon Sep 17 00:00:00 2001
From: Fritz Reese <***@gmail.com>
Date: Wed, 7 Nov 2018 15:13:50 -0500
Subject: [PATCH] PR fortran/87919

Fix handling -fno-* prefix for init-local-zero, check-array-temporaries and dec.

gcc/fortran/
* options.c (SET_FLAG, SET_BITFLAG): New macros.
(set_dec_flags): Unset DEC flags with value==0.
(set_init_local_zero): New helper for -finit-local-zero flag group.
(gfc_init_options): Fix disabling of init flags, array temporaries
check, and dec flags when value is zero (from -fno-*).

gcc/testsuiste/
* gfortran.dg/array_temporaries_5.f90: New test.
* gfortran.dg/dec_bitwise_ops_3.f90: Ditto.
* gfortran.dg/dec_d_lines_3.f: Ditto.
* gfortran.dg/dec_exp_4.f90: Ditto.
* gfortran.dg/dec_exp_5.f90: Ditto.
* gfortran.dg/dec_io_7.f90: Ditto.
* gfortran.dg/dec_structure_24.f: Ditto.
* gfortran.dg/dec_structure_25.f: Ditto.
* gfortran.dg/dec_structure_26.f: Ditto.
* gfortran.dg/dec_structure_27.f: Ditto.
* gfortran.dg/dec_type_print_3.f90: Ditto.
* gfortran.dg/init_flag_20.f90: Ditto.
---
gcc/fortran/options.c | 70 +++++++++++++++--------
gcc/testsuite/gfortran.dg/array_temporaries_5.f90 | 20 +++++++
gcc/testsuite/gfortran.dg/dec_bitwise_ops_3.f90 | 19 ++++++
gcc/testsuite/gfortran.dg/dec_d_lines_3.f | 10 ++++
gcc/testsuite/gfortran.dg/dec_exp_4.f90 | 13 +++++
gcc/testsuite/gfortran.dg/dec_exp_5.f90 | 15 +++++
gcc/testsuite/gfortran.dg/dec_io_7.f90 | 22 +++++++
gcc/testsuite/gfortran.dg/dec_structure_24.f | 21 +++++++
gcc/testsuite/gfortran.dg/dec_structure_25.f | 22 +++++++
gcc/testsuite/gfortran.dg/dec_structure_26.f | 22 +++++++
gcc/testsuite/gfortran.dg/dec_structure_27.f | 20 +++++++
gcc/testsuite/gfortran.dg/dec_type_print_3.f90 | 29 ++++++++++
gcc/testsuite/gfortran.dg/init_flag_20.f90 | 62 ++++++++++++++++++++
13 files changed, 320 insertions(+), 25 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/array_temporaries_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_bitwise_ops_3.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_d_lines_3.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_exp_4.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_exp_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_io_7.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_24.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_25.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_26.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_27.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_type_print_3.f90
create mode 100644 gcc/testsuite/gfortran.dg/init_flag_20.f90
Jakub Jelinek
2018-11-07 22:32:30 UTC
Permalink
On Wed, Nov 07, 2018 at 05:05:13PM -0500, Fritz Reese wrote:

--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -32,6 +32,20 @@ along with GCC; see the file COPYING3. If not see

gfc_option_t gfc_option;

+#define _expand(m) m

I think it would be better to avoid names like _expand, too generic
name and starts with underscore, name it e.g. SET_BITFLAG_1 or something
similar. And it isn't mentioned in the ChangeLog.

@@ -62,14 +75,30 @@ set_dec_flags (int value)
}

What about the
/* Allow legacy code without warnings. */
gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
| GFC_STD_GNU | GFC_STD_LEGACY;
gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
that is done for value, shouldn't set_dec_flags remove those
flags again? Maybe not the allow_std ones, because those are set already by
default, perhaps just the warn_std flags?

/* Set other DEC compatibility extensions. */
- flag_dollar_ok |= value;
- flag_cray_pointer |= value;
- flag_dec_structure |= value;
- flag_dec_intrinsic_ints |= value;
- flag_dec_static |= value;
- flag_dec_math |= value;
+ SET_BITFLAG (flag_dollar_ok, value, value);
+ SET_BITFLAG (flag_cray_pointer, value, value);
+ SET_BITFLAG (flag_dec_structure, value, value);
+ SET_BITFLAG (flag_dec_intrinsic_ints, value, value);
+ SET_BITFLAG (flag_dec_static, value, value);
+ SET_BITFLAG (flag_dec_math, value, value);
}

--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
@@ -0,0 +1,20 @@
+! { dg-do run }
+! { dg-options "-fcheck-array-temporaries -fno-check-array-temporaries" }
+!
+! PR fortran/87919
+!
+! Ensure -fno-check-array-temporaries disables array temporary checking.
+! Copied from array_temporaries_2.f90.

For tests where you expect no errors and that are just copies of other
testcases, perhaps
include 'array_temporaries_2.f90'
or similar instead?

Jakub
Fritz Reese
2018-11-08 17:09:33 UTC
Permalink
Post by Mark Eggleston
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -32,6 +32,20 @@ along with GCC; see the file COPYING3. If not see
gfc_option_t gfc_option;
+#define _expand(m) m
I think it would be better to avoid names like _expand, too generic
name and starts with underscore, name it e.g. SET_BITFLAG_1 or something
similar. And it isn't mentioned in the ChangeLog.
I find "expand" a more helpful name than "set_bitflag_1" since it
describes what the macro does. However, I don't think it makes too
much of a difference so I'll follow your preference (but I'll use
SET_BITFLAG2 since then the definition line fits in 80 characters).
Post by Mark Eggleston
@@ -62,14 +75,30 @@ set_dec_flags (int value)
}
What about the
/* Allow legacy code without warnings. */
gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
| GFC_STD_GNU | GFC_STD_LEGACY;
gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
that is done for value, shouldn't set_dec_flags remove those
flags again? Maybe not the allow_std ones, because those are set already by
default, perhaps just the warn_std flags?
Sure. I wasn't convinced about this and how it might interplay with
-std= so I left it alone, but I suppose it makes sense to unsuppress
the warnings when disabling -fdec.
Post by Mark Eggleston
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
@@ -0,0 +1,20 @@
+! { dg-do run }
+! { dg-options "-fcheck-array-temporaries -fno-check-array-temporaries" }
+!
+! PR fortran/87919
+!
+! Ensure -fno-check-array-temporaries disables array temporary checking.
+! Copied from array_temporaries_2.f90.
For tests where you expect no errors and that are just copies of other
testcases, perhaps
include 'array_temporaries_2.f90'
or similar instead?
Strictly speaking it's not an exact copy because it omits the final {
dg-output } check for the runtime warning, since the warning is
supposed to occur in array_temporaries_2.f90 but not in the new case
array_temporaries_5.f90. I assumed include would propagate the {
dg-output } check -- upon actually testing this, it appears that is
not the case. I find this misleading at a glance, but it works, so I
don't mind this with an extra comment line.

Attached is a new patch which incorporates your recommendations.
Here's the diff between the two, followed by the new changelog:

diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index e703cad96fd..167621905bc 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -32,8 +32,6 @@ along with GCC; see the file COPYING3. If not see

gfc_option_t gfc_option;

-#define _expand(m) m
-
#define SET_FLAG(flag, condition, on_value, off_value) \
do \
{ \
@@ -43,8 +41,10 @@ gfc_option_t gfc_option;
flag = (off_value); \
} while (0)

+#define SET_BITFLAG2(m) m
+
#define SET_BITFLAG(flag, condition, value) \
- _expand (SET_FLAG (flag, condition, (flag | (value)), (flag & ~(value))))
+ SET_BITFLAG2 (SET_FLAG (flag, condition, (flag | (value)), (flag &
~(value))))


/* Set flags that control warnings and errors for different
@@ -66,15 +66,17 @@ set_default_std_flags (void)
static void
set_dec_flags (int value)
{
+ /* Allow legacy code without warnings.
+ Nb. We do not unset the allowed standards with value == 0 because
+ they are set by default in set_default_std_flags. */
if (value)
- {
- /* Allow legacy code without warnings. */
- gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
- | GFC_STD_GNU | GFC_STD_LEGACY;
- gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
- }
+ gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
+ | GFC_STD_GNU | GFC_STD_LEGACY;
+
+ SET_BITFLAG (gfc_option.warn_std, !value, GFC_STD_LEGACY);
+ SET_BITFLAG (gfc_option.warn_std, !value, GFC_STD_F95_DEL);

- /* Set other DEC compatibility extensions. */
+ /* Set (or unset) other DEC compatibility extensions. */
SET_BITFLAG (flag_dollar_ok, value, value);
SET_BITFLAG (flag_cray_pointer, value, value);
SET_BITFLAG (flag_dec_structure, value, value);
@@ -855,3 +871,7 @@ gfc_get_option_string (void)
}

#undef SET_BITFLAG
+#undef SET_BITFLAG2
#undef SET_FLAG
-#undef _expand

diff --git a/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
b/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
index 0070a935933..dd147ba38ed 100644
--- a/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
+++ b/gcc/testsuite/gfortran.dg/array_temporaries_5.f90
@@ -4,17 +4,7 @@
! PR fortran/87919
!
! Ensure -fno-check-array-temporaries disables array temporary checking.
-! Copied from array_temporaries_2.f90.
!

-program test
- implicit none
- integer :: a(3,3)
- call foo(a(:,1)) ! OK, no temporary created
- call foo(a(1,:)) ! BAD, temporary var created
-contains
- subroutine foo(x)
- integer :: x(3)
- x = 5
- end subroutine foo
-end program test
+! Note that 'include' drops the dg-output check from the original test case.
+include 'array_temporaries_2.f90'
Post by Mark Eggleston
ChangeLog <<<<<<
PR fortran/87919

Fix handling -fno-* prefix for init-local-zero, check-array-temporaries and dec.

gcc/fortran/
* options.c (SET_FLAG, SET_BITFLAG, SET_BITFLAG2): New macros.
(set_dec_flags): Set/unset DEC and std flags according to value.
(set_init_local_zero): New helper for -finit-local-zero flag group.
(gfc_init_options): Fix disabling of init flags, array temporaries
check, and dec flags when value is zero (from -fno-*).

gcc/testsuiste/
* gfortran.dg/array_temporaries_5.f90: New test.
* gfortran.dg/dec_bitwise_ops_3.f90: Ditto.
* gfortran.dg/dec_d_lines_3.f: Ditto.
* gfortran.dg/dec_exp_4.f90: Ditto.
* gfortran.dg/dec_exp_5.f90: Ditto.
* gfortran.dg/dec_io_7.f90: Ditto.
* gfortran.dg/dec_structure_24.f: Ditto.
* gfortran.dg/dec_structure_25.f: Ditto.
* gfortran.dg/dec_structure_26.f: Ditto.
* gfortran.dg/dec_structure_27.f: Ditto.
* gfortran.dg/dec_type_print_3.f90: Ditto.
* gfortran.dg/init_flag_20.f90: Ditto.
Jakub Jelinek
2018-11-08 17:53:57 UTC
Permalink
Post by Fritz Reese
I find "expand" a more helpful name than "set_bitflag_1" since it
describes what the macro does. However, I don't think it makes too
much of a difference so I'll follow your preference (but I'll use
SET_BITFLAG2 since then the definition line fits in 80 characters).
Ok.
Post by Fritz Reese
Post by Jakub Jelinek
What about the
/* Allow legacy code without warnings. */
gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
| GFC_STD_GNU | GFC_STD_LEGACY;
gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
that is done for value, shouldn't set_dec_flags remove those
flags again? Maybe not the allow_std ones, because those are set already by
default, perhaps just the warn_std flags?
Sure. I wasn't convinced about this and how it might interplay with
-std= so I left it alone, but I suppose it makes sense to unsuppress
the warnings when disabling -fdec.
Perhaps it might be better not to change the allow_std/warn_std flags
during the option parsing, instead set or clear say flag_dec and
only when option processing is being finalized (gfc_post_options)
check if flag_dec is set and set those. It would change behavior of
-fdec -std=f2018 and similar though. Not sure what users expect.
Post by Fritz Reese
Strictly speaking it's not an exact copy because it omits the final {
dg-output } check for the runtime warning, since the warning is
supposed to occur in array_temporaries_2.f90 but not in the new case
array_temporaries_5.f90. I assumed include would propagate the {
dg-output } check -- upon actually testing this, it appears that is
not the case. I find this misleading at a glance, but it works, so I
don't mind this with an extra comment line.
Directives are only processed in the current file, so it doesn't really
matter what the included file has as directives. One could even have the
included one be with expected dg-error lines and then include it in
the ones that don't expect any.

Anyway, that is all from me, I still don't want to stomp on Fortran
maintainer's review (use my global reviewer's rights for that) and
thus I'm deferring the review to them. When committing, please make sure
to include Mark's email in the ChangeLog next to yours to credit him.

Jakub
Fritz Reese
2018-11-12 20:28:47 UTC
Permalink
Post by Jakub Jelinek
Post by Fritz Reese
Post by Jakub Jelinek
What about the
/* Allow legacy code without warnings. */
gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
| GFC_STD_GNU | GFC_STD_LEGACY;
gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
that is done for value, shouldn't set_dec_flags remove those
flags again? Maybe not the allow_std ones, because those are set already by
default, perhaps just the warn_std flags?
Sure. I wasn't convinced about this and how it might interplay with
-std= so I left it alone, but I suppose it makes sense to unsuppress
the warnings when disabling -fdec.
Perhaps it might be better not to change the allow_std/warn_std flags
during the option parsing, instead set or clear say flag_dec and
only when option processing is being finalized (gfc_post_options)
check if flag_dec is set and set those. It would change behavior of
-fdec -std=f2018 and similar though. Not sure what users expect.
Actually, the gcc frontend appears to move -std= before the
language-specific options before f951 is even executed regardless of
its location compared to the -fdec flags. I don't know if this is a
bug or if it is by design -- the feeling I get is that the gcc
frontend processes it first since it is recognized before the flang
specific options. Therefore, greedily setting the standard options the
first time flag_dec appears means the standard information is lost and
I believe your suggestion is correct: the standard flags must be set
only once in gfc_post_options.

In fact the new testcase dec_bitwise_ops_3.f90 is a good test of this:
it uses -fdec -fno-dec -std=legacy to avoid warnings for XOR. With the
version posted previously, the -std=legacy is overwritten by -fno-dec
and warnings still appear. Here's what I'd change from the previous
patch to support this:

diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index af89a5d2faf..b7f7360215c 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -66,16 +66,6 @@ set_default_std_flags (void)
static void
set_dec_flags (int value)
{
- /* Allow legacy code without warnings.
- Nb. We do not unset the allowed standards with value == 0 because
- they are set by default in set_default_std_flags. */
- if (value)
- gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
- | GFC_STD_GNU | GFC_STD_LEGACY;
-
- SET_BITFLAG (gfc_option.warn_std, !value, GFC_STD_LEGACY);
- SET_BITFLAG (gfc_option.warn_std, !value, GFC_STD_F95_DEL);
-
/* Set (or unset) other DEC compatibility extensions. */
SET_BITFLAG (flag_dollar_ok, value, value);
SET_BITFLAG (flag_cray_pointer, value, value);
@@ -85,6 +75,24 @@ set_dec_flags (int value)
SET_BITFLAG (flag_dec_math, value, value);
}

+/* Finalize DEC flags. */
+
+static void
+post_dec_flags (int value)
+{
+ /* Don't warn for legacy code if -fdec is given; however, setting -fno-dec
+ does not force these warnings. We make one final determination on this
+ at the end because -std= is always set first; thus, we can avoid
+ clobbering the user's desired standard settings in gfc_handle_option
+ e.g. when -fdec and -fno-dec are both given. */
+ if (value)
+ {
+ gfc_option.allow_std |= GFC_STD_F95_OBS | GFC_STD_F95_DEL
+ | GFC_STD_GNU | GFC_STD_LEGACY;
+ gfc_option.warn_std &= ~(GFC_STD_LEGACY | GFC_STD_F95_DEL);
+ }
+}
+
/* Enable (or disable) -finit-local-zero. */

static void
@@ -248,6 +256,9 @@ gfc_post_options (const char **pfilename)
char *source_path;
int i;

+ /* Finalize DEC flags. */
+ post_dec_flags (flag_dec);
+
/* Excess precision other than "fast" requires front-end
support. */
if (flag_excess_precision_cmdline == EXCESS_PRECISION_STANDARD)
@@
Post by Jakub Jelinek
Directives are only processed in the current file, so it doesn't really
matter what the included file has as directives. One could even have the
included one be with expected dg-error lines and then include it in
the ones that don't expect any.
Good to know, thanks! In that case, I like your suggestion of reducing
the test cases to includes. See new the newly attached patch for
updated cases.
Post by Jakub Jelinek
Anyway, that is all from me, I still don't want to stomp on Fortran
maintainer's review (use my global reviewer's rights for that) and
thus I'm deferring the review to them. When committing, please make sure
to include Mark's email in the ChangeLog next to yours to credit him.
Thanks for your comments. I think nobody will feel stomped on since
maintainers are sparse and busy. I will certainly make note of Mark's
contributions when committing.

Attached is the latest version, which builds and regtests cleanly on
x86_64-redhat-linux. OK for trunk, 7-branch, and 8-branch?

Fritz

From 1cae11a88b29fe521e0e6c6c7c1796a7adb34cad Mon Sep 17 00:00:00 2001
From: Fritz Reese <***@gmail.com>
Date: Mon, 12 Nov 2018 13:57:25 -0500
Subject: [PATCH] PR fortran/87919

Fix handling -fno-* prefix for init-local-zero, check-array-temporaries and dec.

gcc/fortran/
* options.c (SET_FLAG, SET_BITFLAG, SET_BITFLAG2): New macros.
(set_dec_flags): Set/unset DEC and std flags according to value.
(set_init_local_zero): New helper for -finit-local-zero flag group.
(gfc_init_options): Fix disabling of init flags, array temporaries
check, and dec flags when value is zero (from -fno-*).

gcc/testsuiste/
* gfortran.dg/array_temporaries_5.f90: New test.
* gfortran.dg/dec_bitwise_ops_3.f90: Ditto.
* gfortran.dg/dec_d_lines_3.f: Ditto.
* gfortran.dg/dec_exp_4.f90: Ditto.
* gfortran.dg/dec_exp_5.f90: Ditto.
* gfortran.dg/dec_io_7.f90: Ditto.
* gfortran.dg/dec_structure_24.f90: Ditto.
* gfortran.dg/dec_structure_25.f90: Ditto.
* gfortran.dg/dec_structure_26.f90: Ditto.
* gfortran.dg/dec_structure_27.f90: Ditto.
* gfortran.dg/dec_type_print_3.f90: Ditto.
* gfortran.dg/init_flag_20.f90: Ditto.
---
gcc/fortran/options.c | 93 +++++++++++++++--------
gcc/testsuite/gfortran.dg/array_temporaries_5.f90 | 10 +++
gcc/testsuite/gfortran.dg/dec_bitwise_ops_3.f90 | 29 +++++++
gcc/testsuite/gfortran.dg/dec_d_lines_3.f | 14 ++++
gcc/testsuite/gfortran.dg/dec_exp_4.f90 | 12 +++
gcc/testsuite/gfortran.dg/dec_exp_5.f90 | 11 +++
gcc/testsuite/gfortran.dg/dec_io_7.f90 | 20 +++++
gcc/testsuite/gfortran.dg/dec_structure_24.f90 | 32 ++++++++
gcc/testsuite/gfortran.dg/dec_structure_25.f90 | 11 +++
gcc/testsuite/gfortran.dg/dec_structure_26.f90 | 34 +++++++++
gcc/testsuite/gfortran.dg/dec_structure_27.f90 | 34 +++++++++
gcc/testsuite/gfortran.dg/dec_type_print_3.f90 | 21 +++++
gcc/testsuite/gfortran.dg/init_flag_20.f90 | 15 ++++
13 files changed, 306 insertions(+), 30 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/array_temporaries_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_bitwise_ops_3.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_d_lines_3.f
create mode 100644 gcc/testsuite/gfortran.dg/dec_exp_4.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_exp_5.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_io_7.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_24.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_25.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_26.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_27.f90
create mode 100644 gcc/testsuite/gfortran.dg/dec_type_print_3.f90
create mode 100644 gcc/testsuite/gfortran.dg/init_flag_20.f90
Jakub Jelinek
2018-11-12 20:42:09 UTC
Permalink
Post by Fritz Reese
Actually, the gcc frontend appears to move -std= before the
language-specific options before f951 is even executed regardless of
its location compared to the -fdec flags. I don't know if this is a
That is because:
#define F951_OPTIONS "%(cc1_options) %{J*} \
%{!nostdinc:-fintrinsic-modules-path finclude%s}\
%{!fsyntax-only:%(invoke_as)}"
and
static const char *cc1_options =
"%{pg:%{fomit-frame-pointer:%e-pg and -fomit-frame-pointer are incompatible}}\
%{!iplugindir*:%{fplugin*:%:find-plugindir()}}\
%1 %{!Q:-quiet} %{!dumpbase:-dumpbase %B} %{d*} %{m*} %{aux-info*}\
%{fcompare-debug-second:%:compare-debug-auxbase-opt(%b)} \
%{!fcompare-debug-second:%{c|S:%{o*:-auxbase-strip %*}%{!o*:-auxbase %b}}}%{!c:%{!S:-auxbase %b}} \
%{g*} %{O*} %{W*&pedantic*} %{w} %{std*&ansi&trigraphs}\
%{v:-version} %{pg:-p} %{p} %{f*} %{undef}\

where %{std*&ansi&trigraphs} comes before %{f*}.
I guess let's not change that behavior.
Post by Fritz Reese
bug or if it is by design -- the feeling I get is that the gcc
frontend processes it first since it is recognized before the flang
specific options. Therefore, greedily setting the standard options the
first time flag_dec appears means the standard information is lost and
I believe your suggestion is correct: the standard flags must be set
only once in gfc_post_options.
it uses -fdec -fno-dec -std=legacy to avoid warnings for XOR. With the
version posted previously, the -std=legacy is overwritten by -fno-dec
and warnings still appear. Here's what I'd change from the previous
LGTM.
Post by Fritz Reese
Post by Jakub Jelinek
Anyway, that is all from me, I still don't want to stomp on Fortran
maintainer's review (use my global reviewer's rights for that) and
thus I'm deferring the review to them. When committing, please make sure
to include Mark's email in the ChangeLog next to yours to credit him.
Thanks for your comments. I think nobody will feel stomped on since
maintainers are sparse and busy. I will certainly make note of Mark's
contributions when committing.
Ok, so I'll ack it for trunk now, but please give the other Fortran
maintainers one day to disagree before committing.
For the release branches, I'd wait two weeks or so before backporting it.

Thanks.

Jakub
Fritz Reese
2018-11-12 20:52:42 UTC
Permalink
Post by Jakub Jelinek
Ok, so I'll ack it for trunk now, but please give the other Fortran
maintainers one day to disagree before committing.
For the release branches, I'd wait two weeks or so before backporting it.
Roger that. I'll happily give it some time. Thanks for looking it over.

Fritz
Jakub Jelinek
2018-11-22 11:44:59 UTC
Permalink
Post by Fritz Reese
Post by Jakub Jelinek
Ok, so I'll ack it for trunk now, but please give the other Fortran
maintainers one day to disagree before committing.
For the release branches, I'd wait two weeks or so before backporting it.
Roger that. I'll happily give it some time. Thanks for looking it over.
I think more than enough time passed, do you plan to commit to trunk now?
Note, small adjustment will be needed for the addition of flag_dec_include
in set_dec_flags.

Jakub
Mark Eggleston
2018-12-03 15:26:28 UTC
Permalink
Post by Fritz Reese
Attached is the latest version, which builds and regtests cleanly on
x86_64-redhat-linux. OK for trunk, 7-branch, and 8-branch?
I'm currently using this patch (pending it being committed) and have
some pending patches that use it.

I can use the old method of setting dec options for the time being and
PR fortran/87919 will have to be modified accordingly.

I note that in gcc/fortran/options the comment preceding
set_default_std_flags it says:

"Keep in sync with libgfortran/runtime/compile_options.c
(init_compile_options)."

and in libgfortran/runtime/compile_options.c preceding
init_compile_options it says:

"Keep in sync with gcc/fortran/options.c (gfc_init_options)."

I think this should have (set_default_std_flags) instead of
(gfc_init_options) and they are not in sync.

Mark
Post by Fritz Reese
Fritz
--
https://www.codethink.co.uk/privacy.html
Fritz Reese
2018-12-03 15:52:18 UTC
Permalink
Post by Jakub Jelinek
I think more than enough time passed, do you plan to commit to trunk now?
Note, small adjustment will be needed for the addition of flag_dec_include
in set_dec_flags.
Jakub- Sorry, yes. I've had other priorities the past few weeks here,
but I just committed r266745 adjusted for -fdec-include.
Post by Jakub Jelinek
I'm currently using this patch (pending it being committed) and have
some pending patches that use it.
I can use the old method of setting dec options for the time being and
PR fortran/87919 will have to be modified accordingly.
Sorry again for the delay. It is committed now, so you may use the
new-style option setting. Thanks in advance for your upcoming patches.
Post by Jakub Jelinek
I note that in gcc/fortran/options the comment preceding
"Keep in sync with libgfortran/runtime/compile_options.c
(init_compile_options)."
and in libgfortran/runtime/compile_options.c preceding
"Keep in sync with gcc/fortran/options.c (gfc_init_options)."
I think this should have (set_default_std_flags) instead of
(gfc_init_options) and they are not in sync.
The first comment (above set_default_std_flags) is also present before
gfc_init_options, which is technically the routine that should mirror
init_compile_options. Since set_default_std_flags is a subroutine of
gfc_init_options, it should also sync with the std flags setting in
init_compile_options, so I don't think any of the comments are
incorrect. However the symmetry may be more obvious if the std flags
setting from init_compile_options was in a subroutine (named similarly
to set_default_std_flags), then the comments on the default standard
flags could reference each other.

That being said, it does appear the default standard flags are out of
sync, as you mentioned... Not sure if this is worth a PR but it's
certainly worth fixing. Seems it was introduced when f2015 was renamed
to f2018 in r255761.


Fritz
Jakub Jelinek
2018-12-03 15:58:51 UTC
Permalink
Post by Fritz Reese
Post by Jakub Jelinek
I think more than enough time passed, do you plan to commit to trunk now?
Note, small adjustment will be needed for the addition of flag_dec_include
in set_dec_flags.
Jakub- Sorry, yes. I've had other priorities the past few weeks here,
Np.
Post by Fritz Reese
but I just committed r266745 adjusted for -fdec-include.
Thanks, though it seems what you've committed in options.c is incomplete.
In the patch you've posted earlier, you've changed also
gfc_init_options and gfc_handle_option, to call the new functions
(set_init_local_zero, post_dec_options) and use SET_FLAG and undefine it at
the end.
None of those changes appears in
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/fortran/options.c?limit_changes=0&r1=266745&r2=266744&pathrev=266745
all changes stop at the newly added set_init_local_zero, so
set_init_local_zero and post_dec_options are now unused functions.

Jakub
Jakub Jelinek
2018-12-03 17:12:11 UTC
Permalink
Post by Jakub Jelinek
Post by Fritz Reese
but I just committed r266745 adjusted for -fdec-include.
Thanks, though it seems what you've committed in options.c is incomplete.
In the patch you've posted earlier, you've changed also
gfc_init_options and gfc_handle_option, to call the new functions
(set_init_local_zero, post_dec_options) and use SET_FLAG and undefine it at
the end.
None of those changes appears in
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/fortran/options.c?limit_changes=0&r1=266745&r2=266744&pathrev=266745
all changes stop at the newly added set_init_local_zero, so
set_init_local_zero and post_dec_options are now unused functions.
I've committed the remaining options.c changes in r266761 now
to unbreak the FAILs.

Jakub
Fritz Reese
2018-12-03 17:39:37 UTC
Permalink
Post by Jakub Jelinek
Post by Jakub Jelinek
Post by Fritz Reese
but I just committed r266745 adjusted for -fdec-include.
Thanks, though it seems what you've committed in options.c is incomplete.
In the patch you've posted earlier, you've changed also
gfc_init_options and gfc_handle_option, to call the new functions
(set_init_local_zero, post_dec_options) and use SET_FLAG and undefine it at
the end.
None of those changes appears in
https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/fortran/options.c?limit_changes=0&r1=266745&r2=266744&pathrev=266745
all changes stop at the newly added set_init_local_zero, so
set_init_local_zero and post_dec_options are now unused functions.
I've committed the remaining options.c changes in r266761 now
to unbreak the FAILs.
Jakub
Thanks, and sorry...

Loading...