Re: [PATCH, v2] Fortran: improve attribute conflict checking [PR93635]

2024-05-24 Thread Harald Anlauf

Hi Mikael,

On 5/24/24 20:17, Mikael Morin wrote:

Le 23/05/2024 à 21:15, Harald Anlauf a écrit :

Hi Mikael,

On 5/23/24 09:49, Mikael Morin wrote:

Le 13/05/2024 à 09:25, Mikael Morin a écrit :

Le 10/05/2024 à 21:56, Harald Anlauf a écrit :

Am 10.05.24 um 21:48 schrieb Harald Anlauf:

Hi Mikael,

Am 10.05.24 um 11:45 schrieb Mikael Morin:

Le 09/05/2024 à 22:30, Harald Anlauf a écrit :

I'll stop here...


Thanks. Go figure, I have no problem reproducing today.
It's PR99798 (and there is even a patch for it).


this patch has rotten a bit: the type of gfc_reluease_symbol
has changed to bool, this can be fixed.

Unfortunately, applying the patch does not remove the ICEs here...


Oops, I take that back!  There was an error on my side applying the
patch; and now it does fix the ICEs after correcting that hickup


Now the PR99798 patch is ready to be pushed, but I won't be available
for a few days.  We can finish our discussion on this topic afterwards.


Hello,

I'm coming back to this.
I think either one of Steve's patch or your variant in the PR is a
better fix for the ICE as a first step; they seem less fragile at least.
Then we can look at a possible reordering of conflict checks as with the
patch you originally submitted in this thread.


like the attached variant?


Yes.  The churn in the testsuite is actually not that bad.
OK for master, thanks for the patch.


thanks, will do.

I wouldn't push for backporting, but if you feel like doing it, it seems 
safe enough (depending on my own backport for PR99798 of course).


There's no pressing need.  I'll mark the patch as backportable
with dependency in my own list, in case the question comes up.

Regarding the conflict check reordering, I'm tempted to just drop it at 
this point, or do you think it remains worth it?


I don't really have a showcase where this would bring a benefit now,
so I'm dropping this idea.

There are issues where specifying a standard version changes
the error recovery path (or rather lead to an ICE), but as some
of these are due to emitting an error during parsing instead of
during resolution, my suggestion does not help there.

If you look for an example: this one is taken from pr101281

subroutine a3pr (xn) bind(C)
  character(len=n), pointer :: xn(..)
end

vs.

subroutine a3pr (xn) bind(C)
  character(len=n), pointer :: xn
  dimension :: xn(..)
end

The first one gives lots of invalid reads in valgrind with -std=f2008,
or ICEs, while the second does not.

Thanks,
Harald




Re: [PATCH, v2] Fortran: improve attribute conflict checking [PR93635]

2024-05-24 Thread Mikael Morin

Le 23/05/2024 à 21:15, Harald Anlauf a écrit :

Hi Mikael,

On 5/23/24 09:49, Mikael Morin wrote:

Le 13/05/2024 à 09:25, Mikael Morin a écrit :

Le 10/05/2024 à 21:56, Harald Anlauf a écrit :

Am 10.05.24 um 21:48 schrieb Harald Anlauf:

Hi Mikael,

Am 10.05.24 um 11:45 schrieb Mikael Morin:

Le 09/05/2024 à 22:30, Harald Anlauf a écrit :

I'll stop here...


Thanks. Go figure, I have no problem reproducing today.
It's PR99798 (and there is even a patch for it).


this patch has rotten a bit: the type of gfc_reluease_symbol
has changed to bool, this can be fixed.

Unfortunately, applying the patch does not remove the ICEs here...


Oops, I take that back!  There was an error on my side applying the
patch; and now it does fix the ICEs after correcting that hickup


Now the PR99798 patch is ready to be pushed, but I won't be available
for a few days.  We can finish our discussion on this topic afterwards.


Hello,

I'm coming back to this.
I think either one of Steve's patch or your variant in the PR is a
better fix for the ICE as a first step; they seem less fragile at least.
Then we can look at a possible reordering of conflict checks as with the
patch you originally submitted in this thread.


like the attached variant?


Yes.  The churn in the testsuite is actually not that bad.
OK for master, thanks for the patch.
I wouldn't push for backporting, but if you feel like doing it, it seems 
safe enough (depending on my own backport for PR99798 of course).


Regarding the conflict check reordering, I'm tempted to just drop it at 
this point, or do you think it remains worth it?





[PATCH, v2] Fortran: improve attribute conflict checking [PR93635]

2024-05-23 Thread Harald Anlauf

Hi Mikael,

On 5/23/24 09:49, Mikael Morin wrote:

Le 13/05/2024 à 09:25, Mikael Morin a écrit :

Le 10/05/2024 à 21:56, Harald Anlauf a écrit :

Am 10.05.24 um 21:48 schrieb Harald Anlauf:

Hi Mikael,

Am 10.05.24 um 11:45 schrieb Mikael Morin:

Le 09/05/2024 à 22:30, Harald Anlauf a écrit :

