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

[Andre Vehreschild]

Hi Mikael,

thanks for the review. I still have some questions/remarks before commiting:

On Thu, 07 May 2015 12:14:59 +0200
Mikael Morin <mikael.morin@sfr.fr> wrote:
<snip>
> > @@ -2158,6 +2158,8 @@ build_function_decl (gfc_symbol * sym, bool global)
> >      gfc_set_decl_assembler_name (fndecl, gfc_sym_mangled_function_id
> > (sym)); 
> >    sym->backend_decl = fndecl;
> > +  if (sym == sym->result && !sym->result->backend_decl)
> > +    sym->result->backend_decl = result_decl;
> 
> Something is seriously misbehaving if the condition is true, and setting
> sym->backend_decl to result_decl doesn't seem any better than keeping it
> NULL.
> So, please remove this change

Did that. I think this was a relic from the start of me trying to understand
what was the issue and how to fix it. Later I didn't check, if it was still
necessary. Sorry for that.

> > @@ -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?

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. 

Attached is a version of the patch that I currently use. Note the testcase
alloc_comp_class_4.f03 fails currently, because of the error noted above in
line 94.

Regards,
	Andre
-- 
Andre Vehreschild * Email: vehre ad gmx dot de 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr58586_4.patch
Type: text/x-patch
Size: 9368 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20150508/52ea6b6a/attachment.bin>