Bug 32760 - [4.3 Regression] Error defining subroutine named PRINT
Summary: [4.3 Regression] Error defining subroutine named PRINT
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.3.0
: P4 normal
Target Milestone: 4.3.0
Assignee: Paul Thomas
URL:
Keywords: rejects-valid
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2007-07-13 22:37 UTC by Harald Anlauf
Modified: 2008-02-03 11:32 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.1.2 4.2.1 4.3.0
Last reconfirmed: 2007-07-26 04:56:18


Attachments
A fix for the PR (436 bytes, patch)
2008-01-31 22:59 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Harald Anlauf 2007-07-13 22:37:57 UTC
Hi,

the following legal code fails to compile with
gfortran build 20070713:

module gfcbug68
  implicit none
  public :: print

contains

  subroutine foo (i)
    integer, intent(in)  :: i

    print *, i
  end subroutine foo

  subroutine print (m)
    integer, intent(in) :: m
  end subroutine print

end module gfcbug68


I get:

gfcbug68.f90:13.18:

  subroutine print (m)
                 1
Error: VARIABLE attribute of 'print' conflicts with PROCEDURE attribute at (1)
gfcbug68.f90:14.28:

    integer, intent(in) :: m
                           1
Error: Unexpected data declaration statement in CONTAINS section at (1)
gfcbug68.f90:15.5:

  end subroutine print
    1
Error: Expecting END MODULE statement at (1)
gfcbug68.f90:3.17:

  public :: print
                1
Error: Symbol 'print' at (1) has no IMPLICIT type


Reversing the subroutines removes (or hides) the problem.

Cheers,
-ha
Comment 1 Daniel Franke 2007-07-13 23:45:42 UTC
Harald, thanks for your report.

> Reversing the subroutines removes (or hides) the problem.
So does the removal of either the "public :: print" or the PRINT statement in function FOO.

