[Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Tobias Burnus]

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

Dear all,

Due to copying the attributes, the temporary variable could get marked 
as function (attr.function, attr.flavor == FL_PROCEDURE). This either 
lead to leaking those attributes into the assembler file - or to cause 
an error due to the call to gfc_add_flavor. With this patch, I now 
explicitly unset those attribues.  (Fund when building ForTrilinos.)

Build and
OK for the trunk and GCC 4.8?

Tobias

[-- Attachment #2: def-assignm.diff --]
[-- Type: text/x-patch, Size: 2059 bytes --]

2013-06-03  Tobias Burnus  <burnus@net-b.de>

	PR fortran/57508
	* resolve.c (get_temp_from_expr): Don't copy function
	result attributes to temporary.

2013-06-03  Tobias Burnus  <burnus@net-b.de>

	PR fortran/57508
	* gfortran.dg/defined_assignment_7.f90: New.

diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index b2e8fdc..655d3c1 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -9293,8 +9293,12 @@ get_temp_from_expr (gfc_expr *e, gfc_namespace *ns)
 	}
     }
 
   /* Add the attributes and the arrayspec to the temporary.  */
   /* Add the attributes and the arrayspec to the temporary.  */
   tmp->n.sym->attr = gfc_expr_attr (e);
+  tmp->n.sym->attr.function = 0;
+  tmp->n.sym->attr.result = 0;
+  tmp->n.sym->attr.flavor = FL_VARIABLE;
+
   if (as)
     {
       tmp->n.sym->as = gfc_copy_array_spec (as);
@@ -9307,7 +9311,6 @@ get_temp_from_expr (gfc_expr *e, gfc_namespace *ns)
     tmp->n.sym->attr.dimension = 0;
 
   gfc_set_sym_referenced (tmp->n.sym);
-  gfc_add_flavor (&tmp->n.sym->attr, FL_VARIABLE, name, NULL);
   gfc_commit_symbol (tmp->n.sym);
   e = gfc_lval_expr_from_sym (tmp->n.sym);
 
--- /dev/null	2013-06-03 08:35:13.011105509 +0200
+++ gcc/gcc/testsuite/gfortran.dg/defined_assignment_7.f90	2013-06-03 15:58:17.227408173 +0200
@@ -0,0 +1,29 @@
+! { dg-compile }
+!
+! PR fortran/57508
+!
+module ForTrilinos_ref_counter
+  type ref_counter
+  contains
+      procedure :: assign
+      generic   :: assignment(=) => assign
+  end type
+contains
+  subroutine assign (lhs, rhs)
+    class (ref_counter), intent(inout) :: lhs
+    class (ref_counter), intent(in) :: rhs
+  end subroutine
+end module
+module FEpetra_BlockMap
+  use ForTrilinos_ref_counter, only : ref_counter
+  type :: Epetra_BlockMap 
+    type(ref_counter) :: counter
+  end type
+contains
+  function from_struct() result(new_Epetra_BlockMap)
+    type(Epetra_BlockMap) :: new_Epetra_BlockMap
+  end function
+  type(Epetra_BlockMap) function create_arbitrary()
+    create_arbitrary = from_struct()
+  end function
+end module

*ping* / Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Tobias Burnus]

Early *ping*.
http://gcc.gnu.org/ml/fortran/2013-06/msg00027.html

Tobias Burnus wrote:
> Dear all,
>
> Due to copying the attributes, the temporary variable could get marked 
> as function (attr.function, attr.flavor == FL_PROCEDURE). This either 
> lead to leaking those attributes into the assembler file - or to cause 
> an error due to the call to gfc_add_flavor. With this patch, I now 
> explicitly unset those attribues.  (Fund when building ForTrilinos.)
>
> Build and
> OK for the trunk and GCC 4.8?
>
> Tobias

Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Mikael Morin]

Hello,

Le 03/06/2013 16:06, Tobias Burnus a écrit :
> Dear all,
> 
> Due to copying the attributes, the temporary variable could get marked
> as function (attr.function, attr.flavor == FL_PROCEDURE). This either
> lead to leaking those attributes into the assembler file - or to cause
> an error due to the call to gfc_add_flavor. With this patch, I now
> explicitly unset those attribues.  (Fund when building ForTrilinos.)
> 

> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
> index b2e8fdc..655d3c1 100644
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -9293,8 +9293,12 @@ get_temp_from_expr (gfc_expr *e, gfc_namespace *ns)
>  	}
>      }
>  
>    /* Add the attributes and the arrayspec to the temporary.  */
>    /* Add the attributes and the arrayspec to the temporary.  */
>    tmp->n.sym->attr = gfc_expr_attr (e);
> +  tmp->n.sym->attr.function = 0;
> +  tmp->n.sym->attr.result = 0;
> +  tmp->n.sym->attr.flavor = FL_VARIABLE;
> +
>    if (as)
>      {
>        tmp->n.sym->as = gfc_copy_array_spec (as);

This fixes the problem, but shouldn't the fix be in gfc_expr_attr instead?
It seems to me that most symbol attributes don't make sense in any case
for non-variables, except for some of the standard ones
(allocatable,...) and possibly a couple more.

Mikael

Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Tobias Burnus]

