Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Steve Kargl]

On Sat, Dec 03, 2016 at 05:58:02PM +0100, Andreas Schwab wrote:
> On Dez 01 2016, Steve Kargl  wrote:
> 
> > PR fortran/78618
> > * gfortran.dg/char_conversion.f90: New test.
> 
> That test still crashes the compiler on m68k:
> 
> 0xb40a7f crash_signal
> ../../gcc/toplev.c:333
> 0x58dd52 gfc_is_constant_expr(gfc_expr*)
> ../../gcc/fortran/expr.c:899
> 0x5feb90 resolve_fl_procedure
> ../../gcc/fortran/resolve.c:12013

Yes, we know.  I've reopen the PR; see audit trail.
It seems to be a used-after-freed issue, but I've
been unable to track down the issue.

-- 
Steve

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Andreas Schwab]

On Dez 01 2016, Steve Kargl  wrote:

>   PR fortran/78618
>   * gfortran.dg/char_conversion.f90: New test.

That test still crashes the compiler on m68k:

/daten/aranym/gcc/gcc-20161203/gcc/testsuite/gfortran.dg/char_conversion.f90:8:30:
 Error: Character '\u0100' in string at (1) cannot be converted into character 
kind 1
f951: internal compiler error: Segmentation fault
0xb40a7f crash_signal
../../gcc/toplev.c:333
0x58dd52 gfc_is_constant_expr(gfc_expr*)
../../gcc/fortran/expr.c:899
0x5feb90 resolve_fl_procedure
../../gcc/fortran/resolve.c:12013
0x5feb90 resolve_symbol
../../gcc/fortran/resolve.c:14721
0x616e73 do_traverse_symtree
../../gcc/fortran/symbol.c:3994
0x6007d4 resolve_types
../../gcc/fortran/resolve.c:15948
0x5fc3e4 gfc_resolve
../../gcc/fortran/resolve.c:16061
0x5e70b2 resolve_all_program_units
../../gcc/fortran/parse.c:5977
0x5e70b2 gfc_parse_file()
../../gcc/fortran/parse.c:6224
0x629da2 gfc_be_parse_file
../../gcc/fortran/f95-lang.c:202

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Steve Kargl]

On Fri, Dec 02, 2016 at 07:39:33PM +0100, Janus Weil wrote:
> 
> Testing went well. Committed as r243201.
> 

Thanks for reviewing my patch, and more importantly
thanks for your patch.

-- 
Steve

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Janus Weil]

2016-12-02 19:06 GMT+01:00 Janus Weil :
> 2016-12-02 18:13 GMT+01:00 Steve Kargl :
>>> >> > The attached patch fixes an ICE, a nearby whitespace issue, and
>>> >> > removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
>>> >> > testing on x86_64-*-freebsd.  Ok to commit?
>>> >>
>>> >> huh, I don't really understand why the argument of RANK is detected to
>>> >> be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
>>> >> an EXPR_CONSTANT?
>>> >>
>>> >> Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
>>> >> is the symbol "c" (as expected), but that clearly is not a function,
>>> >> so it seems to me that the actual bug here is that a->expr_type is set
>>> >> incorrectly ...?
>>> >
>>> > I found that it is the function __convert_s4_s1.
>>>
>>> That's strange. If we see different things here, maybe we are running
>>> into some kind of undefined behavior (possibly related to
>>> gfc_bad_expr?). Anyway, after some more debugging I came to the
>>> conclusion that what actually fails is the error propagation, which
>>> seems to be broken in gfc_check_assign and can be fixed like this:
>>>
>>>
>>> Index: gcc/fortran/expr.c
>>> ===
>>> --- gcc/fortran/expr.c(revision 243194)
>>> +++ gcc/fortran/expr.c(working copy)
>>> @@ -3314,9 +3314,9 @@ gfc_check_assign (gfc_expr *lvalue, gfc_expr *rval
>>>if (lvalue->ts.type == BT_CHARACTER && rvalue->ts.type == BT_CHARACTER)
>>>  {
>>>if (lvalue->ts.kind != rvalue->ts.kind && allow_convert)
>>> -gfc_convert_chartype (rvalue, >ts);
>>> -
>>> -  return true;
>>> +return gfc_convert_chartype (rvalue, >ts);
>>> +  else
>>> +return true;
>>>  }
>>>
>>>if (!allow_convert)
>>>
>>>
>>> This also avoids the ICE and I think is the proper way to fix this ...
>>>
>>
>> When developing the patch, I fixed/avoided the ICE, first.  Then,
>> I tried Gerhard's second testcase without the PARAMETER attribute.
>> An error message is emitted, so I went hunting for why PARAMETER
>> suppressed the error message.  This led to the second part of the
>> patch of changing gfc_error to gfc_error_now.
>
> I think the gfc_error_now should not be necessary with my fix, but
> removing the ATTRIBUTE_UNUSED is certainly a good idea.
>
>
>> In any event, if your patch causes an error message to be emitted,
>> then I think that your patch is better than the one I proposed.
>> Feel free to commit your patch.
>
> I am now regtesting the attached version and, if successful, will commit.

Testing went well. Committed as r243201.

Cheers,
Janus

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Janus Weil]

