[Fortran] PR 92050 - fix ICE with -fcheck=all

Checking produces extra code (unsurprisingly); this code needs to end up 
in the program…

Bootstrapped on x86-64_gnu-linux. OK for the trunk?

Tobias

On Fri, Oct 11, 2019 at 12:17:49PM +0200, Tobias Burnus wrote:
> Checking produces extra code (unsurprisingly); this code needs to end up 
> in the program…
> 
> Bootstrapped on x86-64_gnu-linux. OK for the trunk?
> 

OK.

--
Steve

Hi all,

I was asked to backport this fix to the GCC 9 branch – given that 
-fcheck=bounds is a common option and it fails with code like 
genecode.org. Given that ICEs with -fcheck are a regression and that the 
patch is not that invasive, I am inclined to accept it.

Comments/(dis)approvals?

Tobias

On 10/11/19 12:17 PM, Tobias Burnus wrote:
> Checking produces extra code (unsurprisingly); this code needs to end 
> up in the program…
>
> Bootstrapped on x86-64_gnu-linux. OK for the trunk?
>
> Tobias
>
>
> pr92050.diff
>
> diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c
> index 965ab7786a1..65238ff623d 100644
> --- a/gcc/fortran/trans-expr.c
> +++ b/gcc/fortran/trans-expr.c
> @@ -7031,8 +7031,11 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym,
>   	gfc_allocate_lang_decl (result);
>         GFC_DECL_SAVED_DESCRIPTOR (result) = parmse.expr;
>         gfc_free_expr (class_expr);
> -      gcc_assert (parmse.pre.head == NULL_TREE
> -		  && parmse.post.head == NULL_TREE);
> +      /* -fcheck= can add diagnostic code, which has to be placed before
> +	 the call. */
> +      if (parmse.pre.head != NULL)
> +	  gfc_add_expr_to_block (&se->pre, parmse.pre.head);
> +      gcc_assert (parmse.post.head == NULL_TREE);
>       }
>   
>     /* Follow the function call with the argument post block.  */
> diff --git a/gcc/testsuite/gfortran.dg/pr92050.f90 b/gcc/testsuite/gfortran.dg/pr92050.f90
> new file mode 100644
> index 00000000000..64193878d8f
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/pr92050.f90
> @@ -0,0 +1,53 @@
> +! { dg-do run }
> +! { dg-options "-fcheck=all" }
> +! { dg-shouldfail "above upper bound" }
> +!
> +! PR fortran/92050
> +!
> +!
> +module buggy
> +  implicit none (type, external)
> +
> +  type :: par
> +  contains
> +    procedure, public :: fun => fun_par
> +  end type par
> +
> +  type comp
> +    class(par), allocatable :: p
> +  end type comp
> +
> +  type foo
> +    type(comp), allocatable :: m(:)
> +  end type foo
> +
> +contains
> +
> +  function fun_par(this)
> +    class(par) :: this
> +    integer    :: fun_par(1)
> +    fun_par = 42
> +  end function fun_par
> +
> +  subroutine update_foo(this)
> +    class(foo) :: this
> +    write(*,*) this%m(1)%p%fun()
> +  end subroutine update_foo
> +
> +  subroutine bad_update_foo(this)
> +    class(foo) :: this
> +    write(*,*) this%m(2)%p%fun()
> +  end subroutine bad_update_foo
> +end module buggy
> +
> +program main
> +  use buggy
> +  implicit none (type, external)
> +  type(foo) :: x
> +  allocate(x%m(1))
> +  allocate(x%m(1)%p)
> +  call update_foo(x)
> +  call bad_update_foo(x)
> +end program main
> +
> +! { dg-output "At line 39 of file .*pr92050.f90.*Fortran runtime error: Index '2' of dimension 1 of array 'this%m' above upper bound of 1" }

Just my $0.02.

Backporting a patch from trunk that fixes a regression is
always encouraged.  It is up to the person doing the backport
to determine the level of effort.  If it exceeds some threshold
the person can choose to not backport.

For patches that fix a bug, which is not a regresssion, but
allows gfortran to accept valid Fortran or reject invalid Fortran,
I think that this is up to the discretion of the person doing the
backport.  For example, I backported several patches that fixed
free-source code parsing issues with attribute statements (i.e.,
gfortran accepted "publica" as if it were "public a").

For new features, I think backporting is okay if the new feature
is isolated behind an option.

Hi Steve,

well, the question is what counts as regression. In any case, I have now 
committed that patch as r278689.

Cheers,

Tobias

On Mon, Nov 25, 2019 at 03:34:25PM +0100, Tobias Burnus wrote:
> 
> well, the question is what counts as regression. In any case, I have now 
> committed that patch as r278689.
> 

Regression is fairly easy to define.  Standard conforming code
that compiled and executed correctly (for some definition of
correct) prior to a patch, which no longer compiles and/or
executes, is a regression.  Yes, I know, there are the latent
bugs that pop up, which appear to be regressions. :(

Thanks for your recent patches.