Fortran: different character lengths in array constructor [PR93289]

Message ID f15cb213-a2cb-4582-b422-d1fe935beb7b@gmx.de
State New
Headers
Series Fortran: different character lengths in array constructor [PR93289] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

Harald Anlauf Feb. 1, 2025, 6:25 p.m. UTC
  Dear all,

the attached patch downgrades different constant character lengths in an
array constructor from a GNU to a legacy extension, so that users get a
warning with -std=gnu.  We continue to generate an error when standard
conformance is requested.

Regtested on x86_64-pc-linux-gnu (found one testcase where this
triggered... :)

OK for mainline?

Thanks,
Harald
  

Comments

Jerry D Feb. 1, 2025, 7:30 p.m. UTC | #1
On 2/1/25 10:25 AM, Harald Anlauf wrote:
> Dear all,
> 
> the attached patch downgrades different constant character lengths in an
> array constructor from a GNU to a legacy extension, so that users get a
> warning with -std=gnu.  We continue to generate an error when standard
> conformance is requested.
> 
> Regtested on x86_64-pc-linux-gnu (found one testcase where this
> triggered... :)
> 
> OK for mainline?
> 
> Thanks,
> Harald
> 

Yes, OK, and thanks

Jerry
  
Steve Kargl Feb. 1, 2025, 8:03 p.m. UTC | #2
On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
> 
> the attached patch downgrades different constant character lengths in an
> array constructor from a GNU to a legacy extension, so that users get a
> warning with -std=gnu.  We continue to generate an error when standard
> conformance is requested.
> 
> Regtested on x86_64-pc-linux-gnu (found one testcase where this
> triggered... :)
> 
> OK for mainline?
> 

My vote is 'no'.

This is either a GNU extension or an error.  It is certainly
not a legacy issue as array constructors simple cannot appear
old moldy *legacy* codes.  

I would be in favor of making it a hard error.  If you believe
gfortan must be able to compile invalid source, then add an option
such as -fallow-invalid-scalar-character-entities-in-array-constructor.

$0.02
  
Harald Anlauf Feb. 1, 2025, 8:49 p.m. UTC | #3
Am 01.02.25 um 21:03 schrieb Steve Kargl:
> On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
>>
>> the attached patch downgrades different constant character lengths in an
>> array constructor from a GNU to a legacy extension, so that users get a
>> warning with -std=gnu.  We continue to generate an error when standard
>> conformance is requested.
>>
>> Regtested on x86_64-pc-linux-gnu (found one testcase where this
>> triggered... :)
>>
>> OK for mainline?
>>
> 
> My vote is 'no'.
> 
> This is either a GNU extension or an error.  It is certainly
> not a legacy issue as array constructors simple cannot appear
> old moldy *legacy* codes.

legacy /= moldy.

My intention is to downgrade existing, potentially dangerous
GNU extensions (like this one) carefully to "legacy", but not
with an axe.

> I would be in favor of making it a hard error.  If you believe
> gfortan must be able to compile invalid source, then add an option
> such as -fallow-invalid-scalar-character-entities-in-array-constructor.

I don't see why we shall scare users by making code that is currently
accepted silently, because it is a GNU extension, suddenly to a hard
error.

So why must we be so tough?

> $0.02
>
  
Harald Anlauf Feb. 1, 2025, 9:55 p.m. UTC | #4
Am 01.02.25 um 21:49 schrieb Harald Anlauf:
> So why must we be so tough?

Here's what I get from other compilers:

Intel, Nvidia, AMD flang: silent by default.

ifx -stand f18:

char_array_constructor_5.f90(11): warning #8208: If type specification 
is omitted, each ac-value expression  in the array constructor of type 
CHARACTER must have the same length type parameters.   ['ab']
print *, [str, "ab", "hjf333"]  ! { dg-warning "must have the same length" }
---------------^

So no error, but Intel is known to often be on the loose side.

NAG: hard error.

That's what we get with -std=f*, too.
  
