Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type

[Tobias Burnus <burnus at net-b dot de>]

Janus Weil wrote:
> here is a patch for a pretty bad wrong-code regression, which affects
> all maintained releases of gfortran. For discussion see bugzilla.
>
> 2012-12-15  Janus Weil<janus@gcc.gnu.org>
>      PR fortran/55072
>      * trans-array.c (gfc_conv_array_parameter): No packing was done for
>      full arrays of derived type.
>
> @@ -6995,20 +6995,14 @@ gfc_conv_array_parameter (gfc_se * se, gfc_expr *
>       this_array_result = false;
>   
>     /* Passing address of the array if it is not pointer or assumed-shape.  */
> -  if (full_array_var && g77 && !this_array_result)
> +  if (full_array_var && g77 && !this_array_result
> +      && sym->ts.type != BT_DERIVED && sym->ts.type != BT_CLASS)

Without experimenting more carefully, I have the gut feeling that there 
are still cases where we might end up with invalid code and there is 
missed optimization.


Regarding the latter: If the variable is simply contiguous, there is no 
need to pack it. Hence, for

type(t), allocatable :: a(:)
...
call bar(a)

there is no need to pack the array. The problem with the original test 
case is that one has a non-CONTIGUOUS pointer:

p => tgt(::2,::2)
call bar(p)

But that has in principle nothing to do with BT_DERIVED. In particular, 
I would like to see an additional test case for the first example case 
with "ptr" having the CONTIGUOUS attribute - and a check that then no 
packing call is invoked.


For the second test case (comment 2, from GiBUU): Here, the problem is 
that "full_array_var" is wrongly true:

  call s1(OutPart(1,:))


I wonder whether some call to gfc_is_simply_contiguous could solve the 
problem for both issues.

(For non-whole arrays one still have to ensure that one passes the 
correct element: "call(a)" should pass a->data and not "&a" and "call 
bar(a(:,2))" should neither pass "a->data" nor "&a" but "a->data + offset".)

Regarding BT_CLASS: BT_CLASS -> BT_TYPE (with same declared type) should 
already be handled via gfc_conv_subref_array_arg, which takes care of 
the actual type. Thus, the patched function should only be reachable for 
BT_CLASS -> BT_CLASS. Here, packing is required for non-simply 
contiguous actual arguments; but after the packing, a class container 
has to be re-added. I think one should add a test case for this; testing 
declared type == actual type and declared type != actual type - and 
either one for both declared type being the same and for the dummy 
having the declared type of the ancestor of the declared type of the 
actual argument. And all cases for both simply contiguous arrays and 
(simply - or better actually) noncontiguous arrays.

Regarding the wrong code: I fear that some code involving non-BT_DERIVED 
could lead to wrong code, e.g. "a(:)%x". I don't have an example for 
that but I fear that code which lead to the original issue (e.g. 
"full_array_var" is true although it shouldn't) is not solved via the patch.


Sorry for listing more my concerns that giving a proper review.

Tobias