[fortran] Fix PR 85781, ICE on valid

Hello world,

the attached patch fixes an ICE which could occur for empty
substrings (see test case).

In the spirit of "A patch that works beats an elegant idea every
time" I simply set a substring known to be empty to (1:0) so
that problems further down the road could be avoided.

Regression-tested. OK for trunk?

Regards

	Thomas

2020-01-19  Thomas König  <tkoenig@gcc.gnu.org>

	PR fortran/85781
	* resolve.c (resolve_substring): If the substring is empty, set it
	to (1:0) explicitly.


2020-01-19  Thomas König  <tkoenig@gcc.gnu.org>

	PR fortran/85781
	* gfortran.dg/substr_9.f90: New test.

Hi Thomas,

On 1/19/20 7:21 PM, Thomas König wrote:
> the attached patch fixes an ICE which could occur for empty
> substrings (see test case).

I think one should rather fix the following issue. – While on x86-64 it 
does not seem to cause problems, it might for other platforms. 
Additionally, it is a missed optimization to use "A123" instead of "66" 
in the call.

Namely, gfortran generates a different code when using kind=c_type 
instead of kind=1 – although the kind value is the same! (Without 
BIND(C) or without VALUE, the generated code is the same.) The dump is:

two (character(kind=1)[1:1] x) vs.  three (character(kind=1) x)

Expected is the latter. See attached test case.

Tobias

On 1/20/20 1:31 PM, Tobias Burnus wrote:
> I think one should rather fix the following issue.

That's now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93336

Tobias

Hi Tobias,

>> the attached patch fixes an ICE which could occur for empty
>> substrings (see test case).
> 
> I think one should rather fix the following issue.

I am not sure what you mean.  Does that mean that fixing the following
issue will also fix PR 85781, or that the PR should not be fixed?

Regards

	Thomas

Hi Thomas, hi all,

first, I have now attached a different fix for PR 85781 (= original 
bug). Can you have a look?

I have the feeling (but didn't check) that your patch does not handle 
the following variant of the test case: "print *, x(m:n)" (i.e. the 
lower bound is not known at compile time).

I hope my patch covers all issues. – OK for the trunk?

Secondly:

On 1/21/20 7:32 PM, Thomas König wrote:
>>> the attached patch fixes an ICE which could occur for empty
>>> substrings (see test case).
>> I think one should rather fix the following issue.
> I am not sure what you mean.  Does that mean that fixing the following
> issue will also fix PR 85781

I am no longer sure what I meant myself ;-)

I initially thought those are directly related – but they now look 
related but independent bugs:

PR 85781 is about getting a non-ARRAY_TYPE (tree dump: "character") and 
using it as ARRAY_TYPE (tree dump: "character[lb:ub]").

While PR93336 is about (1) using  an ARRAY_TYPE when one should not. – 
And, additionally, about missing diagnostic related to (2) bind(c) and 
kind=4, (3) passing zero-length strings to non-zero-length dummy args, 
(4) diagnostic about truncating too long strings (esp. if of 
non-default, non-c_char kind).

Tobias

On Wed, Jan 22, 2020 at 11:59:12AM +0100, Tobias Burnus wrote:
>
> And, additionally, about missing diagnostic related to (2) bind(c) and 
> kind=4,
>

Are you sure there is a missing diagnostic?  You need to 
add -Wc-binding-type or -Wall to your command line.

subroutine p(c) bind(c)
   use iso_c_binding
   character(kind=4) c
   print *, c
end

% gfcx -Wall -c a.f90
a.f90:1:14:

    1 | subroutine p(c) bind(c)
      |              1
Warning: Variable 'c' at (1) is a dummy argument of the BIND(C) procedure
'p' but may not be C interoperable [-Wc-binding-type]

Hi Steve,

On 1/22/20 4:02 PM, Steve Kargl wrote:
> On Wed, Jan 22, 2020 at 11:59:12AM +0100, Tobias Burnus wrote:
>> And, additionally, about missing diagnostic related to (2) bind(c) and
>> kind=4,
> Are you sure there is a missing diagnostic?  You need to
> add -Wc-binding-type or -Wall to your command line.

The problem with the ISO C warnings for character is that they are not 
based on the KIND value but whether the variable comes from the 
ISO_C_BINDING module or not.

Namely, for kind=1 and kind=4 you get the warning you have shown while 
for kind=c_char and kind=c_int32_t you don't. (If you use "integer, 
parameter :: mykind = c_char", the result is the same as for c_char as 
this property is inherited.)

Numerically, 1 and c_char are the same – as are 4 and c_int32_t. I think 
one *can* warn for using kind=1 as this is bad style (= assumption that 
c_char is the same as "1" and the same as the default-kind character).

But "kind=4" is as bad as "kind=c_int32_t"! — And as noted in the PR, 
using c_char and c_int32_t (!) will give a "character(kind=1)" (note the 
kind=1!) dummy argument while kind=1 and kind=4 will give 
"character(kind={1 or 4})[1:1]" (= ARRAY_TYPE).

That's complete nonsense and in some cases it does not even depend on 
whether that's a BIND(C) procedure or not!

Cheers,

Tobias