Steve Kargl Feb. 2, 2025, 1:01 a.m. UTC | #5
On Sat, Feb 01, 2025 at 09:49:17PM +0100, Harald Anlauf wrote:
> Am 01.02.25 um 21:03 schrieb Steve Kargl:
> > On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
> > > 
> > > the attached patch downgrades different constant character lengths in an
> > > array constructor from a GNU to a legacy extension, so that users get a
> > > warning with -std=gnu.  We continue to generate an error when standard
> > > conformance is requested.
> > > 
> > > Regtested on x86_64-pc-linux-gnu (found one testcase where this
> > > triggered... :)
> > > 
> > > OK for mainline?
> > > 
> > 
> > My vote is 'no'.
> > 
> > This is either a GNU extension or an error.  It is certainly
> > not a legacy issue as array constructors simple cannot appear
> > old moldy *legacy* codes.
> 
> legacy /= moldy.
> 
> My intention is to downgrade existing, potentially dangerous
> GNU extensions (like this one) carefully to "legacy", but not
> with an axe.
> 
> > I would be in favor of making it a hard error.  If you believe
> > gfortan must be able to compile invalid source, then add an option
> > such as -fallow-invalid-scalar-character-entities-in-array-constructor.
> 
> I don't see why we shall scare users by making code that is currently
> accepted silently, because it is a GNU extension, suddenly to a hard
> error.
> 
> So why must we be so tough?
> 

Because -std=legacy allows a whole bunch of garbage.

Instead of fixing broken code, a user will slap -std=legacy
in a Makefile and move on.  Then years from now, you'll see
-std=legacy in a whole bunch of Makefiles whether it is needed
or not.  See -maligned-double and -fallow-argument-mismatch as
poster children.

Removing these types of extensions and legacy features
will hopefully make gfortran easier to maintain.

Again, just my $0.02.
  
Richard Sandiford Feb. 3, 2025, 10:49 a.m. UTC | #6
Steve Kargl <sgk@troutmask.apl.washington.edu> writes:
> On Sat, Feb 01, 2025 at 09:49:17PM +0100, Harald Anlauf wrote:
>> Am 01.02.25 um 21:03 schrieb Steve Kargl:
>> > On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
>> > > 
>> > > the attached patch downgrades different constant character lengths in an
>> > > array constructor from a GNU to a legacy extension, so that users get a
>> > > warning with -std=gnu.  We continue to generate an error when standard
>> > > conformance is requested.
>> > > 
>> > > Regtested on x86_64-pc-linux-gnu (found one testcase where this
>> > > triggered... :)
>> > > 
>> > > OK for mainline?
>> > > 
>> > 
>> > My vote is 'no'.
>> > 
>> > This is either a GNU extension or an error.  It is certainly
>> > not a legacy issue as array constructors simple cannot appear
>> > old moldy *legacy* codes.
>> 
>> legacy /= moldy.
>> 
>> My intention is to downgrade existing, potentially dangerous
>> GNU extensions (like this one) carefully to "legacy", but not
>> with an axe.
>> 
>> > I would be in favor of making it a hard error.  If you believe
>> > gfortan must be able to compile invalid source, then add an option
>> > such as -fallow-invalid-scalar-character-entities-in-array-constructor.
>> 
>> I don't see why we shall scare users by making code that is currently
>> accepted silently, because it is a GNU extension, suddenly to a hard
>> error.
>> 
>> So why must we be so tough?
>> 
>
> Because -std=legacy allows a whole bunch of garbage.
>
> Instead of fixing broken code, a user will slap -std=legacy
> in a Makefile and move on.  Then years from now, you'll see
> -std=legacy in a whole bunch of Makefiles whether it is needed
> or not.  See -maligned-double and -fallow-argument-mismatch as
> poster children.

I agree that this is what will happen.  But for people running benchmarks,
it's kind-of (kind-of) a feature.  Benchmarks tend to include relatively
old code by the time that they're released, and benchmarks continue to be
relevant (or at least widely tested) after they're out of maintenance.

So it has been really useful to have -std=legacy accept old, dangerous code,
since it means that we can continue to test old benchmarks with newer
compilers.  Improving the benchmark source to avoid the dangerous constructs
would invalidate the test and make it harder to compare with historical
results.

> Again, just my $0.02.

Same here, just wanted to raise the benchmark use case.

Thanks,
Richard
  
