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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Mark Eggleston
Original thread starts here
https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01185.html

OK to commit?

gcc/fortran/ChangeLog

     Mark Eggleston  <[hidden email]>

     * 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 <[hidden email]>

     * 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


0001-Suppress-warning-with-Wno-overwrite-recursive.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Mark Eggleston

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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Tobias Burnus-3
In reply to this post by Mark Eggleston
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  <[hidden email]>
>
>     * 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 <[hidden email]>
>
>     * gfortran.dg/no_overwrite_recursive_1.f90: New test.
>     * gfortran.dg/no_overwrite_recursive_2.f90: New test.
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Tobias Burnus
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Jeff Law
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval

Tobias Burnus-5
In reply to this post by Tobias Burnus-3
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.)

Reply | Threaded
Open this post in threaded view
|

Argument-mismatch fallout (was: Re: [PATCH, Fortran] Optionally suppress no-automatic overwrites recursive warning - for approval)

Tobias Burnus-3
In reply to this post by Jeff Law
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
>
Reply | Threaded
Open this post in threaded view
|

Re: Argument-mismatch fallout

Thomas Koenig-6


> 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