[PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Richard Wai]

Hi,

 

Discovered this error when attempting to allocate from a
Root_Storage_Pool_With_Subpools derived type. If the type is derived in the
current compilation unit, and Allocate is not overridden on derivation (as
is typically the case with Root_Storage_Pool_With_Subpools), the entity for
Allocate for the derived type is then an alias to
System.Storage_Pools.Subpools.Allocate. When the allocator is built,
gnat_to_gnu_entity is called with definition == false for the derived
storage pool's allocate operation. An assertion is gnat_to_gnu_entity fails
in this case, since it is not a definition, and Is_Public is false. If the
storage pool type was instead derived in a different compilation unit, this
assertion is not triggered since the aliased entity has the Public property.

 

This patch adds an extra check in the assertion (decl.c: gnat_to_gnu_entity)
that the entity has the Aliased property. Also included a comment that
describes the special case as per the description above.

 

Bootstrapped and tested on x86_64-unknown-freebsd12.1 with no regressions.

 

 

diff --git a/gcc/ada/gcc-interface/decl.c b/gcc/ada/gcc-interface/decl.c

index 871a309ab7d..5ea930f4f65 100644

--- a/gcc/ada/gcc-interface/decl.c

+++ b/gcc/ada/gcc-interface/decl.c

@@ -447,6 +447,15 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree
gnu_expr, bool definition)

   /* If we get here, it means we have not yet done anything with this
entity.

      If we are not defining it, it must be a type or an entity that is
defined

     elsewhere or externally, otherwise we should have defined it already.
*/

+

+  /* One exception relates to an entity, typically an inherited operation,

+             which has an alias pointing to the parent's operation. Often
such an

+             aliased entity will also carry with it the Is_Public property
if it was

+             declared in a separate compilation unit, but when a type is
extended

+             within the current unit, the aliased entity will not pass this

+             assertion. It is neither defined (since it is an inherited
operation,

+             and is not Public, since it is within the current compilation
unit. */

+

   gcc_assert (definition

                     || is_type

                     || kind == E_Discriminant

@@ -454,6 +463,7 @@ gnat_to_gnu_entity (Entity_Id gnat_entity, tree
gnu_expr, bool definition)

                     || kind == E_Label

                     || (kind == E_Constant && Present (Full_View
(gnat_entity)))

                     || Is_Public (gnat_entity)

+                   || Present (Alias (gnat_entity))

                     || type_annotate_only);

   /* Get the name of the entity and set up the line number and filename of

 

 

Thanks,

 

Richard Wai

ANNEXI-STRAYLINE

 

 

 

Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Eric Botcazou]

> Discovered this error when attempting to allocate from a
> Root_Storage_Pool_With_Subpools derived type. If the type is derived in the
> current compilation unit, and Allocate is not overridden on derivation (as
> is typically the case with Root_Storage_Pool_With_Subpools), the entity for
> Allocate for the derived type is then an alias to
> System.Storage_Pools.Subpools.Allocate. When the allocator is built,
> gnat_to_gnu_entity is called with definition == false for the derived
> storage pool's allocate operation. An assertion is gnat_to_gnu_entity fails
> in this case, since it is not a definition, and Is_Public is false. If the
> storage pool type was instead derived in a different compilation unit, this
> assertion is not triggered since the aliased entity has the Public property.

We need a testcase here, we cannot relax assertions without testcases.

> This patch adds an extra check in the assertion (decl.c: gnat_to_gnu_entity)
> that the entity has the Aliased property. Also included a comment that
> describes the special case as per the description above.

I don't really understand the new condition, did you forget to test Is_Public?

-- 
Eric Botcazou

RE: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Richard Wai]

Richard Wai
Managing Director
T. 416.316.9806


> -----Original Message-----
> From: Eric Botcazou <ebotcazou@adacore.com>
> Sent: March 3, 2020 3:50 PM
> To: Richard Wai <richard@annexi-strayline.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased
entities
> 
> > Discovered this error when attempting to allocate from a
> > Root_Storage_Pool_With_Subpools derived type. If the type is derived
> > in the current compilation unit, and Allocate is not overridden on
> > derivation (as is typically the case with
> > Root_Storage_Pool_With_Subpools), the entity for Allocate for the
> > derived type is then an alias to
> > System.Storage_Pools.Subpools.Allocate. When the allocator is built,
> > gnat_to_gnu_entity is called with definition == false for the derived
> > storage pool's allocate operation. An assertion is gnat_to_gnu_entity
> > fails in this case, since it is not a definition, and Is_Public is
> > false. If the storage pool type was instead derived in a different
> compilation unit, this assertion is not triggered since the aliased entity
has
> the Public property.
> 
> We need a testcase here, we cannot relax assertions without testcases.
> 

I'll have to look into this.. Any pointers? This assertion is not a language
rule assertion.

> > This patch adds an extra check in the assertion (decl.c:
> > gnat_to_gnu_entity) that the entity has the Aliased property. Also
> > included a comment that describes the special case as per the
description
> above.
> 
> I don't really understand the new condition, did you forget to test
Is_Public?
> 

As you see, the assertion being modified already tests for "Is_Public". So
the issue is precisely that this assertion wrongly fails for cases where the
entity is not Public. The specific case we ran into is where you have a
derived Root_Storage_Pool_With_Subpools type where Allocate (resp.
Deallocate) is inherited. If that derived type is anywhere except a package
specification, The N_Defining_Identifier for allocate for the derived
subpool will both be an alias to System.Storage_Pools.Subpools.Allocate, and
will NOT be Public. It will then cause this assertion to fail upon building
of the allocator or deallocator.

> --
> Eric Botcazou 

Richard

Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Eric Botcazou]

> I'll have to look into this.. Any pointers? This assertion is not a language
> rule assertion.

Of course, neither of the 117 assertions in gcc-interface is, instead they are 
assertions meant to prevent wrong-code generation from occuring.

> As you see, the assertion being modified already tests for "Is_Public". So
> the issue is precisely that this assertion wrongly fails for cases where the
> entity is not Public.

We cannot let anything go through if there is not Is_Public set somewhere, 
possibly on the Alias (gnat_entity) since it is present in your case.

-- 
Eric Botcazou

RE: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Richard Wai]

> -----Original Message-----
> From: Eric Botcazou <ebotcazou@adacore.com>
> Sent: March 4, 2020 6:18 AM
> To: Richard Wai <richard@annexi-strayline.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased
entities
> 
> > I'll have to look into this.. Any pointers? This assertion is not a
> > language rule assertion.
> 
> Of course, neither of the 117 assertions in gcc-interface is, instead they
are
> assertions meant to prevent wrong-code generation from occuring.
> 

Please excuse my ignorance as this is my first (and hopefully not last)
patch submission.. But I don't see any testcases in the Ada testsuite except
for the (outdated) ACATS tests, which doesn't cover this assertion. So I'm
honestly not sure how I should go about that..

> > As you see, the assertion being modified already tests for
> > "Is_Public". So the issue is precisely that this assertion wrongly
> > fails for cases where the entity is not Public.
> 
> We cannot let anything go through if there is not Is_Public set somewhere,
> possibly on the Alias (gnat_entity) since it is present in your case.
>  

My reasoning there was that at decl.c:3914 when the E_Function/E_Procedure
that has alias is resolved, there is a recursive call to gnat_to_gnu_entity
on the alias. So if the alias was not Public, the same assertion will be
triggered on that recursive call, so there seems to be no need to check it
at that point. Though I suppose that could leave some holes if the incoming
entity is not a subprogram!

Shall I add the Is_Public check to the Alias and resubmit the patch?

Richard Wai
ANNEXI-STRAYLINE

Re: [PATCH] Ada: gcc-interface: fixed assertion for aliased entities

[Eric Botcazou]

> Please excuse my ignorance as this is my first (and hopefully not last)
> patch submission.. But I don't see any testcases in the Ada testsuite except
> for the (outdated) ACATS tests, which doesn't cover this assertion. So I'm
> honestly not sure how I should go about that..

See testsuite/gnat.dg, there are around 3000 regressions tests.

> Shall I add the Is_Public check to the Alias and resubmit the patch?

Yes, please, but it can only be approved with an associated testcase.

-- 
Eric Botcazou