[PING][PATCH 1/2] Ada: Synchronized private extensions are always limited

[PING][PATCH 1/2] Ada: Synchronized private extensions are always limited

[Richard Wai]

[-- Attachment #1: Type: text/plain, Size: 4084 bytes --]

 

From: Richard Wai <richard@annexi-strayline.com> 
Sent: Thursday, August 10, 2023 12:55 AM
To: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
Cc: 'Eric Botcazou' <ebotcazou@adacore.com>; 'Arnaud Charlet'
<charlet@adacore.com>; 'Stephen Baird' <baird@adacore.com>
Subject: [PATCH 1/2] Ada: Synchronized private extensions are always limited

 

GNAT currently considers a synchronized private extension that derives from
an interface to be limited only when said interface is a concurrent
interface. However it is of course legal for a synchronized private
extension to derive from a limited interface. In this case GNAT fails to
correctly determine that the private extension is limited.

 

This causes two separate problems that makes discriminated types in such a
case impossible:

1.	GNAT inappropriately rejects compilation, claiming default
discriminants on such a private extension are illegal.
2.	GNAT fails to generate the expected discriminals for the
unconstrained discriminanted case, leading to the corresponding
discriminants of the "corresponding record" of the underlying concurrent
type to have no identifiers, and thus compilation fails.

 

Fairly simple fix. If "synchronized" appears in the private extension
declaration, it is limited. This is explicit in the RM as well (7.3(6/2)).

 

Fixing this bug uncovered of a related bug wrt. TSS address finalizer
generation for constrained subtypes of synchronized private extensions with
no default discriminants. That patch is to follow separately.

 

Patch file is attached.

 

--  Begin change log entry --

 

ada: Private extensions with the keyword "synchronized" are always limited.

 

GNAT was relying on synchronized private type extensions deriving from a
concurrent interface to determine its limitedness. This does not cover the
case where such an extension derives a limited interface. RM-7.6(6/2) makes
is clear that "synchronized" in a private extension implies the derived type
is limited. GNAT should explicitly check for the presence of "synchronized"
in a private extension declaration, and it should have the same effect as
the presence of "limited".

 

gcc/ada/

                * sem_ch3.adb (Build_Derived_Record_Type): Treat presence of
keyword "synchronized" the same as "limited" when determining if a private
extension is limited.

 

-- End change log entry --

 

This patch was bootstrapped on x86_64-*-freebsd13.2. Two new test cases were
added. Note that 4 gnat test cases fail currently on master and are
unrelated to this patch.

 

Check-ada output of this patch:

                                === acats tests ===

Running chapter a ...

Running chapter c2 ...

Running chapter c3 ...

Running chapter c4 ...

Running chapter c5 ...

Running chapter c6 ...

Running chapter c7 ...

Running chapter c8 ...

Running chapter c9 ...

Running chapter ca ...

Running chapter cb ...

Running chapter cc ...

Running chapter cd ...

Running chapter ce ...

Running chapter cxa ...

Running chapter cxb ...

Running chapter cxf ...

Running chapter cxg ...

Running chapter cxh ...

Running chapter cz ...

Running chapter d ...

Running chapter e ...

Running chapter l ...

                                === acats Summary ===

# of expected passes                       2328

# of unexpected failures 0

 

Native configuration is x86_64-unknown-freebsd13.2

 

                                === gnat tests ===

 

Schedule of variations:

    unix

 

Running target unix

FAIL: gnat.dg/specs/alignment2.ads  (test for warnings, line 14)

FAIL: gnat.dg/specs/alignment2.ads  (test for warnings, line 20)

FAIL: gnat.dg/specs/alignment2.ads  (test for warnings, line 38)

FAIL: gnat.dg/specs/alignment2.ads  (test for warnings, line 42)

 

                                === gnat Summary ===

 

# of expected passes                       3402

# of unexpected failures                4

# of expected failures                      23

# of unsupported tests                   10

gnatmake version 14.0.0 20230809 (experimental)

 

 

Richard Wai

ANNEXI-STRAYLINE

Re: [PING][PATCH 1/2] Ada: Synchronized private extensions are always limited

[Arnaud Charlet]

Richard,

For some reason, your email is endeing up in a strange format, I almost
missed the .patch file attached, making the review harder.

There's a typo in the comment added:

+      --  explicit limitedness implied by a synchronized private extension
+      --  the does not derive from a synchronized interface (see RM-7.3(6/2)).

the -> that does not derive...

OK with this change.

> From: Richard Wai <richard@annexi-strayline.com> 
> Sent: Thursday, August 10, 2023 12:55 AM
> To: 'gcc-patches@gcc.gnu.org' <gcc-patches@gcc.gnu.org>
> Cc: 'Eric Botcazou' <ebotcazou@adacore.com>; 'Arnaud Charlet'
> <charlet@adacore.com>; 'Stephen Baird' <baird@adacore.com>
> Subject: [PATCH 1/2] Ada: Synchronized private extensions are always limited
> 
>  
> 
> GNAT currently considers a synchronized private extension that derives from
> an interface to be limited only when said interface is a concurrent
> interface. However it is of course legal for a synchronized private
> extension to derive from a limited interface. In this case GNAT fails to
> correctly determine that the private extension is limited.
> 
>  
> 
> This causes two separate problems that makes discriminated types in such a
> case impossible:
> 
> 1.	GNAT inappropriately rejects compilation, claiming default
> discriminants on such a private extension are illegal.
> 2.	GNAT fails to generate the expected discriminals for the
> unconstrained discriminanted case, leading to the corresponding
> discriminants of the "corresponding record" of the underlying concurrent
> type to have no identifiers, and thus compilation fails.
> 
>  
> 
> Fairly simple fix. If "synchronized" appears in the private extension
> declaration, it is limited. This is explicit in the RM as well (7.3(6/2)).
> 
>  
> 
> Fixing this bug uncovered of a related bug wrt. TSS address finalizer
> generation for constrained subtypes of synchronized private extensions with
> no default discriminants. That patch is to follow separately.
> 
>  
> 
> Patch file is attached.
> 
>  
> 
> --  Begin change log entry --
> 
>  
> 
> ada: Private extensions with the keyword "synchronized" are always limited.
> 
>  
> 
> GNAT was relying on synchronized private type extensions deriving from a
> concurrent interface to determine its limitedness. This does not cover the
> case where such an extension derives a limited interface. RM-7.6(6/2) makes
> is clear that "synchronized" in a private extension implies the derived type
> is limited. GNAT should explicitly check for the presence of "synchronized"
> in a private extension declaration, and it should have the same effect as
> the presence of "limited".
> 
>  
> 
> gcc/ada/
> 
>                 * sem_ch3.adb (Build_Derived_Record_Type): Treat presence of
> keyword "synchronized" the same as "limited" when determining if a private
> extension is limited.
> 
> 
> -- End change log entry --
>  
> 
> This patch was bootstrapped on x86_64-*-freebsd13.2. Two new test cases were
> added. Note that 4 gnat test cases fail currently on master and are
> unrelated to this patch.

Re: [PING][PATCH 1/2] Ada: Synchronized private extensions are always limited

[Arnaud Charlet]

> For some reason, your email is endeing up in a strange format, I almost
> missed the .patch file attached, making the review harder.

Never mind, I was on vacation earlier this month and then busy with a seminar last week, so I started looking at your ping email before the original email which did contain the patch easily found, sorry for the noise!

Arno

[PATCH 1/2 v2] Ada: Synchronized private extensions are always limited

[Richard Wai]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

Hi Arno,

No worries, and sorry for the trouble. I’m going to try using a different client for the gcc mailing list, it doesn’t seem to like Outlook. Thanks for catching that mistake!

Please advise how I can get this patch actually applied, given my lack of commit privilege.

Revised patch attached!

Thanks!


[-- Attachment #2: ada-synchronized-private-types-are-limited-v2.patch --]
[-- Type: application/octet-stream, Size: 5896 bytes --]

From f87d0da43296b87cd242380a4077a167e5ea1291 Mon Sep 17 00:00:00 2001
From: Richard Wai <richard@annexi-strayline.com>
Date: Wed, 9 Aug 2023 01:54:48 -0400
Subject: [PATCH 1/2] ada: Consider that any synchronized private extension is
 always a limited type, even if it inherits from a non-concurrent interface.
 Add two regression tests for two separate problems that arose

---
 gcc/ada/sem_ch3.adb                           | 12 +++--
 .../gnat.dg/sync_tag_discriminals.adb         | 51 +++++++++++++++++++
 gcc/testsuite/gnat.dg/sync_tag_limited.adb    | 50 ++++++++++++++++++
 3 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
 create mode 100644 gcc/testsuite/gnat.dg/sync_tag_limited.adb

diff --git a/gcc/ada/sem_ch3.adb b/gcc/ada/sem_ch3.adb
index 042ace01724..48731a7bf04 100644
--- a/gcc/ada/sem_ch3.adb
+++ b/gcc/ada/sem_ch3.adb
@@ -9494,9 +9494,15 @@ package body Sem_Ch3 is
 
       --  AI-419: Limitedness is not inherited from an interface parent, so to
       --  be limited in that case the type must be explicitly declared as
-      --  limited. However, task and protected interfaces are always limited.
-
-      if Limited_Present (Type_Def) then
+      --  limited, or synchronized. While task and protected interfaces are
+      --  always limited, a synchronized private extension might not inherit
+      --  from such interfaces, and so we also need to recognize the
+      --  explicit limitedness implied by a synchronized private extension
+      --  that does not derive from a synchronized interface (see RM-7.3(6/2)).
+
+      if Limited_Present (Type_Def)
+        or else Synchronized_Present (Type_Def)
+      then
          Set_Is_Limited_Record (Derived_Type);
 
       elsif Is_Limited_Record (Parent_Type)
diff --git a/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb b/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
new file mode 100644
index 00000000000..b105acf6e98
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
@@ -0,0 +1,51 @@
+-- This test is related to sync_tag_limited in that previous versions of GNAT
+-- failed to consider a synchronized private extension as limited if it was
+-- not derrived from a synchronized interface (i.e. a limited interface). Since
+-- such a private type would not be considered limited, GNAT would fail to
+-- correctly build the expected discriminals later needed by the creation of
+-- the concurrent type's "corresponding record type", leading to a compilation
+-- error where the discriminants of the corresponding record type had no
+-- identifiers.
+--
+-- This test is in addition to sync_tag_limited because the sync_tag_limited
+-- would fail for "legality" reasons (default discriminants not allowed for
+-- a non-limited taged type). It is also an opportunity to ensure that non-
+-- defaulted discriminated synchronized private extensions work as expected.
+
+--  { dg-do compile }
+
+procedure Sync_Tag_Discriminals is
+   
+   package Ifaces is
+      
+      type Test_Interface is limited interface;
+      
+      procedure Interface_Action (Test: in out Test_Interface) is abstract;
+      
+   end Ifaces;
+   
+   
+   package Implementation is
+      type Test_Implementation
+        (Constraint: Positive) is
+        synchronized new Ifaces.Test_Interface with private;
+      
+   private
+      protected type Test_Implementation
+        (Constraint: Positive)
+      is new Ifaces.Test_Interface with
+      
+         overriding procedure Interface_Action;
+         
+      end Test_Implementation;
+   end Implementation;
+   
+   package body Implementation is
+      protected body Test_Implementation is
+         procedure Interface_Action is null;
+      end;
+   end Implementation;
+   
+begin
+   null;
+end Sync_Tag_Discriminals;
diff --git a/gcc/testsuite/gnat.dg/sync_tag_limited.adb b/gcc/testsuite/gnat.dg/sync_tag_limited.adb
new file mode 100644
index 00000000000..608f10662a3
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/sync_tag_limited.adb
@@ -0,0 +1,50 @@
+--  Synchronized tagged types created by a private extension with the keyword
+--  'synchronized' shall be seen as an (immutably) limited tagged type, and
+--  should therefore accept default disciminant spectifications.
+--  This was a bug in earlier versions of GNAT, whereby GNAT erroneously
+--  relied on a parent synchronized interface to determine limitedness
+--  of a synchronized private extension. The problem being that a synchronized
+--  private extension can derive a non-synchronized interface (specifically a
+--  limited interface), Yet the RM makes it clear (7.3(6/2)) that such
+--  synchronized private extensions are always limited.
+--
+--  Ergo: Default discriminants are of course legal on any synchronized private
+--  extension.
+
+--  { dg-do compile }
+
+procedure Sync_Tag_Limited is
+   
+   package Ifaces is
+      
+      type Test_Interface is limited interface;
+      
+      procedure Interface_Action (Test: in out Test_Interface) is abstract;
+      
+   end Ifaces;
+   
+   
+   package Implementation is
+      type Test_Implementation
+        (Constraint: Positive := 1) is
+        synchronized new Ifaces.Test_Interface with private;
+      
+   private
+      protected type Test_Implementation
+        (Constraint: Positive := 1)
+      is new Ifaces.Test_Interface with
+      
+         overriding procedure Interface_Action;
+         
+      end Test_Implementation;
+   end Implementation;
+   
+   package body Implementation is
+      protected body Test_Implementation is
+         procedure Interface_Action is null;
+      end;
+   end Implementation;
+   
+begin
+   null;
+end Sync_Tag_Limited;
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 461 bytes --]




> On Sep 1, 2023, at 08:08, Arnaud Charlet <charlet@adacore.com> wrote:
> 
>> For some reason, your email is endeing up in a strange format, I almost
>> missed the .patch file attached, making the review harder.
> 
> Never mind, I was on vacation earlier this month and then busy with a seminar last week, so I started looking at your ping email before the original email which did contain the patch easily found, sorry for the noise!
> 
> Arno

Re: [PATCH 1/2 v2] Ada: Synchronized private extensions are always limited

[Arnaud Charlet]

> No worries, and sorry for the trouble. I’m going to try using a different client for the gcc mailing list, it doesn’t seem to like Outlook. Thanks for catching that mistake!
> 
> Please advise how I can get this patch actually applied, given my lack of commit privilege.

You first need to follow instructions from https://gcc.gnu.org/contribute.html
and in particular meet the legal requirements.

Then get someone with write approval to commit the change.

Arno

Re: [PATCH 1/2 v3] Ada: Synchronized private extensions are always limited

[Marc Poulhiès]

Hello Richard,

> I have added the required “Signed-off-by” tag to the patch and to the change log
> entry below. I believe for all other aspects I have followed the instructions.

Thanks for doing these modifications. I believe you have read the
Developer's Certificate of Origin (https://gcc.gnu.org/dco.html) and
accept it?

> For getting the patch applied it states "If you do not have write access and a
> patch of yours has been approved, but not committed, please advise the approver
> of that fact.” So I think I have done that correctly.. However let me know if
> there is someone else not included in the CC that should be handling that.

I can take it from here and apply both your changes if that's ok with
you.

Thanks for your patches!

Marc

Re: [PATCH 1/2 v3] Ada: Synchronized private extensions are always limited

[Richard Wai]

Hi Marc,

Indeed I have read and accept the terms of the DCO. Please proceed to apply these changes at your convenience.

Thanks for helping me get these committed!

Richard Wai

> On Sep 18, 2023, at 04:31, Marc Poulhiès <poulhies@adacore.com> wrote:
> 
> 
> Hello Richard,
> 
>> I have added the required “Signed-off-by” tag to the patch and to the change log
>> entry below. I believe for all other aspects I have followed the instructions.
> 
> Thanks for doing these modifications. I believe you have read the
> Developer's Certificate of Origin (https://gcc.gnu.org/dco.html) and
> accept it?
> 
>> For getting the patch applied it states "If you do not have write access and a
>> patch of yours has been approved, but not committed, please advise the approver
>> of that fact.” So I think I have done that correctly.. However let me know if
>> there is someone else not included in the CC that should be handling that.
> 
> I can take it from here and apply both your changes if that's ok with
> you.
> 
> Thanks for your patches!
> 
> Marc

[COMMITTED] ada: Private extensions with the keyword "synchronized" are always limited.

[Marc Poulhiès]

From: Richard Wai <richard@annexi-strayline.com>

GNAT was relying on synchronized private type extensions deriving from a
concurrent interface to determine its limitedness. This does not cover the case
where such an extension derives a limited interface. RM-7.6(6/2) makes is clear
that "synchronized" in a private extension implies the derived type is limited.
GNAT should explicitly check for the presence of "synchronized" in a private
extension declaration, and it should have the same effect as the presence of
“limited”.

gcc/ada/ChangeLog:

	* sem_ch3.adb (Build_Derived_Record_Type): Treat presence of
	keyword "synchronized" the same as "limited" when determining if a
	private extension is limited.

gcc/testsuite/ChangeLog:

	* gnat.dg/sync_tag_discriminals.adb: New test.
	* gnat.dg/sync_tag_limited.adb: New test.

Signed-off-by: Richard Wai <richard@annexi-strayline.com>
---
 gcc/ada/sem_ch3.adb                           | 12 +++--
 .../gnat.dg/sync_tag_discriminals.adb         | 51 +++++++++++++++++++
 gcc/testsuite/gnat.dg/sync_tag_limited.adb    | 50 ++++++++++++++++++
 3 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
 create mode 100644 gcc/testsuite/gnat.dg/sync_tag_limited.adb

diff --git a/gcc/ada/sem_ch3.adb b/gcc/ada/sem_ch3.adb
index 3262236dd14..92902a7debb 100644
--- a/gcc/ada/sem_ch3.adb
+++ b/gcc/ada/sem_ch3.adb
@@ -9599,9 +9599,15 @@ package body Sem_Ch3 is
 
       --  AI-419: Limitedness is not inherited from an interface parent, so to
       --  be limited in that case the type must be explicitly declared as
-      --  limited. However, task and protected interfaces are always limited.
-
-      if Limited_Present (Type_Def) then
+      --  limited, or synchronized. While task and protected interfaces are
+      --  always limited, a synchronized private extension might not inherit
+      --  from such interfaces, and so we also need to recognize the
+      --  explicit limitedness implied by a synchronized private extension
+      --  that does not derive from a synchronized interface (see RM-7.3(6/2)).
+
+      if Limited_Present (Type_Def)
+        or else Synchronized_Present (Type_Def)
+      then
          Set_Is_Limited_Record (Derived_Type);
 
       elsif Is_Limited_Record (Parent_Type)
diff --git a/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb b/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
new file mode 100644
index 00000000000..b105acf6e98
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/sync_tag_discriminals.adb
@@ -0,0 +1,51 @@
+-- This test is related to sync_tag_limited in that previous versions of GNAT
+-- failed to consider a synchronized private extension as limited if it was
+-- not derrived from a synchronized interface (i.e. a limited interface). Since
+-- such a private type would not be considered limited, GNAT would fail to
+-- correctly build the expected discriminals later needed by the creation of
+-- the concurrent type's "corresponding record type", leading to a compilation
+-- error where the discriminants of the corresponding record type had no
+-- identifiers.
+--
+-- This test is in addition to sync_tag_limited because the sync_tag_limited
+-- would fail for "legality" reasons (default discriminants not allowed for
+-- a non-limited taged type). It is also an opportunity to ensure that non-
+-- defaulted discriminated synchronized private extensions work as expected.
+
+--  { dg-do compile }
+
+procedure Sync_Tag_Discriminals is
+   
+   package Ifaces is
+      
+      type Test_Interface is limited interface;
+      
+      procedure Interface_Action (Test: in out Test_Interface) is abstract;
+      
+   end Ifaces;
+   
+   
+   package Implementation is
+      type Test_Implementation
+        (Constraint: Positive) is
+        synchronized new Ifaces.Test_Interface with private;
+      
+   private
+      protected type Test_Implementation
+        (Constraint: Positive)
+      is new Ifaces.Test_Interface with
+      
+         overriding procedure Interface_Action;
+         
+      end Test_Implementation;
+   end Implementation;
+   
+   package body Implementation is
+      protected body Test_Implementation is
+         procedure Interface_Action is null;
+      end;
+   end Implementation;
+   
+begin
+   null;
+end Sync_Tag_Discriminals;
diff --git a/gcc/testsuite/gnat.dg/sync_tag_limited.adb b/gcc/testsuite/gnat.dg/sync_tag_limited.adb
new file mode 100644
index 00000000000..608f10662a3
--- /dev/null
+++ b/gcc/testsuite/gnat.dg/sync_tag_limited.adb
@@ -0,0 +1,50 @@
+--  Synchronized tagged types created by a private extension with the keyword
+--  'synchronized' shall be seen as an (immutably) limited tagged type, and
+--  should therefore accept default disciminant spectifications.
+--  This was a bug in earlier versions of GNAT, whereby GNAT erroneously
+--  relied on a parent synchronized interface to determine limitedness
+--  of a synchronized private extension. The problem being that a synchronized
+--  private extension can derive a non-synchronized interface (specifically a
+--  limited interface), Yet the RM makes it clear (7.3(6/2)) that such
+--  synchronized private extensions are always limited.
+--
+--  Ergo: Default discriminants are of course legal on any synchronized private
+--  extension.
+
+--  { dg-do compile }
+
+procedure Sync_Tag_Limited is
+   
+   package Ifaces is
+      
+      type Test_Interface is limited interface;
+      
+      procedure Interface_Action (Test: in out Test_Interface) is abstract;
+      
+   end Ifaces;
+   
+   
+   package Implementation is
+      type Test_Implementation
+        (Constraint: Positive := 1) is
+        synchronized new Ifaces.Test_Interface with private;
+      
+   private
+      protected type Test_Implementation
+        (Constraint: Positive := 1)
+      is new Ifaces.Test_Interface with
+      
+         overriding procedure Interface_Action;
+         
+      end Test_Implementation;
+   end Implementation;
+   
+   package body Implementation is
+      protected body Test_Implementation is
+         procedure Interface_Action is null;
+      end;
+   end Implementation;
+   
+begin
+   null;
+end Sync_Tag_Limited;
-- 
2.40.0