Hello Mikael,

Mikael Morin wrote:
> Le 03/06/2013 16:06, Tobias Burnus a écrit :
>> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
>> index b2e8fdc..655d3c1 100644
>> --- a/gcc/fortran/resolve.c
>> +++ b/gcc/fortran/resolve.c
>> @@ -9293,8 +9293,12 @@ get_temp_from_expr (gfc_expr *e, gfc_namespace *ns)
>>   	}
>>       }
>>   
>>     /* Add the attributes and the arrayspec to the temporary.  */
>>     /* Add the attributes and the arrayspec to the temporary.  */
>>     tmp->n.sym->attr = gfc_expr_attr (e);
>> +  tmp->n.sym->attr.function = 0;
>> +  tmp->n.sym->attr.result = 0;
>> +  tmp->n.sym->attr.flavor = FL_VARIABLE;
>> +
>>     if (as)
>>       {
>>         tmp->n.sym->as = gfc_copy_array_spec (as);
> This fixes the problem, but shouldn't the fix be in gfc_expr_attr instead?
> It seems to me that most symbol attributes don't make sense in any case
> for non-variables, except for some of the standard ones
> (allocatable,...) and possibly a couple more.

I will audit the use of gfc_expr_attr and send an updated patch later.

Is the current patch okay for GCC 4.8? I prefer simpler patches for the 
branch.

Tobias

PS: Still pending review: [Patch, Fortran] PR57535 - Fix class-array 
handling for function result variables, 
http://gcc.gnu.org/ml/fortran/2013-06/msg00053.html

Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Mikael Morin]

Le 11/06/2013 12:00, Tobias Burnus a écrit :
> Hello Mikael,
> 
> Mikael Morin wrote:
>> Le 03/06/2013 16:06, Tobias Burnus a écrit :
>>> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
>>> index b2e8fdc..655d3c1 100644
>>> --- a/gcc/fortran/resolve.c
>>> +++ b/gcc/fortran/resolve.c
>>> @@ -9293,8 +9293,12 @@ get_temp_from_expr (gfc_expr *e, gfc_namespace
>>> *ns)
>>>       }
>>>       }
>>>       /* Add the attributes and the arrayspec to the temporary.  */
>>>     /* Add the attributes and the arrayspec to the temporary.  */
>>>     tmp->n.sym->attr = gfc_expr_attr (e);
>>> +  tmp->n.sym->attr.function = 0;
>>> +  tmp->n.sym->attr.result = 0;
>>> +  tmp->n.sym->attr.flavor = FL_VARIABLE;
>>> +
>>>     if (as)
>>>       {
>>>         tmp->n.sym->as = gfc_copy_array_spec (as);
>> This fixes the problem, but shouldn't the fix be in gfc_expr_attr
>> instead?
>> It seems to me that most symbol attributes don't make sense in any case
>> for non-variables, except for some of the standard ones
>> (allocatable,...) and possibly a couple more.
> 
> I will audit the use of gfc_expr_attr and send an updated patch later.
> 
> Is the current patch okay for GCC 4.8? I prefer simpler patches for the
> branch.
> 
Yes.

Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Tobias Burnus]

Mikael Morin wrote:
> This fixes the problem, but shouldn't the fix be in gfc_expr_attr instead?

I tried it - but it does not work: In many case, one actually needs a 
function, e.g. for procedure pointers or for C_FUNLOC. Thus, I had to 
add an additional flag to tell whether the function or the function 
result it needed. But instead of adding a Boolean flag to 55 calls, 
which can be false in 54 case and true in 1, I think that the original 
patch is better. It's the only case where not an attribute it checked - 
but where attributes are copied.

Thus, is the original patch okay? Or do you have a better 
proposal?http://gcc.gnu.org/ml/fortran/2013-06/msg00027.html

Tobias

PS: Other pending patches:
* Unreviewed: Print exception status at STOP, 
http://gcc.gnu.org/ml/fortran/2013-06/msg00077.html
* PR57596 - Fix OPTIONAL handling of deferred-length strings, 
http://gcc.gnu.org/ml/fortran/2013-06/msg00082.html

Re: [Patch, Fortran] PR57508 - Fix ICE/Reject-valid issue with get_temp_from_expr (intrinsic assignment with defined assignment)

[Mikael Morin]

Le 13/06/2013 19:56, Tobias Burnus a écrit :
> Mikael Morin wrote:
>> This fixes the problem, but shouldn't the fix be in gfc_expr_attr
>> instead?
> 
> I tried it - but it does not work: In many case, one actually needs a
> function, e.g. for procedure pointers or for C_FUNLOC. Thus, I had to
> add an additional flag to tell whether the function or the function
> result it needed. But instead of adding a Boolean flag to 55 calls,
> which can be false in 54 case and true in 1, I think that the original
> patch is better. It's the only case where not an attribute it checked -
> but where attributes are copied.
> 
> Thus, is the original patch okay? Or do you have a better
> proposal?http://gcc.gnu.org/ml/fortran/2013-06/msg00027.html
> 
Sorry, I didn't see what the problem really was.
Yes, the original patch is OK.

Mikael