Jerry D Feb. 3, 2025, 6:31 p.m. UTC | #7
On 2/3/25 2:49 AM, Richard Sandiford wrote:
> Steve Kargl <sgk@troutmask.apl.washington.edu> writes:
>> On Sat, Feb 01, 2025 at 09:49:17PM +0100, Harald Anlauf wrote:
>>> Am 01.02.25 um 21:03 schrieb Steve Kargl:
>>>> On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
>>>>>
>>>>> the attached patch downgrades different constant character lengths in an
>>>>> array constructor from a GNU to a legacy extension, so that users get a
>>>>> warning with -std=gnu.  We continue to generate an error when standard
>>>>> conformance is requested.
>>>>>
>>>>> Regtested on x86_64-pc-linux-gnu (found one testcase where this
>>>>> triggered... :)
>>>>>
>>>>> OK for mainline?
>>>>>
>>>>
>>>> My vote is 'no'.
>>>>
>>>> This is either a GNU extension or an error.  It is certainly
>>>> not a legacy issue as array constructors simple cannot appear
>>>> old moldy *legacy* codes.
>>>
>>> legacy /= moldy.
>>>
>>> My intention is to downgrade existing, potentially dangerous
>>> GNU extensions (like this one) carefully to "legacy", but not
>>> with an axe.
>>>
>>>> I would be in favor of making it a hard error.  If you believe
>>>> gfortan must be able to compile invalid source, then add an option
>>>> such as -fallow-invalid-scalar-character-entities-in-array-constructor.
>>>
>>> I don't see why we shall scare users by making code that is currently
>>> accepted silently, because it is a GNU extension, suddenly to a hard
>>> error.
>>>
>>> So why must we be so tough?
>>>
>>
>> Because -std=legacy allows a whole bunch of garbage.
>>
>> Instead of fixing broken code, a user will slap -std=legacy
>> in a Makefile and move on.  Then years from now, you'll see
>> -std=legacy in a whole bunch of Makefiles whether it is needed
>> or not.  See -maligned-double and -fallow-argument-mismatch as
>> poster children.
> 
> I agree that this is what will happen.  But for people running benchmarks,
> it's kind-of (kind-of) a feature.  Benchmarks tend to include relatively
> old code by the time that they're released, and benchmarks continue to be
> relevant (or at least widely tested) after they're out of maintenance.
> 
> So it has been really useful to have -std=legacy accept old, dangerous code,
> since it means that we can continue to test old benchmarks with newer
> compilers.  Improving the benchmark source to avoid the dangerous constructs
> would invalidate the test and make it harder to compare with historical
> results.
> 
>> Again, just my $0.02.
> 
> Same here, just wanted to raise the benchmark use case.
> 
> Thanks,
> Richard

I think we have had good discussion and for sake of the good of the 
order I recommend we push this for now.  The work has been done.

Regards,

Jerry
  
Harald Anlauf Feb. 3, 2025, 9:41 p.m. UTC | #8
Am 03.02.25 um 19:31 schrieb Jerry D:
> On 2/3/25 2:49 AM, Richard Sandiford wrote:
>> Steve Kargl <sgk@troutmask.apl.washington.edu> writes:
>>> On Sat, Feb 01, 2025 at 09:49:17PM +0100, Harald Anlauf wrote:
>>>> Am 01.02.25 um 21:03 schrieb Steve Kargl:
>>>>> On Sat, Feb 01, 2025 at 07:25:51PM +0100, Harald Anlauf wrote:
>>>>>>
>>>>>> the attached patch downgrades different constant character lengths 
>>>>>> in an
>>>>>> array constructor from a GNU to a legacy extension, so that users 
>>>>>> get a
>>>>>> warning with -std=gnu.  We continue to generate an error when 
>>>>>> standard
>>>>>> conformance is requested.
>>>>>>
>>>>>> Regtested on x86_64-pc-linux-gnu (found one testcase where this
>>>>>> triggered... :)
>>>>>>
>>>>>> OK for mainline?
>>>>>>
>>>>>
>>>>> My vote is 'no'.
>>>>>
>>>>> This is either a GNU extension or an error.  It is certainly
>>>>> not a legacy issue as array constructors simple cannot appear
>>>>> old moldy *legacy* codes.
>>>>
>>>> legacy /= moldy.
>>>>
>>>> My intention is to downgrade existing, potentially dangerous
>>>> GNU extensions (like this one) carefully to "legacy", but not
>>>> with an axe.
>>>>
>>>>> I would be in favor of making it a hard error.  If you believe
>>>>> gfortan must be able to compile invalid source, then add an option
>>>>> such as -fallow-invalid-scalar-character-entities-in-array- 
>>>>> constructor.
>>>>
>>>> I don't see why we shall scare users by making code that is currently
>>>> accepted silently, because it is a GNU extension, suddenly to a hard
>>>> error.
>>>>
>>>> So why must we be so tough?
>>>>
>>>
>>> Because -std=legacy allows a whole bunch of garbage.
>>>
>>> Instead of fixing broken code, a user will slap -std=legacy
>>> in a Makefile and move on.  Then years from now, you'll see
>>> -std=legacy in a whole bunch of Makefiles whether it is needed
>>> or not.  See -maligned-double and -fallow-argument-mismatch as
>>> poster children.
>>
>> I agree that this is what will happen.  But for people running 
>> benchmarks,
>> it's kind-of (kind-of) a feature.  Benchmarks tend to include relatively
>> old code by the time that they're released, and benchmarks continue to be
>> relevant (or at least widely tested) after they're out of maintenance.
>>
>> So it has been really useful to have -std=legacy accept old, dangerous 
>> code,
>> since it means that we can continue to test old benchmarks with newer
>> compilers.  Improving the benchmark source to avoid the dangerous 
>> constructs
>> would invalidate the test and make it harder to compare with historical
>> results.
>>
>>> Again, just my $0.02.
>>
>> Same here, just wanted to raise the benchmark use case.
>>
>> Thanks,
>> Richard
> 
> I think we have had good discussion and for sake of the good of the 
> order I recommend we push this for now.  The work has been done.
> 
> Regards,
> 
> Jerry
> 