I'll stop here...


Thanks. Go figure, I have no problem reproducing today.
It's PR99798 (and there is even a patch for it).


this patch has rotten a bit: the type of gfc_reluease_symbol
has changed to bool, this can be fixed.

Unfortunately, applying the patch does not remove the ICEs here...


Oops, I take that back!  There was an error on my side applying the
patch; and now it does fix the ICEs after correcting that hickup

Now the PR99798 patch is ready to be pushed, but I won't be available 
for a few days.  We can finish our discussion on this topic afterwards.



Hello,

I'm coming back to this.
I think either one of Steve's patch or your variant in the PR is a 
better fix for the ICE as a first step; they seem less fragile at least.
Then we can look at a possible reordering of conflict checks as with the 
patch you originally submitted in this thread.


like the attached variant?

Harald


Mikael

From 68d73e6e2efa692afff10ea16eafb88236cbe69c Mon Sep 17 00:00:00 2001
From: Harald Anlauf 
Date: Thu, 23 May 2024 21:13:00 +0200
Subject: [PATCH] Fortran: improve attribute conflict checking [PR93635]

gcc/fortran/ChangeLog:

	PR fortran/93635
	* symbol.cc (conflict_std): Helper function for reporting attribute
	conflicts depending on the Fortran standard version.
	(conf_std): Helper macro for checking standard-dependent conflicts.
	(gfc_check_conflict): Use it.

gcc/testsuite/ChangeLog:

	PR fortran/93635
	* gfortran.dg/c-interop/c1255-2.f90: Adjust pattern.
	* gfortran.dg/pr87907.f90: Likewise.
	* gfortran.dg/pr93635.f90: New test.

Co-authored-by: Steven G. Kargl 
---
 gcc/fortran/symbol.cc | 63 +--
 .../gfortran.dg/c-interop/c1255-2.f90 |  4 +-
 gcc/testsuite/gfortran.dg/pr87907.f90 |  8 ++-
 gcc/testsuite/gfortran.dg/pr93635.f90 | 19 ++
 4 files changed, 54 insertions(+), 40 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/pr93635.f90

diff --git a/gcc/fortran/symbol.cc b/gcc/fortran/symbol.cc
index 0a1646def67..5db3c887127 100644
--- a/gcc/fortran/symbol.cc
+++ b/gcc/fortran/symbol.cc
@@ -407,18 +407,36 @@ gfc_check_function_type (gfc_namespace *ns)
 
 / Symbol attribute stuff */
 
+/* Older standards produced conflicts for some attributes that are allowed
+   in newer standards.  Check for the conflict and issue an error depending
+   on the standard in play.  */
+
+static bool
+conflict_std (int standard, const char *a1, const char *a2, const char *name,
+	  locus *where)
+{
+  if (name == NULL)
+{
+  return gfc_notify_std (standard, "%s attribute conflicts "
+			 "with %s attribute at %L", a1, a2,
+			 where);
+}
+  else
+{
+  return gfc_notify_std (standard, "%s attribute conflicts "
+			 "with %s attribute in %qs at %L",
+			 a1, a2, name, where);
+}
+}
+
 /* This is a generic conflict-checker.  We do this to avoid having a
single conflict in two places.  */
 
 #define conf(a, b) if (attr->a && attr->b) { a1 = a; a2 = b; goto conflict; }
 #define conf2(a) if (attr->a) { a2 = a; goto conflict; }
-#define conf_std(a, b, std) if (attr->a && attr->b)\
-  {\
-a1 = a;\
-a2 = b;\
-standard = std;\
-goto conflict_std;\
-  }
+#define conf_std(a, b, std) if (attr->a && attr->b \
+&& !conflict_std (std, a, b, name, where)) \
+return false;
 
 bool
 gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where)
@@ -451,7 +469,6 @@ gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where)
 		"OACC DECLARE DEVICE_RESIDENT";
 
   const char *a1, *a2;
-  int standard;
 
   if (attr->artificial)
 return true;
@@ -460,20 +477,10 @@ gfc_check_conflict (symbol_attribute *attr, const char *name, locus *where)
 where = _current_locus;
 
   if (attr->pointer && attr->intent != INTENT_UNKNOWN)
-{
-  a1 = pointer;
-  a2 = intent;
-  standard = GFC_STD_F2003;
-  goto conflict_std;
-}
+conf_std (pointer, intent, GFC_STD_F2003);
 
-  if (attr->in_namelist && (attr->allocatable || attr->pointer))
-{
-  a1 = in_namelist;
-  a2 = attr->allocatable ? allocatable : pointer;
-  standard = GFC_STD_F2003;
-  goto conflict_std;
-}
+  conf_std (in_namelist, allocatable, GFC_STD_F2003);
+  conf_std (in_namelist, pointer, GFC_STD_F2003);
 
   /* Check for attributes not allowed in a BLOCK DATA.  */
   if (gfc_current_state () == COMP_BLOCK_DATA)
@@ -922,20 +929,6 @@ conflict:
 	   a1, a2,