On Wed, Jan 22, 2020 at 04:43:49PM +0100, Tobias Burnus wrote:
> Hi Steve,
> 
> On 1/22/20 4:02 PM, Steve Kargl wrote:
> > On Wed, Jan 22, 2020 at 11:59:12AM +0100, Tobias Burnus wrote:
> >> And, additionally, about missing diagnostic related to (2) bind(c) and
> >> kind=4,
> > Are you sure there is a missing diagnostic?  You need to
> > add -Wc-binding-type or -Wall to your command line.
> 
> The problem with the ISO C warnings for character is that they are not 
> based on the KIND value but whether the variable comes from the 
> ISO_C_BINDING module or not.
> 
> Namely, for kind=1 and kind=4 you get the warning you have shown while 
> for kind=c_char and kind=c_int32_t you don't. (If you use "integer, 
> parameter :: mykind = c_char", the result is the same as for c_char as 
> this property is inherited.)
> 
> Numerically, 1 and c_char are the same – as are 4 and c_int32_t. I think 
> one *can* warn for using kind=1 as this is bad style (= assumption that 
> c_char is the same as "1" and the same as the default-kind character).
> 
> But "kind=4" is as bad as "kind=c_int32_t"! — And as noted in the PR, 
> using c_char and c_int32_t (!) will give a "character(kind=1)" (note the 
> kind=1!) dummy argument while kind=1 and kind=4 will give 
> "character(kind={1 or 4})[1:1]" (= ARRAY_TYPE).
> 
> That's complete nonsense and in some cases it does not even depend on 
> whether that's a BIND(C) procedure or not!
> 

Not that I disagree with you, but this is what happens when
UTF-8 (kind=4) is wedged on top of ISO C Binding (kind=c_char),
which is wedged on top of a Fortran 95 compiler.  Supposedly, one
can use attr.is_c_interop and attr.is_iso_c to determine if something
is interoperable (if it is consistently set) and if it is from 
the ISO C Binding module.

The solution is to use unique kind values forall types (reserve
1-9 for integer, 11-19 for real, 21-29 for character, etc).  Then
one simply needs to define a storage association mapping.

Hi Steve,

On 1/22/20 4:54 PM, Steve Kargl wrote:
> Supposedly, one can use attr.is_c_interop and attr.is_iso_c to 
> determine if something is interoperable (if it is consistently set) 
> and if it is from the ISO C Binding module. 

I think this is fixable on two sides:

* For the used tree type, one just has to check kind value (the integer) 
plus whether the dummy's procedure is BIND(C). Here the problem is that 
we do not always have this information available (i.e. it needs to be 
passed on). Not difficult, but one needs to find all spots and think of 
how to pass the information.

* For the diagnostic, we can check whether the dummy argument is in 
bind(C) and do some additional diagnostic in this case (e.g. always warn 
for kind=4 with BIND(C) – or reject it unconditionally or with -std=... 
or …).

* The other issue related to kind=4: Fortran permits to pass a too long 
string as actual argument; but this is only permitted in the standard 
for kind=1 and kind=c_char strings. In gfortran, this also works fine 
with kind=4 – but one might still want to warn (with some -Wall or 
-std=f... or ...). Likewise, the trimming happening in this case might 
indicate a bug in the program: "Hello World" as actual to 
character(len=10) is most likely a bug, even if this is valid Fortran.

Cheers,

Tobias

Admittedly an early PING.

On 1/22/20 11:59 AM, Tobias Burnus wrote:
> Hi Thomas, hi all,
>
> first, I have now attached a different fix for PR 85781 (= original 
> bug). Can you have a look?
>
> I have the feeling (but didn't check) that your patch does not handle 
> the following variant of the test case: "print *, x(m:n)" (i.e. the 
> lower bound is not known at compile time).

I confirmed that my suspicion was right: the "resolve_substring" patch 
(first patch in this email thread) is not sufficient as all my test 
cases (of this patch) will still ICE with it.

> I hope my patch covers all issues. – OK for the trunk?

Cheers,

Tobias

> Secondly:
>
> On 1/21/20 7:32 PM, Thomas König wrote:
>>>> the attached patch fixes an ICE which could occur for empty
>>>> substrings (see test case).
>>> I think one should rather fix the following issue.
>> I am not sure what you mean.  Does that mean that fixing the following
>> issue will also fix PR 85781
>
> I am no longer sure what I meant myself ;-)
>
> I initially thought those are directly related – but they now look 
> related but independent bugs:
>
> PR 85781 is about getting a non-ARRAY_TYPE (tree dump: "character") 
> and using it as ARRAY_TYPE (tree dump: "character[lb:ub]").
>
> While PR93336 is about (1) using  an ARRAY_TYPE when one should not. – 
> And, additionally, about missing diagnostic related to (2) bind(c) and 
> kind=4, (3) passing zero-length strings to non-zero-length dummy args, 
> (4) diagnostic about truncating too long strings (esp. if of 
> non-default, non-c_char kind).
>
> Tobias
>

Hi Tobias,

> I hope my patch covers all issues. – OK for the trunk?

Yep.

Thanks a lot for the patch!

Regards

	Thomas