Bug 35840 - ICE for character expression in I/O specifier
Summary: ICE for character expression in I/O specifier
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Jerry DeLisle
URL:
Keywords: rejects-valid
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2008-04-06 13:03 UTC by Tobias Burnus
Modified: 2008-11-01 15:13 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.4.0
Last reconfirmed: 2008-10-11 15:32:03


Attachments
proposed patch (1.12 KB, patch)
2008-09-14 16:42 UTC, Mikael Morin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2008-04-06 13:03:50 UTC
The following gives an ICE:

==18935== Invalid read of size 1
==18935==    at 0x4C25D82: strlen (in /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so)
==18935==    by 0x438ABE: compare_to_allowed_values (io.c:1404)
==18935==    by 0x43A892: match_io (io.c:2883)
==18935==    by 0x451BA9: match_word (parse.c:64)


write(10,*, asynchronous="Y"//"E"//trim("S  "))
end

However, it is a valid initialization expression. I tried the following, but it did not help. (It still fails at "len = strlen (value)".)


--- io.c        (Revision 133959)
+++ io.c        (Arbeitskopie)
@@ -1389,11 +1389,17 @@ gfc_resolve_open (gfc_open *open)
 static int
 compare_to_allowed_values (const char *specifier, const char *allowed[],
                           const char *allowed_f2003[],
-                          const char *allowed_gnu[], char *value,
+                          const char *allowed_gnu[], gfc_expr *expr,
                           const char *statement, bool warn)
 {
   int i;
   unsigned int len;
+  const char *value;
+
+  if (gfc_simplify_expr (expr, 0) == 0)
+    return 1;
+
+  value = expr->value.character.string;

   len = strlen (value);
   if (len > 0)
Comment 1 Tobias Schlüter 2008-04-06 21:03:01 UTC
I'm on it.
Comment 2 Joost VandeVondele 2008-08-08 21:31:13 UTC
This turned in a rejects valid ?
pr35840.f90:1.25:

write(10,*, asynchronous="Y"//"E"//trim("S  "))
                        1
Error: ASYNCHRONOUS= specifier at (1) must be an initialization expression
Comment 3 Tobias Schlüter 2008-08-17 12:43:32 UTC
I've not had the time to finish this, and now I don't remember the details of my analysis.  Sorry, unassigning myself.
Comment 4 Mikael Morin 2008-09-14 16:42:25 UTC
Created attachment 16318 [details]
proposed patch

The problem here is that the parser matches a general expression and has to check later that it is an initialization expression. 
I have taken the code in gfc_match_init_expr and put it in its own function (gfc_reduce_init_expr) so that checking can be deferred. 
Fixing the bug is then trivial. 
This has just been regression tested. 
Should I send this to the fortran mailing list ?
Comment 5 Daniel Kraft 2008-09-14 16:51:54 UTC
Yes, that's probably the best to get comments/reviews for your patch; if you think it is already mature, CC gcc-patches@gcc.gnu.org, too.
Comment 6 Jerry DeLisle 2008-09-14 17:58:17 UTC
I see that I did not use RESOLVE_TAG in gfc_resolve_dt.  Doing so resolves the ICE issue.  Then if we really want to accept this poorly written code, I think the place to fix it may be in io.c (resolve_tag)

@@ -2481,6 +2483,7 @@ gfc_resolve_dt (gfc_dt *dt)
   RESOLVE_TAG (&tag_e_round, dt->round);
   RESOLVE_TAG (&tag_e_blank, dt->blank);
   RESOLVE_TAG (&tag_e_decimal, dt->decimal);
+  RESOLVE_TAG (&tag_e_async, dt->asynchronous);
 
   e = dt->io_unit;
   if (gfc_resolve_expr (e) == SUCCESS


Comment 7 Jerry DeLisle 2008-09-14 19:01:46 UTC
Subject: Bug 35840

Author: jvdelisle
Date: Sun Sep 14 19:00:26 2008
New Revision: 140366

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=140366
Log:
2008-09-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
	    Tobias Burnus  <burnus@net.b.de>

	PR fortran/35840
	* io.c (match_vtag): Add tag name to error message.
	(match_out_tag): Cleanup whitespace.
	(gfc_resolve_dt): Resolve id and async tags. 

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/io.c

Comment 8 Jerry DeLisle 2008-09-14 21:29:05 UTC
Patch committed in Comment #7 only eliminates the ICE.  Reduction of the initialization expression remains.
Comment 9 Mikael Morin 2008-09-15 12:49:38 UTC
(In reply to comment #5)
> if you think it is already mature, CC gcc-patches@gcc.gnu.org, too.
> 
There are already complains about my patch. Obviously it isn't. :(

(In reply to comment #6)
> Doing so resolves the ICE issue.  
Were you still getting an ICE ? (cf comment #2)

> Then if we really want to accept this poorly written code, 
If you think it is poorly written, all apologies. 
In fact, I was rather proud of it. :'(
I'm open to comments to improve it. 

> I think the place to fix it may be in io.c (resolve_tag)
Yes, in io.c, but not in resolve_tag, as it seems that an initialization expression is not required for asynchronous= in the case of an open statement. Correct me if I'm wrong. 
Comment 10 Jerry DeLisle 2008-09-15 13:39:11 UTC
Mikael, I was not referring to your code as poorly written.  I was referring to the Fortran test case in the original posting.  The test case may be valid, but somewhat ugly.

By all means, your code looks fine!  You have done a great job.  I am suggesting to see if you can resolve the reduction of the initialization expression inside gfc_resolve tag.  That way it will apply to all cases of IO tags. I submitted the partial patch partly to help show you where I was referring to and partly to get another ICE out of the way while I debug the ABI breakage on a different PR. 
Comment 11 Jerry DeLisle 2008-09-15 13:48:42 UTC
Subject: Re:  ICE for character expression in I/O specifier

mikael dot morin at tele2 dot fr wrote:
>
>> I think the place to fix it may be in io.c (resolve_tag)
> Yes, in io.c, but not in resolve_tag, as it seems that an initialization
> expression is not required for asynchronous= in the case of an open statement.
> Correct me if I'm wrong. 
I have not reviewed the standard specifically for this case.  If you have done 
so, then fixing it where you have is OK.  We should review all such tags against 
the standard to make sure we don't miss any others.  It seems odd to me that 
only asynchronous= has this requirement.


Comment 12 Dominique d'Humieres 2008-09-15 14:53:24 UTC
I have applied the patch in comment #4 on i686-apple-darwin9 and it worked as expected and passes my quick tests. I'll regtest later this evening.
Comment 13 Dominique d'Humieres 2008-09-15 19:10:43 UTC
Testing finished without regression.

Thanks for the patch.

Comment 14 Mikael Morin 2008-09-15 19:42:19 UTC
Sorry Jerry, I didn't read your message (I mean the one to the list with the patch to io.c) before. 
I have just checked again, the patch does work for me. 
And Dominique agrees with me. :)

(In reply to comment #11)
> We should review all such tags against the standard to make sure we don't 
> miss any others. 
I have just done it. Asynchronous in data transfer statements is the only case requiring an initialization expression. 

> It seems odd to me that only asynchronous= has this requirement.
> 
to me too. 

Note 9.29:
[...]
The ASYNCHRONOUS= specifier value in a data transfer statement is an initialization expression because it effects compiler optimizations and, therefore, needs to be known at compile time.
Comment 15 Dominique d'Humieres 2008-10-11 10:35:53 UTC
Ping!

Comment 16 Jerry DeLisle 2008-10-11 15:32:03 UTC
I will see if I can finalize this patch. Mikael, are you still with us? Your approach was fine.
Comment 17 Mikael Morin 2008-10-12 11:47:16 UTC
(In reply to comment #16)
> Mikael, are you still with us? Your approach was fine.
> 
Yep, I'm not dead yet. 
I was waiting for my copyright assignment form. 
Now it's on the way back, I will post to gcc-patches soon. 
Comment 18 Mikael Morin 2008-10-28 14:08:57 UTC
The final patch is here:
http://gcc.gnu.org/ml/fortran/2008-10/msg00104.html
Comment 19 Mikael Morin 2008-10-31 15:57:49 UTC
Subject: Bug 35840

Author: mikael
Date: Fri Oct 31 15:56:21 2008
New Revision: 141497

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141497
Log:

2008-10-31  Mikael Morin  <mikael.morin@tele2.fr>

	PR fortran/35840
	* expr.c (gfc_reduce_init_expr): New function, containing checking code
	from gfc_match_init_expr, so that checking can be deferred. 
	(gfc_match_init_expr): Use gfc_reduce_init_expr.
	* io.c (check_io_constraints): Use gfc_reduce_init_expr instead of 
	checking that the expression is a constant. 
	* match.h (gfc_reduce_init_expr): Prototype added. 

2008-10-31  Mikael Morin  <mikael.morin@tele2.fr>

	PR fortran/35840
	* gfortran.dg/write_check4.f90: New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/write_check4.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/expr.c
    trunk/gcc/fortran/io.c
    trunk/gcc/fortran/match.h
    trunk/gcc/testsuite/ChangeLog

Comment 20 Jerry DeLisle 2008-11-01 15:13:56 UTC
Closing, thanks for patch.