[Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Tobias Burnus]

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

See PR for some analysis. The problem is that during
gfc_intrinsic_func_interface, sym->attr.flavor == FL_PROCEDURE,
hence, attr.intrinsic is not set – but later when parsing
'null()', gfortran calls:

   if (sym->attr.proc != PROC_INTRINSIC
       && !(sym->attr.use_assoc && sym->attr.intrinsic)
       && (!gfc_add_procedure(&sym->attr, PROC_INTRINSIC, sym->name, NULL)
           || !gfc_add_function (&sym->attr, sym->name, NULL)))
     return MATCH_ERROR;

The gfc_add_procedure call fails as 'sym' is use-associated and
may not be modified.

The obvious solution to also set attr.intrinsic for FL_PROCEDURE fails
in multiple ways, e.g. for gfortran.dg/char_length_16.f90 which has:
   CHARACTER (LEN(ITEMVAL)) :: ITEM
   INTRINSIC LEN
the error is that INTRINSIC has been speicified twice. It also affects
the error diagnostic in for generic resolution due to  (I think):
   if (sym->attr.intrinsic)
     return gfc_intrinsic_func_interface (expr, 0);
For gfortran.dg/allocatable_scalar_11.f90 the specific
   ‘array’ argument of ‘allocated’ intrinsic at (1) must be a variable
gets replaced by the generic
   Generic function 'allocated' at (1) is not consistent with a specific intrinsic interface

I have now tried it as shown. I think attr.function was not set, but
am not sure. But setting it again for FL_PROCEDURE looks sensible.

I am far from certain that now everything fits together, but I do hope
that nothing fails which did work before ...

OK for mainline? And after waiting a while for GCC 10?

Tobias

PS: I did see once a fail for pr93792.f90 (additional error as 't' in type(t)
is not known) – but I could not reproduce it; the error is valid but later runs
stopped with 'cannot open module file' and did not reach that follow-up error.

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

[-- Attachment #2: null-fix.diff --]
[-- Type: text/x-patch, Size: 1428 bytes --]

Fortran: Fix intrinsic null() handling [PR99651]

gcc/fortran/ChangeLog:

	PR fortran/99651
	* intrinsic.c (gfc_intrinsic_func_interface): Set
	attr.proc = PROC_INTRINSIC if FL_PROCEDURE.

gcc/testsuite/ChangeLog:

	PR fortran/99651
	* gfortran.dg/null_11.f90: New test.

 gcc/fortran/intrinsic.c               |  5 +++++
 gcc/testsuite/gfortran.dg/null_11.f90 | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/gcc/fortran/intrinsic.c b/gcc/fortran/intrinsic.c
index e68eff8bdbb..17fd92eb462 100644
--- a/gcc/fortran/intrinsic.c
+++ b/gcc/fortran/intrinsic.c
@@ -5071,6 +5071,11 @@ got_specific:
       sym->attr.intrinsic = 1;
       sym->attr.flavor = FL_PROCEDURE;
     }
+  if (sym->attr.flavor == FL_PROCEDURE)
+    {
+      sym->attr.function = 1;
+      sym->attr.proc = PROC_INTRINSIC;
+    }
 
   if (!sym->module)
     gfc_intrinsic_symbol (sym);
diff --git a/gcc/testsuite/gfortran.dg/null_11.f90 b/gcc/testsuite/gfortran.dg/null_11.f90
new file mode 100644
index 00000000000..040cc260b5d
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/null_11.f90
@@ -0,0 +1,16 @@
+! { dg-do compile }
+!
+! PR fortran/99651
+!
+module m
+    type :: CHAR_STAR
+      character(len=1),dimension(:),pointer :: ptr
+    end type
+    type(CHAR_STAR), parameter ::CHAR_STAR_NULL = CHAR_STAR(NULL())
+end module m
+
+use m
+type typeNode
+    type(typeNode),   pointer       :: Next => null()
+end type typeNode
+end