2016-12-02 18:13 GMT+01:00 Steve Kargl :
>> >> > The attached patch fixes an ICE, a nearby whitespace issue, and
>> >> > removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
>> >> > testing on x86_64-*-freebsd.  Ok to commit?
>> >>
>> >> huh, I don't really understand why the argument of RANK is detected to
>> >> be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
>> >> an EXPR_CONSTANT?
>> >>
>> >> Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
>> >> is the symbol "c" (as expected), but that clearly is not a function,
>> >> so it seems to me that the actual bug here is that a->expr_type is set
>> >> incorrectly ...?
>> >
>> > I found that it is the function __convert_s4_s1.
>>
>> That's strange. If we see different things here, maybe we are running
>> into some kind of undefined behavior (possibly related to
>> gfc_bad_expr?). Anyway, after some more debugging I came to the
>> conclusion that what actually fails is the error propagation, which
>> seems to be broken in gfc_check_assign and can be fixed like this:
>>
>>
>> Index: gcc/fortran/expr.c
>> ===
>> --- gcc/fortran/expr.c(revision 243194)
>> +++ gcc/fortran/expr.c(working copy)
>> @@ -3314,9 +3314,9 @@ gfc_check_assign (gfc_expr *lvalue, gfc_expr *rval
>>if (lvalue->ts.type == BT_CHARACTER && rvalue->ts.type == BT_CHARACTER)
>>  {
>>if (lvalue->ts.kind != rvalue->ts.kind && allow_convert)
>> -gfc_convert_chartype (rvalue, >ts);
>> -
>> -  return true;
>> +return gfc_convert_chartype (rvalue, >ts);
>> +  else
>> +return true;
>>  }
>>
>>if (!allow_convert)
>>
>>
>> This also avoids the ICE and I think is the proper way to fix this ...
>>
>
> When developing the patch, I fixed/avoided the ICE, first.  Then,
> I tried Gerhard's second testcase without the PARAMETER attribute.
> An error message is emitted, so I went hunting for why PARAMETER
> suppressed the error message.  This led to the second part of the
> patch of changing gfc_error to gfc_error_now.

I think the gfc_error_now should not be necessary with my fix, but
removing the ATTRIBUTE_UNUSED is certainly a good idea.


> In any event, if your patch causes an error message to be emitted,
> then I think that your patch is better than the one I proposed.
> Feel free to commit your patch.

I am now regtesting the attached version and, if successful, will commit.

Cheers,
Janus
Index: gcc/fortran/check.c
===
--- gcc/fortran/check.c (revision 243194)
+++ gcc/fortran/check.c (working copy)
@@ -3667,7 +3667,7 @@ gfc_check_range (gfc_expr *x)
 
 
 bool
-gfc_check_rank (gfc_expr *a ATTRIBUTE_UNUSED)
+gfc_check_rank (gfc_expr *a)
 {
   /* Any data object is allowed; a "data object" is a "constant (4.1.3),
  variable (6), or subobject of a constant (2.4.3.2.3)" (F2008, 1.3.45).  */
Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c  (revision 243194)
+++ gcc/fortran/expr.c  (working copy)
@@ -3314,9 +3314,9 @@ gfc_check_assign (gfc_expr *lvalue, gfc_expr *rval
   if (lvalue->ts.type == BT_CHARACTER && rvalue->ts.type == BT_CHARACTER)
 {
   if (lvalue->ts.kind != rvalue->ts.kind && allow_convert)
-   gfc_convert_chartype (rvalue, >ts);
-
-  return true;
+   return gfc_convert_chartype (rvalue, >ts);
+  else
+   return true;
 }
 
   if (!allow_convert)

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Steve Kargl]

On Fri, Dec 02, 2016 at 06:00:48PM +0100, Janus Weil wrote:
> 2016-12-02 17:30 GMT+01:00 Steve Kargl :
> > On Fri, Dec 02, 2016 at 04:47:19PM +0100, Janus Weil wrote:
> >> Hi Steve,
> >>
> >> 2016-12-02 2:33 GMT+01:00 Steve Kargl :
> >> > The attached patch fixes an ICE, a nearby whitespace issue, and
> >> > removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
> >> > testing on x86_64-*-freebsd.  Ok to commit?
> >>
> >> huh, I don't really understand why the argument of RANK is detected to
> >> be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
> >> an EXPR_CONSTANT?
> >>
> >> Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
> >> is the symbol "c" (as expected), but that clearly is not a function,
> >> so it seems to me that the actual bug here is that a->expr_type is set
> >> incorrectly ...?
> >
> > I found that it is the function __convert_s4_s1.
> 
> That's strange. If we see different things here, maybe we are running
> into some kind of undefined behavior (possibly related to
> gfc_bad_expr?). Anyway, after some more debugging I came to the
> conclusion that what actually fails is the error propagation, which
> seems to be broken in gfc_check_assign and can be fixed like this:
> 
> 
> Index: gcc/fortran/expr.c
> ===
> --- gcc/fortran/expr.c(revision 243194)
> +++ gcc/fortran/expr.c(working copy)
> @@ -3314,9 +3314,9 @@ gfc_check_assign (gfc_expr *lvalue, gfc_expr *rval
>if (lvalue->ts.type == BT_CHARACTER && rvalue->ts.type == BT_CHARACTER)
>  {
>if (lvalue->ts.kind != rvalue->ts.kind && allow_convert)
> -gfc_convert_chartype (rvalue, >ts);
> -
> -  return true;
> +return gfc_convert_chartype (rvalue, >ts);
> +  else
> +return true;
>  }
> 
>if (!allow_convert)
> 
> 
> This also avoids the ICE and I think is the proper way to fix this ...
> 

When developing the patch, I fixed/avoided the ICE, first.  Then,
I tried Gerhard's second testcase without the PARAMETER attribute.
An error message is emitted, so I went hunting for why PARAMETER
suppressed the error message.  This led to the second part of the
patch of changing gfc_error to gfc_error_now.  Perhaps, forcing
the gfc_error_now prevents gfortran from inserting the 
__convert_s4_s1 call.

In any event, if your patch causes an error message to be emitted,
then I think that your patch is better than the one I proposed.  
Feel free to commit your patch.

-- 
Steve

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Janus Weil]

2016-12-02 17:30 GMT+01:00 Steve Kargl :
> On Fri, Dec 02, 2016 at 04:47:19PM +0100, Janus Weil wrote:
>> Hi Steve,
>>
>> 2016-12-02 2:33 GMT+01:00 Steve Kargl :
>> > The attached patch fixes an ICE, a nearby whitespace issue, and
>> > removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
>> > testing on x86_64-*-freebsd.  Ok to commit?
>>
>> huh, I don't really understand why the argument of RANK is detected to
>> be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
>> an EXPR_CONSTANT?
>>
>> Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
>> is the symbol "c" (as expected), but that clearly is not a function,
>> so it seems to me that the actual bug here is that a->expr_type is set
>> incorrectly ...?
>
> I found that it is the function __convert_s4_s1.

That's strange. If we see different things here, maybe we are running
into some kind of undefined behavior (possibly related to
gfc_bad_expr?). Anyway, after some more debugging I came to the
conclusion that what actually fails is the error propagation, which
seems to be broken in gfc_check_assign and can be fixed like this:


Index: gcc/fortran/expr.c
===
--- gcc/fortran/expr.c(revision 243194)
+++ gcc/fortran/expr.c(working copy)
@@ -3314,9 +3314,9 @@ gfc_check_assign (gfc_expr *lvalue, gfc_expr *rval
   if (lvalue->ts.type == BT_CHARACTER && rvalue->ts.type == BT_CHARACTER)
 {
   if (lvalue->ts.kind != rvalue->ts.kind && allow_convert)
-gfc_convert_chartype (rvalue, >ts);
-
-  return true;
+return gfc_convert_chartype (rvalue, >ts);
+  else
+return true;
 }

   if (!allow_convert)


This also avoids the ICE and I think is the proper way to fix this ...

Cheers,
Janus

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Steve Kargl]

On Fri, Dec 02, 2016 at 04:47:19PM +0100, Janus Weil wrote:
> Hi Steve,
> 
> 2016-12-02 2:33 GMT+01:00 Steve Kargl :
> > The attached patch fixes an ICE, a nearby whitespace issue, and
> > removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
> > testing on x86_64-*-freebsd.  Ok to commit?
> 
> huh, I don't really understand why the argument of RANK is detected to
> be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
> an EXPR_CONSTANT?
> 
> Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
> is the symbol "c" (as expected), but that clearly is not a function,
> so it seems to me that the actual bug here is that a->expr_type is set
> incorrectly ...?
> 

I found that it is the function __convert_s4_s1.   Namely,  we have

character, parameter :: c = char(256,4)
print *, rank(c)

c is kind=1 and char(256,4) is kind 4.  so, we end up with

print *, rank(__convert_s4_s1(...))

As __convert_s4_s1() has neither isym nor esym set, you get a problem.

-- 
Steve

Re: [PATCH] PR fortran/78618 -- RANK() should not ICE

[Janus Weil]

Hi Steve,

2016-12-02 2:33 GMT+01:00 Steve Kargl :
> The attached patch fixes an ICE, a nearby whitespace issue, and
> removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
> testing on x86_64-*-freebsd.  Ok to commit?

huh, I don't really understand why the argument of RANK is detected to
be an EXPR_FUNCTION for the test case at hand. Shouldn't it rather be
an EXPR_CONSTANT?

Some debugging in gfc_check_rank shows that a->symtree->n.sym indeed
is the symbol "c" (as expected), but that clearly is not a function,
so it seems to me that the actual bug here is that a->expr_type is set
incorrectly ...?

Cheers,
Janus




> 2016-12-01  Steven G. Kargl  
>
> PR fortran/78618
> * check.c (gfc_check_rank): Remove ATTRIBUTE_UNUSED.  Fix whitespace.
> Fix ICE where a NULL pointer is dereferenced.
> * simplify.c (gfc_convert_char_constant):  Do not buffer error.
>
> 2016-12-01  Steven G. Kargl  
>
> PR fortran/78618
> * gfortran.dg/char_conversion.f90: New test.
>
> --
> Steve

[PATCH] PR fortran/78618 -- RANK() should not ICE

[Steve Kargl]

The attached patch fixes an ICE, a nearby whitespace issue, and
removed the ATTRIBUTE_UNUSED tag.  THe change has passed regression
testing on x86_64-*-freebsd.  Ok to commit?

2016-12-01  Steven G. Kargl  

PR fortran/78618
* check.c (gfc_check_rank): Remove ATTRIBUTE_UNUSED.  Fix whitespace.
Fix ICE where a NULL pointer is dereferenced.
* simplify.c (gfc_convert_char_constant):  Do not buffer error.

2016-12-01  Steven G. Kargl  

PR fortran/78618
* gfortran.dg/char_conversion.f90: New test.

-- 
Steve
Index: gcc/fortran/check.c
===
--- gcc/fortran/check.c	(revision 243142)
+++ gcc/fortran/check.c	(working copy)
@@ -3667,7 +3667,7 @@ gfc_check_range (gfc_expr *x)
 
 
 bool
-gfc_check_rank (gfc_expr *a ATTRIBUTE_UNUSED)
+gfc_check_rank (gfc_expr *a)
 {
   /* Any data object is allowed; a "data object" is a "constant (4.1.3),
  variable (6), or subobject of a constant (2.4.3.2.3)" (F2008, 1.3.45).  */
@@ -3676,9 +3676,16 @@ gfc_check_rank (gfc_expr *a ATTRIBUTE_UN
 
   /* Functions returning pointers are regarded as variable, cf. F2008, R602.  */
   if (a->expr_type == EXPR_FUNCTION)
-is_variable = a->value.function.esym
-		  ? a->value.function.esym->result->attr.pointer
-		  : a->symtree->n.sym->result->attr.pointer;
+{
+  if (a->value.function.esym)
+	is_variable = a->value.function.esym->result->attr.pointer;
+  else if (a->symtree->n.sym->result)
+	is_variable = a->symtree->n.sym->result->attr.pointer;
+  else if (a->symtree->n.sym->value)
+	is_variable = true;
+  else
+	gfc_internal_error ("gfc_check_rank(): invalid function result");
+}
 
   if (a->expr_type == EXPR_OP || a->expr_type == EXPR_NULL
   || a->expr_type == EXPR_COMPCALL|| a->expr_type == EXPR_PPC
Index: gcc/fortran/simplify.c
===
--- gcc/fortran/simplify.c	(revision 243142)
+++ gcc/fortran/simplify.c	(working copy)
@@ -7148,7 +7148,7 @@ gfc_convert_char_constant (gfc_expr *e, 
 	if (!gfc_check_character_range (result->value.character.string[i],
 	kind))
 	  {
-	gfc_error ("Character %qs in string at %L cannot be converted "
+	gfc_error_now ("Character %qs in string at %L cannot be converted "
 		   "into character kind %d",
 		   gfc_print_wide_char (result->value.character.string[i]),
 		   >where, kind);
Index: gcc/testsuite/gfortran.dg/char_conversion.f90
===
--- gcc/testsuite/gfortran.dg/char_conversion.f90	(nonexistent)
+++ gcc/testsuite/gfortran.dg/char_conversion.f90	(working copy)
@@ -0,0 +1,6 @@
+! { dg-do compile }
+! PR fortran/78618
+program p
+   character, parameter :: c = char(256,4) ! { dg-error "cannot be converted" }
+   if (rank(c) /= 0) call abort
+end