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