Re: [Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Paul Richard Thomas]

Hi Tobias,

I took something of a detour in reviewing this patch. Although short,
understanding it is not straightforward!

Your patch works as advertised and regtests OK (with the patch for PR93660
on board as well). Is NULL the only case where this can happen?
Just to aid my understanding, I tried:
diff --git a/gcc/fortran/primary.c b/gcc/fortran/primary.c
index a6df885c80c..f4c43a7c38b 100644
--- a/gcc/fortran/primary.c
+++ b/gcc/fortran/primary.c
@@ -3922,6 +3922,9 @@ gfc_match_rvalue (gfc_expr **result)
       if (m == MATCH_NO)
        m = MATCH_YES;

+      if (!strcmp (sym->name, "NULL") || !strcmp (sym->name, "null"))
+       sym->attr.intrinsic = 1;
+
       break;

     generic_function:

which also works and regtests OK. (I couldn't remember whether sym->name is
upper or lower case at this stage.)

Thus, I THINK that your patch is OK and haven't managed to find any tests
which it breaks. I'll come back with a more definitive opinion tomorrow.

Paul



On Fri, 19 Mar 2021 at 08:51, Tobias Burnus <tobias@codesourcery.com> wrote:

> See PR for some analysis. The problem is that during
> gfc_intrinsic_func_interface, sym->attr.flavor == FL_PROCEDURE,
> hence, attr.intrinsic is not set – but later when parsing
> 'null()', gfortran calls:
>
>    if (sym->attr.proc != PROC_INTRINSIC
>        && !(sym->attr.use_assoc && sym->attr.intrinsic)
>        && (!gfc_add_procedure(&sym->attr, PROC_INTRINSIC, sym->name, NULL)
>            || !gfc_add_function (&sym->attr, sym->name, NULL)))
>      return MATCH_ERROR;
>
> The gfc_add_procedure call fails as 'sym' is use-associated and
> may not be modified.
>
> The obvious solution to also set attr.intrinsic for FL_PROCEDURE fails
> in multiple ways, e.g. for gfortran.dg/char_length_16.f90 which has:
>    CHARACTER (LEN(ITEMVAL)) :: ITEM
>    INTRINSIC LEN
> the error is that INTRINSIC has been speicified twice. It also affects
> the error diagnostic in for generic resolution due to  (I think):
>    if (sym->attr.intrinsic)
>      return gfc_intrinsic_func_interface (expr, 0);
> For gfortran.dg/allocatable_scalar_11.f90 the specific
>    ‘array’ argument of ‘allocated’ intrinsic at (1) must be a variable
> gets replaced by the generic
>    Generic function 'allocated' at (1) is not consistent with a specific
> intrinsic interface
>
> I have now tried it as shown. I think attr.function was not set, but
> am not sure. But setting it again for FL_PROCEDURE looks sensible.
>
> I am far from certain that now everything fits together, but I do hope
> that nothing fails which did work before ...
>
> OK for mainline? And after waiting a while for GCC 10?
>
> Tobias
>
> PS: I did see once a fail for pr93792.f90 (additional error as 't' in
> type(t)
> is not known) – but I could not reproduce it; the error is valid but later
> runs
> stopped with 'cannot open module file' and did not reach that follow-up
> error.
>
> -----------------
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
> Thürauf
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein

Re: [Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Paul Richard Thomas]

Cancel the thought on my patchlet null_5.f90 fails on excess errors.

Paul


On Tue, 23 Mar 2021 at 17:34, Paul Richard Thomas <
paul.richard.thomas@gmail.com> wrote:

> Hi Tobias,
>
> I took something of a detour in reviewing this patch. Although short,
> understanding it is not straightforward!
>
> Your patch works as advertised and regtests OK (with the patch for PR93660
> on board as well). Is NULL the only case where this can happen?
> Just to aid my understanding, I tried:
> diff --git a/gcc/fortran/primary.c b/gcc/fortran/primary.c
> index a6df885c80c..f4c43a7c38b 100644
> --- a/gcc/fortran/primary.c
> +++ b/gcc/fortran/primary.c
> @@ -3922,6 +3922,9 @@ gfc_match_rvalue (gfc_expr **result)
>        if (m == MATCH_NO)
>         m = MATCH_YES;
>
> +      if (!strcmp (sym->name, "NULL") || !strcmp (sym->name, "null"))
> +       sym->attr.intrinsic = 1;
> +
>        break;
>
>      generic_function:
>
> which also works and regtests OK. (I couldn't remember whether sym->name
> is upper or lower case at this stage.)
>
> Thus, I THINK that your patch is OK and haven't managed to find any tests
> which it breaks. I'll come back with a more definitive opinion tomorrow.
>
> Paul
>
>
>
> On Fri, 19 Mar 2021 at 08:51, Tobias Burnus <tobias@codesourcery.com>
> wrote:
>
>> See PR for some analysis. The problem is that during
>> gfc_intrinsic_func_interface, sym->attr.flavor == FL_PROCEDURE,
>> hence, attr.intrinsic is not set – but later when parsing
>> 'null()', gfortran calls:
>>
>>    if (sym->attr.proc != PROC_INTRINSIC
>>        && !(sym->attr.use_assoc && sym->attr.intrinsic)
>>        && (!gfc_add_procedure(&sym->attr, PROC_INTRINSIC, sym->name, NULL)
>>            || !gfc_add_function (&sym->attr, sym->name, NULL)))
>>      return MATCH_ERROR;
>>
>> The gfc_add_procedure call fails as 'sym' is use-associated and
>> may not be modified.
>>
>> The obvious solution to also set attr.intrinsic for FL_PROCEDURE fails
>> in multiple ways, e.g. for gfortran.dg/char_length_16.f90 which has:
>>    CHARACTER (LEN(ITEMVAL)) :: ITEM
>>    INTRINSIC LEN
>> the error is that INTRINSIC has been speicified twice. It also affects
>> the error diagnostic in for generic resolution due to  (I think):
>>    if (sym->attr.intrinsic)
>>      return gfc_intrinsic_func_interface (expr, 0);
>> For gfortran.dg/allocatable_scalar_11.f90 the specific
>>    ‘array’ argument of ‘allocated’ intrinsic at (1) must be a variable
>> gets replaced by the generic
>>    Generic function 'allocated' at (1) is not consistent with a specific
>> intrinsic interface
>>
>> I have now tried it as shown. I think attr.function was not set, but
>> am not sure. But setting it again for FL_PROCEDURE looks sensible.
>>
>> I am far from certain that now everything fits together, but I do hope
>> that nothing fails which did work before ...
>>
>> OK for mainline? And after waiting a while for GCC 10?
>>
>> Tobias
>>
>> PS: I did see once a fail for pr93792.f90 (additional error as 't' in
>> type(t)
>> is not known) – but I could not reproduce it; the error is valid but
>> later runs
>> stopped with 'cannot open module file' and did not reach that follow-up
>> error.
>>
>> -----------------
>> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
>> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
>> Thürauf
>>
>
>
> --
> "If you can't explain it simply, you don't understand it well enough" -
> Albert Einstein
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein

Re: [Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Tobias Burnus]

Hi Paul,

On 23.03.21 18:34, Paul Richard Thomas wrote:
> I took something of a detour in reviewing this patch. Although short,
> understanding it is not straightforward!

I concur – and as I wrote both in the patch email and in the PR, it is
not straight forward which message is showing with which setting.

