Re: [PATCH] Fortran: reject MODULE PROCEDURE outside generic module interface [PR99036]
Hi Tobias,
Am 21.03.23 um 09:31 schrieb Tobias Burnus:
On 20.03.23 21:57, Harald Anlauf via Gcc-patches wrote:
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9998,6 +9998,7 @@ gfc_match_modproc (void)
if ((gfc_state_stack->state != COMP_INTERFACE
&& gfc_state_stack->state != COMP_CONTAINS)
|| gfc_state_stack->previous == NULL
+ || !current_interface.type
|| current_interface.type == INTERFACE_NAMELESS
|| current_interface.type == INTERFACE_ABSTRACT)
{
First, I do not like '!var' comparisons for enum values,
only for Booleans/logicals and pointer.
I was hesitating to do this and thought about adding an
enum value that it 0 numerically, but ...
Secondly, I am not sure that it is really guaranteed that
the value is 0.
... had assumed that this would be guaranteed.
I think something like the following makes more sense
and, as just tried, it also regtests (w/ your testcase included).
If you agree, feel free to package and commit it.
diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index c8f0bb83c2c..233bf244d62 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9996,7 +9996,8 @@ gfc_match_modproc (void)
gfc_interface *old_interface_head, *interface;
- if ((gfc_state_stack->state != COMP_INTERFACE
- && gfc_state_stack->state != COMP_CONTAINS)
- || gfc_state_stack->previous == NULL
+ if (gfc_state_stack->previous == NULL
+ || (gfc_state_stack->state != COMP_INTERFACE
+ && (gfc_state_stack->state != COMP_CONTAINS
+ || gfc_state_stack->previous->state != COMP_INTERFACE))
|| current_interface.type == INTERFACE_NAMELESS
|| current_interface.type == INTERFACE_ABSTRACT)
Yes, that's a much cleaner solution. Pushed as:
https://gcc.gnu.org/g:dd282b16bfd3c6e218dffb7798a375365b10ae22
commit r13-6790-gdd282b16bfd3c6e218dffb7798a375365b10ae22
Thanks for the review!
Harald
Thanks for working on this and all the other issues!
Tobias
-
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: [PATCHv4, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi,
LGTM, except for:
On 21.03.23 07:29, HAO CHEN GUI wrote:
@@ -4708,7 +4710,12 @@ do_simplify (gfc_intrinsic_sym *specific, gfc_expr *e)
finish:
if (result == _bad_expr)
-return false;
+{
+ if (errorcount == old_errorcount
+ && (!gfc_buffered_p () && !gfc_error_flag_test ()))
+ gfc_error ("Cannot simplify expression at %L", >where);
+ return false;
+}
The second line of the condition now tests:
* 'If buffering is disabled and no pending buffed error exists
then show an error'
But if should tests:
* 'If (buffering is disabled) OR ((it is enabled but) no buffered error exists)
then show¹ an error'
Thus, you should use an '||' not a '&&':
+ && (!gfc_buffered_p () || !gfc_error_flag_test ()))
as proposed in previous email. A quick regtesting shows no fails when doing so.
OK with that change.
(¹or rather: 'then buffer an error')
Thanks for the patch!
Tobias
-
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] Fortran: reject MODULE PROCEDURE outside generic module interface [PR99036]
On 20.03.23 21:57, Harald Anlauf via Gcc-patches wrote:
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9998,6 +9998,7 @@ gfc_match_modproc (void)
if ((gfc_state_stack->state != COMP_INTERFACE
&& gfc_state_stack->state != COMP_CONTAINS)
|| gfc_state_stack->previous == NULL
+ || !current_interface.type
|| current_interface.type == INTERFACE_NAMELESS
|| current_interface.type == INTERFACE_ABSTRACT)
{
First, I do not like '!var' comparisons for enum values,
only for Booleans/logicals and pointer.
Secondly, I am not sure that it is really guaranteed that
the value is 0.
I think something like the following makes more sense
and, as just tried, it also regtests (w/ your testcase included).
If you agree, feel free to package and commit it.
diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index c8f0bb83c2c..233bf244d62 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9996,7 +9996,8 @@ gfc_match_modproc (void)
gfc_interface *old_interface_head, *interface;
- if ((gfc_state_stack->state != COMP_INTERFACE
- && gfc_state_stack->state != COMP_CONTAINS)
- || gfc_state_stack->previous == NULL
+ if (gfc_state_stack->previous == NULL
+ || (gfc_state_stack->state != COMP_INTERFACE
+ && (gfc_state_stack->state != COMP_CONTAINS
+ || gfc_state_stack->previous->state != COMP_INTERFACE))
|| current_interface.type == INTERFACE_NAMELESS
|| current_interface.type == INTERFACE_ABSTRACT)
Thanks for working on this and all the other issues!
Tobias
-
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] Fortran: reject MODULE PROCEDURE outside generic module interface [PR99036]
Hi Harald, This is good for trunk and for backporting. Thanks for the rapid fix. Paul On Mon, 20 Mar 2023 at 20:57, Harald Anlauf via Fortran wrote: > Dear all, > > the attached trivial patch catches a MODULE PROCEDURE outside of a > module interface before we run into an internal error. > > Regtested on x86_64-pc-linux-gnu. OK for mainline? > > This PR is marked as an 11/12/13 regression, so this is a candidate > for backporting. > > Thanks, > Harald > > -- "If you can't explain it simply, you don't understand it well enough" - Albert Einstein
[PATCHv4, gfortran] Escalate failure when Hollerith constant to real conversion fails [PR103628]
Hi,
I refined the patch according to reviewer's advice. The main change is to
check if buffer_p is set and buffered error exists. Also two regtests are
fixed by catching the new error.
I sent out the revised one for review due to my limited knowledge on
Fortran front end.
The patch escalates the failure when Hollerith constant to real conversion
fails in native_interpret_expr. It finally reports an "Cannot simplify
expression" error in do_simplify method.
The patch for pr95450 added a verification for decoding/encoding checking
in native_interpret_expr. native_interpret_expr may fail on real type
conversion and returns a NULL tree then. But upper layer calls don't handle
the failure so that an ICE is reported when the verification fails.
IBM long double is an example. It doesn't have a unique memory presentation
for some real values. So it may not pass the verification. The new test
case shows the problem.
errorcount is used to check if an error is already reported or not when
getting a bad expr. Buffered errors need to be excluded as they don't
increase error count either.
The patch passed regression test on Power and x86 linux platforms.
Thanks
Gui Haochen
ChangeLog
2023-03-21 Haochen Gui
gcc/
PR target/103628
* fortran/target-memory.cc (gfc_interpret_float): Return FAIL when
native_interpret_expr gets a NULL tree.
* fortran/arith.cc (gfc_hollerith2real): Return NULL when
gfc_interpret_float fails.
* fortran/error.cc (gfc_buffered_p): Define.
* fortran/gfortran.h (gfc_buffered_p): Declare.
* fortran/intrinsic.cc: Add diagnostic.h to include list.
(do_simplify): Save errorcount and check it at finish. Report a
"Cannot simplify expression" error on a bad result if error count
doesn't change and no other errors buffered.
gcc/testsuite/
PR target/103628
* gfortran.dg/assumed_size_refs_2.f90: Catch "Cannot simplify
expression" error.
* gfortran.dg/unpack_field_1.f90: Likewise.
* gfortran.dg/pr103628.f90: New.
Co-Authored-By: Tobias Burnus
patch.diff
diff --git a/gcc/fortran/arith.cc b/gcc/fortran/arith.cc
index c0d12cfad9d..d3d38c7eb6a 100644
--- a/gcc/fortran/arith.cc
+++ b/gcc/fortran/arith.cc
@@ -2752,10 +2752,12 @@ gfc_hollerith2real (gfc_expr *src, int kind)
result = gfc_get_constant_expr (BT_REAL, kind, >where);
hollerith2representation (result, src);
- gfc_interpret_float (kind, (unsigned char *) result->representation.string,
- result->representation.length, result->value.real);
-
- return result;
+ if (gfc_interpret_float (kind,
+ (unsigned char *) result->representation.string,
+ result->representation.length, result->value.real))
+return result;
+ else
+return NULL;
}
/* Convert character to real. The constant will be padded or truncated. */
diff --git a/gcc/fortran/error.cc b/gcc/fortran/error.cc
index 214fb78ba7b..872d42e731e 100644
--- a/gcc/fortran/error.cc
+++ b/gcc/fortran/error.cc
@@ -49,6 +49,13 @@ static gfc_error_buffer error_buffer;
static output_buffer *pp_error_buffer, *pp_warning_buffer;
static int warningcount_buffered, werrorcount_buffered;
+/* Return buffered_p. */
+bool
+gfc_buffered_p (void)
+{
+ return buffered_p;
+}
+
/* Return true if there output_buffer is empty. */
static bool
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 219ef8c7612..edfe11796a6 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3328,6 +3328,7 @@ void gfc_internal_error (const char *, ...)
ATTRIBUTE_NORETURN ATTRIBUTE_GCC_GFC
void gfc_clear_error (void);
bool gfc_error_check (void);
bool gfc_error_flag_test (void);
+bool gfc_buffered_p (void);
notification gfc_notification_std (int);
bool gfc_notify_std (int, const char *, ...) ATTRIBUTE_GCC_GFC(2,3);
diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc
index e89131f5a71..2572b7a3448 100644
--- a/gcc/fortran/intrinsic.cc
+++ b/gcc/fortran/intrinsic.cc
@@ -25,6 +25,7 @@ along with GCC; see the file COPYING3. If not see
#include "options.h"
#include "gfortran.h"
#include "intrinsic.h"
+#include "diagnostic.h" /* For errorcount. */
/* Namespace to hold the resolved symbols for intrinsic subroutines. */
static gfc_namespace *gfc_intrinsic_namespace;
@@ -4620,6 +4621,7 @@ do_simplify (gfc_intrinsic_sym *specific, gfc_expr *e)
{
gfc_expr *result, *a1, *a2, *a3, *a4, *a5, *a6;
gfc_actual_arglist *arg;
+ int old_errorcount = errorcount;
/* Max and min require special handling due to the variable number
of args. */
@@ -4708,7 +4710,12 @@ do_simplify (gfc_intrinsic_sym *specific, gfc_expr *e)
finish:
if (result == _bad_expr)
-return false;
+{
+ if (errorcount == old_errorcount
+ && (!gfc_buffered_p () && !gfc_error_flag_test ()))
+ gfc_error ("Cannot simplify expression at %L", >where);
+
