Re: [Patch, Fortran, PR58586, v3] ICE with derived type with allocatable component passed by value

[Mikael Morin <mikael.morin@sfr.fr>]

Le 08/05/2015 12:54, Andre Vehreschild a écrit :
> Hi Mikael,
> 
> thanks for the review. I still have some questions/remarks before commiting:
> 
>>> @@ -5898,8 +5900,21 @@ gfc_generate_function_code (gfc_namespace * ns)
>>>  
>>>    if (TREE_TYPE (DECL_RESULT (fndecl)) != void_type_node)
>>>      {
>>> +      bool artificial_result_decl = false;
>>>        tree result = get_proc_result (sym);
>>>  
>>> +      /* Make sure that a function returning an object with
>>> +	 alloc/pointer_components always has a result, where at least
>>> +	 the allocatable/pointer components are set to zero.  */
>>> +      if (result == NULL_TREE && sym->attr.function
>>> +	  && sym->ts.type == BT_DERIVED
>>> +	  && (sym->ts.u.derived->attr.alloc_comp
>>> +	      || sym->ts.u.derived->attr.pointer_comp))
>>> +	{
>>> +	  artificial_result_decl = true;
>>> +	  result = gfc_get_fake_result_decl (sym, 0);
>>> +	}
>>
>> I expect the "fake" result decl to be needed in more cases.
>> For example, if type is BT_CLASS.
>> Here is a variant of alloc_comp_class_4.f03:c_init for such a case.
>>
>>   class(c) function c_init2()
>>     allocatable :: c_init2
>>   end function
>>
>> or even without class:
>>
>>   type(t) function t_init()
>>     allocatable :: t_init
>>   end function
>>
>> for some any type t.
>>
>> So, remove the check for alloc_comp/pointer_comp and permit BT_CLASS.
>> One minor thing, check sym->result's type and attribute instead of sym's
>> here.  It should not make a difference, but I think it's more correct.
> 
> I am d'accord with checking sym->result, but I am not happy with removing the
> checks for alloc_comp|pointer_comp. When I got you right there, you propose the
> if to be like this:
> 
>       if (result == NULL_TREE && sym->attr.function
> 	  && (sym->result->ts.type == BT_DERIVED
> 	      || sym->result->ts.type == BT_CLASS))
> 
> Removing the attribute checks means to initialize every derived/class type
> result, which may change the semantics of the code more than intented. Look for
> example at this code
> 
>   type t
>     integer :: i = 5
>   end type
> 
>   type(t) function static_t_init()
>   end function
> 
> When one compiles this code with -Wreturn-type, then the warning of an
> uninitialized return value is issued at the function declaration. Nevertheless
> the result of static_t_init is validly initialized and i is 5. This may
> confuse users.
> 
> I therefore came to the very ugly solution to make this:
> 
>       if (result == NULL_TREE && sym->attr.function
> 	  && ((sym->result->ts.type == BT_DERIVED
> 	       && (sym->results->attr.allocatable
> 		   || sym->result->ts.u.derived->attr.alloc_comp
> 		   || sym->result->ts.u.derived->attr.pointer_comp))
> 	      || (sym->result->ts.type == BT_CLASS
> 		  && (CLASS_DATA (sym->result)->attr.allocatable
> 		      || CLASS_DATA (sym->result)->attr.alloc_comp
> 		      || CLASS_DATA (sym->result)->attr.pointer_comp))))
> 
> (I am not yet sure, whether the pointer attribute needs to be added to.) With
> the code above the result of static_t_init is not initialized with all the
> consequences. 
> 
> So what do you propose to do here?

To be honest, I don't know this part of the code very well.
I'll think about it some more.

> Btw, I think I found an additional bug during testing: 
>   type(t) function t_init()
>     allocatable :: t_init
>   end function
>  
> when called by:
>   type(t), allocatable :: temp
>   temp = t_init()
> 
> a segfault occurs, because the result of t_init() is NULL, which is
> dereferenced by the caller in this pseudo-code:
> 
>   if (temp != 0B) goto L.12;
>   temp = (struct t *) __builtin_malloc (4);
> L.12:;
>   *temp = *t_init (); <-- This obviously is problematic.
> 
>> The rest looks good.
>> The patch is OK with the suggested changes above.  Thanks.
>> I don't think the test functions above work well enough to be
>> incorporated in a testcase for now.
> 
> ?? I don't get you there? What do you mean? Do you think the
> alloc_comp_class_3/4.* are not correctly testing the issue? Any idea of how to
> test this better? I mean the pr is about this artificial constructs. I merely
> struck it in search of a pr about allocatable components. 

I was talking about the bug you found with t_init above.  :-)
the compiler is not ready to accept that function in a testcase.
The alloc_omp_class_3/4 are fine.

Mikael