Actually, I think there are many straight-forward ways to fix it
"properly" but they tend to hide some nicer error messages in favour of
a more generic one. Only by taking into account all the diagnostic and
hidden/delayed diagnostic, the patch becomes complex. (-:

> Your patch works as advertised and regtests OK (with the patch for
> PR93660 on board as well). Is NULL the only case where this can happen?

I am not sure whether any other code would profit form getting one of
the attributes conditionally assigned in intrinsic.c.

Otherwise, I think that's the only code which verifies like that it in
such a way whether an intrinsic function was used. I think the reason
that it does so is because 'null' is turned early during parsing into
EXPR_NULL, which is not a function and, hence, bypasses some of the
later checking code in resolve.c.


> Just to aid my understanding, I tried:
> diff --git a/gcc/fortran/primary.c b/gcc/fortran/primary.c
>
> --- a/gcc/fortran/primary.c
> +++ b/gcc/fortran/primary.c
> @@ -3922,6 +3922,9 @@ gfc_match_rvalue (gfc_expr **result)
>        if (m == MATCH_NO)
>         m = MATCH_YES;
>
> +      if (!strcmp (sym->name, "NULL") || !strcmp (sym->name, "null"))
> +       sym->attr.intrinsic = 1;
> +
>        break;
>
>      generic_function:
>
> which also works and regtests OK. (I couldn't remember whether
> sym->name is upper or lower case at this stage.)

Should be always lowercase, I think. But I am also not sure that your
aid-understanding patch will work correctly with 'external null' or a
use-/host-associated/interface 'null' procedure or some array variable.
Inside intrinsic.c, we are at least sure that we did get an intrinsic
function after having passed all intrinsic checks.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf

Re: [Patch] Fortran: Fix intrinsic null() handling [PR99651]

[Paul Richard Thomas]

Hi Tobias,

Please go ahead and commit the patch. I think that your analysis is correct
about expr_null and that your patch is the best way to deal with the
problem.

Best regards

Paul


On Tue, 23 Mar 2021 at 17:54, Tobias Burnus <tobias@codesourcery.com> wrote:

> Hi Paul,
>
> On 23.03.21 18:34, Paul Richard Thomas wrote:
> > I took something of a detour in reviewing this patch. Although short,
> > understanding it is not straightforward!
>
> I concur – and as I wrote both in the patch email and in the PR, it is
> not straight forward which message is showing with which setting.
>
> Actually, I think there are many straight-forward ways to fix it
> "properly" but they tend to hide some nicer error messages in favour of
> a more generic one. Only by taking into account all the diagnostic and
> hidden/delayed diagnostic, the patch becomes complex. (-:
>
> > Your patch works as advertised and regtests OK (with the patch for
> > PR93660 on board as well). Is NULL the only case where this can happen?
>
> I am not sure whether any other code would profit form getting one of
> the attributes conditionally assigned in intrinsic.c.
>
> Otherwise, I think that's the only code which verifies like that it in
> such a way whether an intrinsic function was used. I think the reason
> that it does so is because 'null' is turned early during parsing into
> EXPR_NULL, which is not a function and, hence, bypasses some of the
> later checking code in resolve.c.
>
>
> > Just to aid my understanding, I tried:
> > diff --git a/gcc/fortran/primary.c b/gcc/fortran/primary.c
> >
> > --- a/gcc/fortran/primary.c
> > +++ b/gcc/fortran/primary.c
> > @@ -3922,6 +3922,9 @@ gfc_match_rvalue (gfc_expr **result)
> >        if (m == MATCH_NO)
> >         m = MATCH_YES;
> >
> > +      if (!strcmp (sym->name, "NULL") || !strcmp (sym->name, "null"))
> > +       sym->attr.intrinsic = 1;
> > +
> >        break;
> >
> >      generic_function:
> >
> > which also works and regtests OK. (I couldn't remember whether
> > sym->name is upper or lower case at this stage.)
>
> Should be always lowercase, I think. But I am also not sure that your
> aid-understanding patch will work correctly with 'external null' or a
> use-/host-associated/interface 'null' procedure or some array variable.
> Inside intrinsic.c, we are at least sure that we did get an intrinsic
> function after having passed all intrinsic checks.
>
> Tobias
>
> -----------------
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
> Thürauf
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein