[Patch, Fortran, PR58586, v3] ICE with derived type with allocatable component passed by value
Mikael Morin
mikael.morin@sfr.fr
Fri May 8 13:21:00 GMT 2015
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
More information about the Gcc-patches
mailing list