[PATCH] Fortran : Two further previously missed ICEs PR53298

[Mark Eggleston]

On 16/09/2020 08:02, Andre Vehreschild wrote:
> Hi Mark,
>
> a few remarks:
>
> [...]
>
>> [PATCH] Fortran  : Two further previously missed ICEs PR53298
>>
>> There were 3 ICEs with different call stacks in the comments of this
>> PR.  A previous commit fixed only one of those ICEs.
>>
>> The ICEs fixed here are in trans-array.c and trans-expr.c.
>>
>> The first ICE occurred when the array reference is not AR_ELEMENT
>> gfc_conv_scalarized_array_ref is called with se and ar, if se->ss is
>> NULL the ICE occurs.  If se->ss is NULL there is nothing to do before
>> the return.
>>
>> The second ICE occurs in code that did not match its comments.  Fixing
>> the code to match the comments fixes the ICE.  A side affect is that
>> the in the tree dumps for finalize_35.f90 and finalize_36.f90 contain
>    ^^^
>    Spurious "the" found.
wording has been updated.
>
> [...]
>
> diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c
> index 6566c47d4ae..06268739515 100644
> --- a/gcc/fortran/trans-array.c
> +++ b/gcc/fortran/trans-array.c
> @@ -3638,8 +3638,11 @@ gfc_conv_array_ref (gfc_se * se, gfc_array_ref *
> ar, gfc_expr *expr, /* Handle scalarized references separately.  */
>     if (ar->type != AR_ELEMENT)
>       {
> -      gfc_conv_scalarized_array_ref (se, ar);
> -      gfc_advance_se_ss_chain (se);
> +      if (se->ss)
> +	{
> +	  gfc_conv_scalarized_array_ref (se, ar);
> +	  gfc_advance_se_ss_chain (se);
> +	}
>
> Why is this only in element ref needed and not every else? When I tried
> to fix ICEs this way, I was usually asked if was fixing symptom and not
> the error.

This is for references that aren't elements. As I understand it se 
corresponds to the upper bound, e.g. for a(1:) the upper bound may not 
be known at this point so se is NULL resulting in a crash due to it 
being de-referenced in gfc_conv_scalarized_array_ref.

>
>         return;
>       }
>   
> diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
> index 36ff9b5cbc6..193553ace0b 100644
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -2474,8 +2474,8 @@ gfc_conv_component_ref (gfc_se * se, gfc_ref *
> ref) RECORD_TYPE within a UNION_TYPE) always use the given FIELD_DECL.
> */
>     if (context != TREE_TYPE (decl)
> -      && !(   TREE_CODE (TREE_TYPE (field)) == UNION_TYPE /* Field is
> union */
> -           || TREE_CODE (context) == UNION_TYPE))         /* Field is
> map */
> +      && (   TREE_CODE (context) == UNION_TYPE /* Field is union */
> +          || TREE_CODE (context) == MAP_TYPE)) /* Field is map */
>       {
>         tree f2 = c->norestrict_decl;
>         if (!f2 || DECL_FIELD_CONTEXT (f2) != TREE_TYPE (decl))
>
> (Sorry for the line breaks).
>
> I can't help it, but the old code looked so dubious that I wonder why
> it worked in the first place. Have you tried with a mapped type?
structure /st/
   union
     map
       character(5) a, b
     end map
     map
       character(10) c
     end map
   end union
end structure

record /st/ r

r.c = "abcde12345"
write(*,*) r.a(2:), r.b(3:), r.c(6:)

end

outputs (as expected) with and without the patch

  bcde34512345

I can add this as an extra test case if required.

>
> Regards,
> 	Andre
>
>
-- 
https://www.codethink.co.uk/privacy.html