[Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Message ID 5757e425-1980-f148-b870-6d93cfc41916@codethink.co.uk
State New
Headers show
Series
  • [Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval
Related show

Commit Message

Mark Eggleston Sept. 26, 2019, 8:45 a.m.
Original thread starts here 
https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

OK to commit?

gcc/fortran/ChangeLog

     Mark Eggleston  <mark.eggleston@codethink.com>

     * invoke.texi: Add -Wno-overwrite-recursive to list of options. Add
     description of -Wno-overwrite-recursive. Fix typo in description
     of -Winteger-division.
     * lang.opt: Add option -Woverwrite-recursive initialised as on.
     * option.c (gfc_post_options): Output warning only if it is enabled.

gcc/testsuite/ChangeLog

     Mark Eggleston <mark.eggleston@codethink.com>

     * gfortran.dg/no_overwrite_recursive_1.f90: New test.
     * gfortran.dg/no_overwrite_recursive_2.f90: New test.

-- 
https://www.codethink.co.uk/privacy.html

Comments

Steve Kargl Sept. 28, 2019, 4:50 p.m. | #1
On Thu, Sep 26, 2019 at 09:45:28AM +0100, Mark Eggleston wrote:
> Original thread starts here 

> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

> 

> OK to commit?

> 


I'm not a big fan of option proliferation.  If you don't 
want to see warns just use -w.  Of course, this is just
MHO.

-- 
Steve
Mark Eggleston Oct. 2, 2019, 8 a.m. | #2
On 28/09/2019 17:50, Steve Kargl wrote:
> On Thu, Sep 26, 2019 at 09:45:28AM +0100, Mark Eggleston wrote:

>> Original thread starts here

>> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

>>

>> OK to commit?

>>

> I'm not a big fan of option proliferation.  If you don't

> want to see warns just use -w.  Of course, this is just

> MHO.

Unfortunately -w switches off ALL warnings.

-- 
https://www.codethink.co.uk/privacy.html
Mark Eggleston Oct. 21, 2019, 1:37 p.m. | #3
PING - OK to commit?

On 02/10/2019 09:00, Mark Eggleston wrote:
>

> On 28/09/2019 17:50, Steve Kargl wrote:

>> On Thu, Sep 26, 2019 at 09:45:28AM +0100, Mark Eggleston wrote:

>>> Original thread starts here

>>> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

>>>

>>> OK to commit?

>>>

>> I'm not a big fan of option proliferation.  If you don't

>> want to see warns just use -w.  Of course, this is just

>> MHO.

> Unfortunately -w switches off ALL warnings.

>

-- 
https://www.codethink.co.uk/privacy.html
Tobias Burnus Oct. 25, 2019, 8:35 a.m. | #4
On 9/26/19 10:45 AM, Mark Eggleston wrote:
> Original thread starts here 

> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

> OK to commit?


As Steve, I am not really happy about adding yet another option and 
especially not about legacy features. On the other hand, I see that 
legacy code is still used.

Having said this, the patch is OK from my side.

Tobias

PS: I was also not that happy about the BOZ changes by Steve, which 
broke code here – but, fortunately, adding int( ,kind=) around it was 
sufficient and that code was supposed to be F2003 standard conforming.  
I could ping the authors and is now fixed. Still, I wonder how much code 
broke due to that change; code is not that simple to fix. – But, in 
general, I am very much in favour in having valid Fortran 2018 code (can 
be fixed form, old and use old features, that's fine).


> gcc/fortran/ChangeLog

>

>     Mark Eggleston  <mark.eggleston@codethink.com>

>

>     * invoke.texi: Add -Wno-overwrite-recursive to list of options. Add

>     description of -Wno-overwrite-recursive. Fix typo in description

>     of -Winteger-division.

>     * lang.opt: Add option -Woverwrite-recursive initialised as on.

>     * option.c (gfc_post_options): Output warning only if it is enabled.

>

> gcc/testsuite/ChangeLog

>

>     Mark Eggleston <mark.eggleston@codethink.com>

>

>     * gfortran.dg/no_overwrite_recursive_1.f90: New test.

>     * gfortran.dg/no_overwrite_recursive_2.f90: New test.

>
Jeff Law Oct. 25, 2019, 1:22 p.m. | #5
On 10/25/19 2:35 AM, Tobias Burnus wrote:
> On 9/26/19 10:45 AM, Mark Eggleston wrote:

>> Original thread starts here

>> https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

>> OK to commit?

> 

> As Steve, I am not really happy about adding yet another option and

> especially not about legacy features. On the other hand, I see that

> legacy code is still used.

> 

> Having said this, the patch is OK from my side.

> 

> Tobias

> 

> PS: I was also not that happy about the BOZ changes by Steve, which

> broke code here – but, fortunately, adding int( ,kind=) around it was

> sufficient and that code was supposed to be F2003 standard conforming. 

> I could ping the authors and is now fixed. Still, I wonder how much code

> broke due to that change; code is not that simple to fix. – But, in

> general, I am very much in favour in having valid Fortran 2018 code (can

> be fixed form, old and use old features, that's fine).

So across Fedora the BOZ stuff tripped 2-3 packages.  In comparison the
function argument stuff broke 30-40 packages, many of which still don't
build without -fallow-argument-mismatch.

jeff
Tobias Burnus Oct. 25, 2019, 1:54 p.m. | #6
Hi Jeff,

On 10/25/19 3:22 PM, Jeff Law wrote:
> So across Fedora the BOZ stuff tripped 2-3 packages. In comparison the 

> function argument stuff broke 30-40 packages, many of which still 

> don't build without -fallow-argument-mismatch.


Regarding the latter:
The initial patch was too strict – an also rejected valid code 
(according to the Fortran 2018 standard). That has been fixed.* – Thus, 
either some valid cases were missed (gfortran bug) or all those packages 
indeed have an argument mismatch.

*That fix is: 2019-10-14 / r276972 / PR fortran/92004 / 
https://gcc.gnu.org/ml/fortran/2019-10/msg00128.html

Do you know whether those 30–40 packages have code bugs or could there 
be gfortran bugs (too strict checking) lurking?


Regarding the BOZ: One difference to the argument mismatch is that the 
latter has an option to still accept it (-fallow-argument-mismatch) and 
potentially generates wrong code – depending what the ME does with the 
mismatches – while the former, once parsed, causes no potential ME 
problems and as there is no flag, it always requires code changes. (On 
the other hand, fixing the BOZ issue is straight forward; argument 
changes are trickier.)

Cheers,

Tobias
Steve Kargl Oct. 25, 2019, 2:17 p.m. | #7
On Fri, Oct 25, 2019 at 10:35:24AM +0200, Tobias Burnus wrote:
> On 9/26/19 10:45 AM, Mark Eggleston wrote:

> 

> PS: I was also not that happy about the BOZ changes by Steve, which 

> broke code here – but, fortunately, adding int( ,kind=) around it was 

> sufficient and that code was supposed to be F2003 standard conforming.  

> I could ping the authors and is now fixed. Still, I wonder how much code 

> broke due to that change; code is not that simple to fix. – But, in 

> general, I am very much in favour in having valid Fortran 2018 code (can 

> be fixed form, old and use old features, that's fine).

> 


My BOZ patch brought gfortran closer to an actual comforming
Fortran compiler while providing an option that would allow
quite a few documented and undocumented extensions.  If the
patch broke some of your code, and -fallow-invalid-boz did not
allow the code to compile, and you were forced to use INT(, kind=)
to get it to compile, then, no, the code was not conforming.

And since you mention F2003, for the record

C410  (R411) A boz-literal-constant shall appear only as a
      data-stmt-constant in a DATA statement, as the actual
      argument associated with the dummy argument A of the
      numeric intrinsic functions DBLE, REAL or INT, or as
      the actual argument associated with the X or Y dummy
      argument of the intrinsic function CMPLX.

-- 
Steve
Jeff Law Oct. 25, 2019, 2:48 p.m. | #8
On 10/25/19 7:54 AM, Tobias Burnus wrote:
> Hi Jeff,

> 

> On 10/25/19 3:22 PM, Jeff Law wrote:

>> So across Fedora the BOZ stuff tripped 2-3 packages. In comparison the

>> function argument stuff broke 30-40 packages, many of which still

>> don't build without -fallow-argument-mismatch.

> 

> Regarding the latter:

> The initial patch was too strict – an also rejected valid code

> (according to the Fortran 2018 standard).

That was my understanding from loosely following the threads.


 That has been fixed.* – Thus,
> either some valid cases were missed (gfortran bug) or all those packages

> indeed have an argument mismatch.

> 

> *That fix is: 2019-10-14 / r276972 / PR fortran/92004 /

> https://gcc.gnu.org/ml/fortran/2019-10/msg00128.html

Yea.  That patch certainly helped lapack and others.

> 

> Do you know whether those 30–40 packages have code bugs or could there

> be gfortran bugs (too strict checking) lurking?

I'm not familiar enough with the issue & packages to know if they're
cases of source bugs or gfortran being too strict.

My plan has always been to extract a few cases and pass them along for
that kind of analysis.  I've just been too busy lately with other
regressions :(

A partial list of the affected packages:


R-deldir
R
atlas
cgnslib
cp2k
elk
elpa
exciting
ga
getdata
grib_api
hdf
libccp4
mpich
hwchem
psblas3
qrmumps
qrupdate
quantum-espresso
scalapack
scipy
scorep
wannier90
wsjtx
xfoil
xrotor

There's certainly more, that list just represents those I've locally
worked around with -fallow-argument-mismatch.  Several more trigger the
mismatch error, but I haven't bothered working around yet.

That list comes from _after_ the  Oct 14 patch to correct issues in the
argument mismatch testing.

> 

> 

> Regarding the BOZ: One difference to the argument mismatch is that the

> latter has an option to still accept it (-fallow-argument-mismatch) and

> potentially generates wrong code – depending what the ME does with the

> mismatches – while the former, once parsed, causes no potential ME

> problems and as there is no flag, it always requires code changes. (On

> the other hand, fixing the BOZ issue is straight forward; argument

> changes are trickier.)

Absolutely.  That's the primary reason why I haven't contacted the
affected package maintainers yet -- I don't want them blindly adding
-fallow-argument-mismatch to their flags.

Jeff
Tobias Burnus Oct. 25, 2019, 2:53 p.m. | #9
Hi Steve,

On 10/25/19 4:17 PM, Steve Kargl wrote:
> My BOZ patch brought gfortran closer to an actual comforming Fortran 

> compiler while providing an option that would allow quite a few 

> documented and undocumented extensions. If the patch broke some of 

> your code, and -fallow-invalid-boz did not allow the code to compile

Actually, it does allow it to compile – but I missed the option; 
-std=legacy used to be sufficient.

For the code at hand, it was actually better to nag the author to change 
the code to valid Fortran than to fiddle around with some options.

But I think that point to a problem: Some compiler flags do exist, but 
it is are hard to find them (especially, if one does not know that they 
do exist). … I am not claiming that it should be done in this case, but 
sometimes something like:

"Error: BOZ literal constant at (1) is neither a DATA statement value 
nor an actual argument of INT/REAL/DBLE/CMPLX intrinsic subprogram; _for 
legacy code, -fallow-invalid-boz might be an option"_

would have surely helped.**

Side note: to me, "the … intrinsic function" is clearer than "… 
intrinsic subprogram".


> and you were forced to use INT(, kind=) to get it to compile


Well, using INT() was the way in order to get the code standard 
conforming :-)

As you you quoted:

> C410  (R411) A boz-literal-constant shall appear only as a

>        data-stmt-constant in a DATA statement, as the actual

>        argument associated with the dummy argument A of the

>        numeric intrinsic functions DBLE, REAL or INT, or as

>        the actual argument associated with the X or Y dummy

>        argument of the intrinsic function CMPLX.


Cheers,

Tobias

**This does not mean that users will always read this. (I get this 
error, what shall I do?) Nor that fixing the code would be usually be 
the better option. (As in my case.)
Tobias Burnus Oct. 25, 2019, 5:57 p.m. | #10
Hi Jeff, hi Thomas, hi all,

I had a look at Wannier90. (Fedora uses Version 2.0.0 of 2013, 2.0.1 was 
released 2015; current is 3.0 of Feb 2019 and does build.) I think that 
problem in Wannier90 it typical for all failing code, although there are 
likely a few other failures. Namely,

That code uses something like:
   subroutine sub(array, n)
      integer array(*)
      integer N
   end subroutine

with the call:
     integer :: scalar
     call sub(scalar, 1)

That fails with:
Error: Rank mismatch in argument ‘array’ at (1) (rank-1 and scalar)

That's a scalar actual argument is passed to an array dummy argument.


Technically, the error is correct and the code invalid:

F2018, 15.5.2.4, paragraph 14 has:
"If the actual argument is a … scalar, the corresponding dummy
argument shall be scalar unless … the actual argument …
is an element … of an array."

Fortran 66, https://wg5-fortran.org/ARCHIVE/Fortran66.pdf , [8.4.2, ll. 33-35]
"If a dummy argument of an external function is an array name,
the corresponding actual argument must be an array name
or array element name (10.1.3)"

Fortran IV, p. 104,  http://web.eah-jena.de/~kleine/history/languages/GC28-6515-10-FORTRAN-IV-Language.pdf
"If a dummy argument is an array, the corresponding actual argument
must be (1) an array, or (2) an array element."


[Fortran IV only permitted literal constants in
'dimension(123)' and 'dimension(N)' for dummy arguments,
i.e. the size is variable and provided by the callee.
[Number of ranks not specified, the linked specification
supports rank-7 arrays.]
Fortran 66 seems to be likewise, and specifies only rank-4 arrays.
Fortran 77 has finally assumed-size arrays (which seem to be
backwards, 'dimension(N)' I regard as better than 'dimension(*)'),
supports other lower bounds than 1, has rank-7 arrays and a few
additional array extensions to 66.]


Current result:
* Error with -std=gnu (the default) or -std=f95/f2003/f…
* Downgraded to a warning with either -std=legacy or -fallow-argument-mismatch


However, passing a scalar instead of an array/array
element worked/works with (nearly?) all compilers. Hence, passing
a scalar is seemingly common pattern. Thus, I wonder whether we
should do something about this.

Ideas/possibilities:
* Use warning instead of an error with -std=gnu (and only error
   for std=f…)
* Keep the error for most mismatches but downgrade to a warning
   if -std=gnu for (only) the scalar-to-array case.
   (a) do this always or (b) only for assumes-size dummy args.
* (a) Keep the status quo and require -std=legacy/
-fallow-argument-mismatch to get it compiled.
   (b) modify the error message such that the user knows about
   the arg-mismatch flag?
* Do something else?

Ideas? Comments? Opinions?
   

Cheers,

Tobias


On 10/25/19 4:48 PM, Jeff Law wrote:
>>> […]the function argument stuff broke 30-40 packages, many of which still

>>> don't build without -fallow-argument-mismatch.


>> Do you know whether those 30–40 packages have code bugs or could there

>> be gfortran bugs (too strict checking) lurking?

> I'm not familiar enough with the issue & packages to know if they're

> cases of source bugs or gfortran being too strict.

>

> My plan has always been to extract a few cases and pass them along for

> that kind of analysis.  I've just been too busy lately with other

> regressions :(

>

> A partial list of the affected packages:

>

>

> R-deldir

> R

> atlas

> cgnslib

> cp2k

> elk

> elpa

> exciting

> ga

> getdata

> grib_api

> hdf

> libccp4

> mpich

> hwchem

> psblas3

> qrmumps

> qrupdate

> quantum-espresso

> scalapack

> scipy

> scorep

> wannier90

> wsjtx

> xfoil

> xrotor

>

> There's certainly more, that list just represents those I've locally

> worked around with -fallow-argument-mismatch.  Several more trigger the

> mismatch error, but I haven't bothered working around yet.

>

> That list comes from _after_ the  Oct 14 patch to correct issues in the

> argument mismatch testing.

>

>>

>> Regarding the BOZ: One difference to the argument mismatch is that the

>> latter has an option to still accept it (-fallow-argument-mismatch) and

>> potentially generates wrong code – depending what the ME does with the

>> mismatches – while the former, once parsed, causes no potential ME

>> problems and as there is no flag, it always requires code changes. (On

>> the other hand, fixing the BOZ issue is straight forward; argument

>> changes are trickier.)

> Absolutely.  That's the primary reason why I haven't contacted the

> affected package maintainers yet -- I don't want them blindly adding

> -fallow-argument-mismatch to their flags.

>

> Jeff

>
Thomas Koenig Oct. 25, 2019, 7:03 p.m. | #11
> However, passing a scalar instead of an array/array

> element worked/works with (nearly?) all compilers. Hence, passing

> a scalar is seemingly common pattern. Thus, I wonder whether we

> should do something about this.


Maybe we could mention -fallow-argument-mismatch in the error message.

(Really, the changes needed are quite trivial, and the code is in fact
invalid.  I actually wrote the patch for LAPACK which removed all the
fallout from their TESTING routines; it didn't take long).

Regards

	Thomas

Patch

From 30d7915ce44629edc85dba210cca9007d1c70d02 Mon Sep 17 00:00:00 2001
From: Mark Eggleston <markeggleston@codethink.com>
Date: Tue, 16 Apr 2019 09:09:12 +0100
Subject: [PATCH] Suppress warning with -Wno-overwrite-recursive

The message "Warning: Flag '-fno-automatic' overwrites '-frecursive'" is
output by default when -fno-automatic and -frecursive are used together.
It warns that recursion may be broken, however if all the relavent variables
in the recursive procedure have automatic attributes the warning is
unnecessary so -Wno-overwrite-recursive can be used to suppress it. This
will allow compilation when warnings are regarded as errors.

Suppress warning with -Wno-overwrite-recursive
---
 gcc/fortran/invoke.texi                              | 20 +++++++++++++++-----
 gcc/fortran/lang.opt                                 |  4 ++++
 gcc/fortran/options.c                                |  3 ++-
 .../gfortran.dg/no_overwrite_recursive_1.f90         | 11 +++++++++++
 .../gfortran.dg/no_overwrite_recursive_2.f90         | 10 ++++++++++
 5 files changed, 42 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/no_overwrite_recursive_1.f90
 create mode 100644 gcc/testsuite/gfortran.dg/no_overwrite_recursive_2.f90

diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi
index b911ac90018..0177fb400a1 100644
--- a/gcc/fortran/invoke.texi
+++ b/gcc/fortran/invoke.texi
@@ -149,10 +149,11 @@  and warnings}.
 -Wc-binding-type -Wcharacter-truncation -Wconversion @gol
 -Wdo-subscript -Wfunction-elimination -Wimplicit-interface @gol
 -Wimplicit-procedure -Wintrinsic-shadow -Wuse-without-only @gol
--Wintrinsics-std -Wline-truncation -Wno-align-commons -Wno-tabs @gol
--Wreal-q-constant -Wsurprising -Wunderflow -Wunused-parameter @gol
--Wrealloc-lhs -Wrealloc-lhs-all -Wfrontend-loop-interchange @gol
--Wtarget-lifetime -fmax-errors=@var{n} -fsyntax-only -pedantic @gol
+-Wintrinsics-std -Wline-truncation -Wno-align-commons @gol
+-Wno-overwrite-recursive -Wno-tabs -Wreal-q-constant -Wsurprising @gol
+-Wunderflow -Wunused-parameter -Wrealloc-lhs -Wrealloc-lhs-all @gol
+-Wfrontend-loop-interchange -Wtarget-lifetime -fmax-errors=@var{n} @gol
+-fsyntax-only -pedantic @gol
 -pedantic-errors @gol
 }
 
@@ -995,7 +996,7 @@  nor has been declared as @code{EXTERNAL}.
 @opindex @code{Winteger-division}
 @cindex warnings, integer division
 @cindex warnings, division of integers
-Warn if a constant integer division truncates it result.
+Warn if a constant integer division truncates its result.
 As an example, 3/5 evaluates to 0.
 
 @item -Wintrinsics-std
@@ -1008,6 +1009,15 @@  it as @code{EXTERNAL} procedure because of this.  @option{-fall-intrinsics} can
 be used to never trigger this behavior and always link to the intrinsic
 regardless of the selected standard.
 
+@item -Wno-overwrite-recursive
+@opindex @code{Woverwrite-recursive}
+@cindex  warnings, overwrite recursive
+Do not warn when @option{-fno-automatic} is used with @option{-frecursive}. Recursion
+will be broken if the relevant local variables do not have the attribute
+@code{AUTOMATIC} explicitly declared. This option can be used to suppress the warning
+when it is known that recursion is not broken. Useful for build environments that use
+@option{-Werror}.
+
 @item -Wreal-q-constant
 @opindex @code{Wreal-q-constant}
 @cindex warnings, @code{q} exponent-letter
diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt
index b8bf7fe7508..1b296c92d19 100644
--- a/gcc/fortran/lang.opt
+++ b/gcc/fortran/lang.opt
@@ -293,6 +293,10 @@  Wopenmp-simd
 Fortran
 ; Documented in C
 
+Woverwrite-recursive
+Fortran Warning Var(warn_overwrite_recursive) Init(1)
+Warn that -fno-automatic may break recursion.
+
 Wpedantic
 Fortran
 ; Documented in common.opt
diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c
index a0f95fa8832..47042a25eab 100644
--- a/gcc/fortran/options.c
+++ b/gcc/fortran/options.c
@@ -419,7 +419,8 @@  gfc_post_options (const char **pfilename)
     gfc_warning_now (0, "Flag %<-fno-automatic%> overwrites %<-fmax-stack-var-size=%d%>",
 		     flag_max_stack_var_size);
   else if (!flag_automatic && flag_recursive)
-    gfc_warning_now (0, "Flag %<-fno-automatic%> overwrites %<-frecursive%>");
+    gfc_warning_now (OPT_Woverwrite_recursive, "Flag %<-fno-automatic%> "
+		     "overwrites %<-frecursive%>");
   else if (!flag_automatic && flag_openmp)
     gfc_warning_now (0, "Flag %<-fno-automatic%> overwrites %<-frecursive%> implied by "
 		     "%<-fopenmp%>");
diff --git a/gcc/testsuite/gfortran.dg/no_overwrite_recursive_1.f90 b/gcc/testsuite/gfortran.dg/no_overwrite_recursive_1.f90
new file mode 100644
index 00000000000..f12c4b8a4c7
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/no_overwrite_recursive_1.f90
@@ -0,0 +1,11 @@ 
+! { dg-do compile }
+! { dg-options "-fno-automatic -frecursive" }
+!
+! Test case contributed by Mark Eggleston  <mark.eggleston@codethink.com>
+!
+
+program test
+  ! do nothing
+end program
+
+! { dg-warning "Flag '-fno-automatic' overwrites '-frecursive'" "warning" { target *-*-* } 0 } 
diff --git a/gcc/testsuite/gfortran.dg/no_overwrite_recursive_2.f90 b/gcc/testsuite/gfortran.dg/no_overwrite_recursive_2.f90
new file mode 100644
index 00000000000..09445375481
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/no_overwrite_recursive_2.f90
@@ -0,0 +1,10 @@ 
+! { dg-do compile }
+! { dg-options "-fno-automatic -frecursive -Wno-overwrite-recursive" }
+!
+! Test case contributed by Mark Eggleston  <mark.eggleston@codethink.com>
+!
+
+program test
+  ! do nothing
+end program
+
-- 
2.11.0