Thanks, Jerry!

Pushed: r15-7336-gf3a41e6cb5d70f
  

Patch

From c2ab5fc3e4cc758ca00e1878394c8f1466462f84 Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
Date: Sat, 1 Feb 2025 19:14:21 +0100
Subject: [PATCH] Fortran: different character lengths in array constructor
 [PR93289]

	PR fortran/93289

gcc/fortran/ChangeLog:

	* decl.cc (gfc_set_constant_character_len): Downgrade different
	string lengths in character array constructor to legacy extension.

gcc/testsuite/ChangeLog:

	* gfortran.dg/unlimited_polymorphic_1.f03: Pad element in character
	array constructor to correct length.
	* gfortran.dg/char_array_constructor_5.f90: New test.
---
 gcc/fortran/decl.cc                           | 20 +++++++++++++------
 .../gfortran.dg/char_array_constructor_5.f90  | 13 ++++++++++++
 .../gfortran.dg/unlimited_polymorphic_1.f03   |  2 +-
 3 files changed, 28 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/char_array_constructor_5.f90

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index 7954a845bc0..5a46658651a 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -1889,12 +1889,20 @@  gfc_set_constant_character_len (gfc_charlen_t len, gfc_expr *expr,
 
       /* Apply the standard by 'hand' otherwise it gets cleared for
 	 initializers.  */
-      if (check_len != -1 && slen != check_len
-          && !(gfc_option.allow_std & GFC_STD_GNU))
-	gfc_error_now ("The CHARACTER elements of the array constructor "
-		       "at %L must have the same length (%ld/%ld)",
-		       &expr->where, (long) slen,
-		       (long) check_len);
+      if (check_len != -1 && slen != check_len)
+	{
+	  if (!(gfc_option.allow_std & GFC_STD_GNU))
+	    gfc_error_now ("The CHARACTER elements of the array constructor "
+			   "at %L must have the same length (%ld/%ld)",
+			   &expr->where, (long) slen,
+			   (long) check_len);
+	  else
+	    gfc_notify_std (GFC_STD_LEGACY,
+			    "The CHARACTER elements of the array constructor "
+			    "at %L must have the same length (%ld/%ld)",
+			    &expr->where, (long) slen,
+			    (long) check_len);
+	}
 
       s[len] = '\0';
       free (expr->value.character.string);
diff --git a/gcc/testsuite/gfortran.dg/char_array_constructor_5.f90 b/gcc/testsuite/gfortran.dg/char_array_constructor_5.f90
new file mode 100644
index 00000000000..0cbe6b1468d
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/char_array_constructor_5.f90
@@ -0,0 +1,13 @@ 
+! { dg-do compile }
+! { dg-additional-options "-std=gnu" }
+!
+! PR fortran/93289
+!
+! Contributed by Tobias Burnus
+
+character(len=*), parameter ::  str = "abj", str2 = "1234"
+print *, [character(5) :: str, "ab", "hjf333"]
+print *, [character(5) :: str, str2]
+print *, [str, "ab", "hjf333"]  ! { dg-warning "must have the same length" }
+print *, [str, str2]            ! { dg-warning "must have the same length" }
+end
diff --git a/gcc/testsuite/gfortran.dg/unlimited_polymorphic_1.f03 b/gcc/testsuite/gfortran.dg/unlimited_polymorphic_1.f03
index 8634031ad81..51483ed0332 100644
--- a/gcc/testsuite/gfortran.dg/unlimited_polymorphic_1.f03
+++ b/gcc/testsuite/gfortran.dg/unlimited_polymorphic_1.f03
@@ -155,7 +155,7 @@  END MODULE
   call foo([a(8),a(9)], res)
   if (trim (res) .ne. "type(a) array   8   9") STOP 1
 
-  call foo([sun, " & rain"], res)
+  call foo([sun, " & rain "], res)
   if (trim (res) .ne. "char( 8, 2)sunshine & rain") STOP 1
 
   call foo([sun//" never happens", " & rain always happens"], res)
-- 
2.43.0