Are you sure this is a regression? I see the same errors with v4.1.2 and v4.2.1 (20070620).
Comment 2 Harald Anlauf 2007-07-14 10:21:01 UTC
(In reply to comment #1)

> Are you sure this is a regression? I see the same errors with v4.1.2 and v4.2.1
> (20070620).

Well, you're right.

The point is that the above snippet is the extract from a larger module
that used to compile with older snapshots of 4.2.0 and 4.3.0 (before about
March or April).  After that time I don't have any samples because gfortran
IDE'd on various other parts of the project.

I guess that that above was hidden for some reason and was uncovered only
recently when compiling the larger module.  So it's not a regression in
the strict sense.
Comment 3 Tobias Burnus 2007-07-16 09:58:30 UTC
Work around: Remove "public :: print".

I wonder what is that specific about PRINT. It works with WRITE, OPEN, ABS, ... but it does not work with PRINT ?!?
Comment 4 Jerry DeLisle 2007-07-17 04:26:43 UTC
It does not work with WRITE.  If you replace the PRINT statement inside foo with an equivalent WRITE statement, you get the same error.  The public :: print symbol is getting used by the PRINT statement rather than a new symbol.

I will work on this a bit.
Comment 5 Jerry DeLisle 2007-07-22 04:08:37 UTC
Daniel, I bet this is related to the print foo bug you were working on.  Same kind of thing.  Will you take a further look?
Comment 6 Daniel Franke 2007-07-22 08:21:24 UTC
> Daniel, I bet this is related to the print foo bug you were working on.  
> Same kind of thing.  Will you take a further look?

Jerry, you lost. While skipping the "fixup" in pr32710 avoids the ICE and results in a correct parse tree, it has no effect on this one. Same problem, different origin.
Comment 7 Daniel Franke 2007-07-22 17:39:16 UTC
Digging in ...
Comment 8 Daniel Franke 2007-07-24 19:53:52 UTC
Traced it down to primary.c(match_variable). Here, a symbol for PRINT is found (originating from the PUBLIC-statement). As the current flavour of the symbol is FL_UNKNOWN, it is set to FL_VARIABLE. The subroutine PRINT then conflicts with the (nonexistent) variable PRINT ...
Comment 9 Daniel Franke 2007-07-24 19:59:33 UTC
Obviously, this mechanism fails:

  /* Since nothing has any business being an lvalue in a module
     specification block, an interface block or a contains section,
     we force the changed_symbols mechanism to work by setting
     host_flag to 0. This prevents valid symbols that have the name
     of keywords, such as 'end', being turned into variables by
     failed matching to assignments for, eg., END INTERFACE.  */
  if (gfc_current_state () == COMP_MODULE
      || gfc_current_state () == COMP_INTERFACE
      || gfc_current_state () == COMP_CONTAINS)
    host_flag = 0;
Comment 10 Daniel Franke 2007-07-24 20:51:27 UTC
> Obviously, this mechanism fails

OTOH, maybe it doesn't. Setting host_flag to 0 in gdb does not seem to have any effect.

Currently I'm lost. No idea how to fix this, thus unassigning myself. Jerry?
Comment 11 Jerry DeLisle 2007-07-26 04:02:53 UTC
The following seems to allow the test case to compile without error:

Index: primary.c
===================================================================
--- primary.c	(revision 126937)
+++ primary.c	(working copy)
@@ -2452,6 +2452,8 @@ match_variable (gfc_expr **result, int e
       break;
 
     case FL_UNKNOWN:
+      if (sym->attr.access == ACCESS_PUBLIC)
+	break;
       if (gfc_add_flavor (&sym->attr, FL_VARIABLE,
 			  sym->name, NULL) == FAILURE)
 	return MATCH_ERROR;

I am regression testing now and I need to verify if the resulting executable actually works or not.
Comment 12 Jerry DeLisle 2007-07-26 04:54:43 UTC
This test case appears to execute correctly:

module gfcbug68
  implicit none
  public :: write

contains

  function foo (i)
    integer, intent(in)  :: i
    integer foo

    write (*,*) i
    foo = i
  end function foo

  subroutine write (m)
    integer, intent(in) :: m
    print *, m*m*m
  end subroutine write

end module gfcbug68

program testit
  use gfcbug68
  integer :: i = 27
  integer :: k
  
  k = foo(i)
  print *, "in the main:", k
  call write(33)
end program testit
Comment 13 Jerry DeLisle 2007-07-26 04:56:18 UTC
I will prepare a submittal to the list.  Regression tested OK too.
Comment 14 Daniel Franke 2007-07-26 06:57:54 UTC
>     case FL_UNKNOWN:
>+      if (sym->attr.access == ACCESS_PUBLIC)
>+       break;
>       if (gfc_add_flavor (&sym->attr, FL_VARIABLE,
>                          sym->name, NULL) == FAILURE)
>        return MATCH_ERROR;

Nitpick: I guess, the same holds for PRIVATE. Are there any other way to create a symbol that do not use access specifiers?
Comment 15 patchapp@dberlin.org 2007-07-27 05:47:20 UTC
Subject: Bug number PR32760

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-07/msg01960.html
Comment 16 Jerry DeLisle 2007-07-27 16:30:22 UTC
Subject: Bug 32760

Author: jvdelisle
Date: Fri Jul 27 16:30:10 2007
New Revision: 126981

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=126981
Log:
2007-07-27  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
	    Daniel Franke  <franke.daniel@gmail.com>

	PR fortran/32760
	* primary.c (match_variable): Do not call gfc_add_flavor if symbol has
	attribute of ACCESS_PUBLIC or ACCESS_PRIVATE already marked.

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

Comment 17 Jerry DeLisle 2007-07-27 16:34:03 UTC
Subject: Bug 32760

Author: jvdelisle
Date: Fri Jul 27 16:33:50 2007
New Revision: 126982

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=126982
Log:
2007-07-27  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/32760
	* gfortran.dg/private_type_7.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/private_type_7.f90
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 18 Jerry DeLisle 2007-07-27 16:53:00 UTC
Fixed on 4.3
Comment 19 Harald Anlauf 2008-01-22 22:39:04 UTC
It appears that the fix to PR34760 breaks my original testcase.
(r131624 from 20080118 still works, all builds after 20080120 fail).

Tobias?
Comment 20 Tobias Burnus 2008-01-22 22:55:37 UTC
> It appears that the fix to PR34760 breaks my original testcase.
> (r131624 from 20080118 still works, all builds after 20080120 fail).

O well, the checked in test case of this PR uses "WRITE(" (note the "(") while the original test case uses "PRINT *".
Comment 21 Tobias Burnus 2008-01-23 16:36:10 UTC
Another test case

module m
  public :: volatile
contains
  subroutine foo
    volatile :: bar
  end subroutine foo
  subroutine volatile
  end subroutine volatile
end module
Comment 22 Jerry DeLisle 2008-01-24 04:52:54 UTC
Putting this back in fixes the new test case in comment #21

Index: primary.c
===================================================================
--- primary.c   (revision 131752)
+++ primary.c   (working copy)
@@ -2538,6 +2538,10 @@ match_variable (gfc_expr **result, int e
                 || sym->attr.pointer || sym->as != NULL)
          flavor = FL_VARIABLE;
 
+       if (sym->attr.access == ACCESS_PUBLIC
+           || sym->attr.access == ACCESS_PRIVATE)
+         break;
+
        if (flavor != FL_UNKNOWN
            && gfc_add_flavor (&sym->attr, flavor, sym->name, NULL) == FAILURE)
          return MATCH_ERROR;

Shall we go back to this? or just add it in as above?  It does seem simpler and more elegant and it makes sense.  I am regression testing now.

Comment 23 Jerry DeLisle 2008-01-24 05:20:31 UTC
Oh, I see, the trick is dealing with implicit_11.f90 as well. and around and around we go.
Comment 24 Tobias Burnus 2008-01-24 14:14:49 UTC
How about the following? If we need a variable as actual argument, one should either have
  allocate(...., stat=istat)
or
  allocate(...., stat=istat, source=bar)
i.e. a ')' or a ','.  I think for "istat = ..." the flavor is not needed. What do you think were it will break now?


Index: primary.c
===================================================================
--- primary.c   (Revision 131778)
+++ primary.c   (Arbeitskopie)
@@ -2534,7 +2534,8 @@ match_variable (gfc_expr **result, int e
        if (sym->attr.external || sym->attr.procedure
            || sym->attr.function || sym->attr.subroutine)
          flavor = FL_PROCEDURE;
-       else if (gfc_peek_char () != '(' || sym->ts.type != BT_UNKNOWN
+       else if (gfc_peek_char () != ')' || gfc_peek_char () != ','
+                || sym->ts.type != BT_UNKNOWN
                 || sym->attr.pointer || sym->as != NULL)
          flavor = FL_VARIABLE;
Comment 25 Tobias Burnus 2008-01-24 15:09:01 UTC
> i.e. a ')' or a ','.  I think for "istat = ..." the flavor is not needed. What
> do you think were it will break now?

Answer: The following. I still wonder whether one should not check it in as interim solution for 4.3.0. The following program is presumably a valid Fortran 2003 program (without the ",source=t" it is valid F95), but the "integer," makes problems with my patch above; on the other hand it is needed for the check in allocate. I wonder how other compilers such as NAG f95 or g95 solve this problem.


module m
  public :: integer
  private :: istat
contains
  subroutine foo
    integer, allocatable :: s(:), t(:)
    allocate(t(5))
    allocate(s(4), stat=istat, source=t)
  end subroutine foo
  subroutine integer()
  end subroutine integer
end module m
Comment 26 Paul Thomas 2008-01-31 22:59:29 UTC
Created attachment 15071 [details]
A fix for the PR

This is regtesting as I write.  It fixes the first three PRs but not that of comment #25.  I believe that this will turn out to be a problem in patching allocate because

    write (*, *, iostat = isat) "hello"

is OK.

Paul
Comment 27 Paul Thomas 2008-02-01 06:00:59 UTC
(In reply to comment #26)
> Created an attachment (id=15071) [edit]
> A fix for the PR
> 
> This is regtesting as I write.  It fixes the first three PRs but not that of
> comment #25.  I believe that this will turn out to be a problem in patching
> allocate because

The patch regtests OK.

> 
>     write (*, *, iostat = isat) "hello"
> 
> is OK.


I will investigate this before submitting. I feel rather sure that a comparison of write and allocate will quickly reveal the problem.

Paul
Comment 28 Tobias Burnus 2008-02-01 09:34:20 UTC
> > A fix for the PR
> > This is regtesting as I write.  It fixes the first three PRs but not that of
> > comment #25.

I'm not so happy about the != '(' in:

+ 	/* These are definitive indicators that this is a variable.  */
  	else if (gfc_peek_char () != '(' || sym->ts.type != BT_UNKNOWN

As "integer, .... ::" shows,  != '('  is not a bullet-proof sign for a variable. (I should not complain too loudly, however, as I have written that part.)


+ 	else if (sym->ns == gfc_current_ns->parent
+ 		   && sym->ts.type == BT_UNKNOWN)
+ 	  break;

I think you can have:
 	else if (sym->ns == gfc_current_ns->parent
 		 && (sym->ts.type == BT_UNKNOWN || sym->attr.pointer
                     || sym->as != NULL))

As the following can also not be a procedure name:

module m
  pointer p
  dimension d(4)
contains
  ...

(A procedure can have a pointer attribute or be a vector but this is already taken care of by: "if(sym->attr.external || sym->attr.procedure || sym->attr.function || sym->attr.subroutine) flavor = FL_PROCEDURE;")

Thanks for looking into this.
Comment 29 Dominique d'Humieres 2008-02-02 21:31:55 UTC
With the patch in http://gcc.gnu.org/ml/fortran/2008-02/msg00006.html, I still get an error for the test case in comment #25:

pr32760_2.f90:8.29:

    allocate(s(4), stat=istat, source=t)
                            1
Error: Syntax error in ALLOCATE statement at (1)

Is this normal?

Comment 30 Dominique d'Humieres 2008-02-03 11:10:42 UTC
With the patch in http://gcc.gnu.org/ml/fortran/2008-02/msg00006.html, the test suite passed without new regression on ppc/intel darwin9, 32 and 64 bit modes.

As discussed on IRC, the test in comment #29 gives an error due to "source=t". It would be more helpful to have the pointer under the second (not expected) comma.  However I don't think this should block the patch.

The test case gfortran.dg/host_assoc_variable_1.f90 is compile only.  Would not it be safer to make it "dg-do run" to rule out unexpected side effects in future releases? If yes, the output should be checked by inserting something like (regtested)

! { dg-output " *27(\n|\r\n|\r)" }
! { dg-output " *in the main: *27(\n|\r\n|\r)" }
! { dg-output " *35937(\n|\r\n|\r)" }

before

! { dg-final { cleanup-modules "gfcbug68 gfcbug68a m n" } }

Thanks for the patch.


Comment 31 Paul Thomas 2008-02-03 11:30:17 UTC
Subject: Bug 32760

Author: pault
Date: Sun Feb  3 11:29:27 2008
New Revision: 132078

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132078
Log:
2008-02-03  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/32760
	* resolve.c (resolve_allocate_deallocate): New function.
	(resolve_code): Call it for allocate and deallocate.
	* match.c (gfc_match_allocate, gfc_match_deallocate) : Remove
	the checking of the STAT tag and put in above new function.
	* primary,c (match_variable): Do not fix flavor of host
	associated symbols yet if the type is not known.

2008-02-03  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/32760
	* gfortran.dg/host_assoc_variable_1.f90: New test.
	* gfortran.dg/allocate_stat.f90: Change last three error messages.

Added:
    trunk/gcc/testsuite/gfortran.dg/host_assoc_variable_1.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/match.c
    trunk/gcc/fortran/primary.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/allocate_stat.f90

Comment 32 Paul Thomas 2008-02-03 11:32:01 UTC
Fixed on trunk

Paul