[Bug fortran/35037] New: VOLATILE attribute not being honored with common block variable

[Bug fortran/35037] New: VOLATILE attribute not being honored with common block variable

[w6ws at earthlink dot net]

The following code demonstrates that when optimization is turned on, the memory
reference is being moved out of the loop.  Thus, the volatile attribute is not
being honored.

As a workaround (and perhaps better solution), the user could use a module
instead of a common block.  Gfortran works ok in the case of module variables.

The below example is from x86 cygwin.  But the same thing happens with x86_64
Linux on the current trunk.

$ cat vol.f
      subroutine wait4it ()
        implicit none

        logical event
        volatile event
        common /xyzzy/ event

        do
          if (event) exit
        end do

      end subroutine

$ gfortran -O -S vol.f

$ cat vol.s
        .file   "vol.f"
        .text
.globl _wait4it_
        .def    _wait4it_;      .scl    2;      .type   32;     .endef
_wait4it_:
        pushl   %ebp
        movl    %esp, %ebp
        cmpl    $0, _xyzzy_
        jne     L4
L5:
        jmp     L5
L4:
        popl    %ebp
        ret
        .comm   _xyzzy_, 16      # 4

$

$ gfortran --version
GNU Fortran (GCC) 4.3.0 20071222 (experimental) [trunk revision 127783]
Copyright (C) 2007 Free Software Foundation, Inc.

GNU Fortran comes with NO WARRANTY, to the extent permitted by law.
You may redistribute copies of GNU Fortran
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING


$


-- 
           Summary: VOLATILE attribute not being honored with common block
                    variable
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: w6ws at earthlink dot net
 GCC build triplet: i686-pc-cygwin
  GCC host triplet: i686-pc-cygwin
GCC target triplet: i686-pc-cygwin


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[w6ws at earthlink dot net]

------- Comment #1 from w6ws at earthlink dot net  2008-01-31 03:09 -------
Adding myself to the cc list.


-- 

w6ws at earthlink dot net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |w6ws at earthlink dot net


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[burnus at gcc dot gnu dot org]

------- Comment #2 from burnus at gcc dot gnu dot org  2008-01-31 13:59 -------
trans-decl has:

  if (sym->attr.volatile_)
    {
      TREE_THIS_VOLATILE (decl) = 1;
      new = build_qualified_type (TREE_TYPE (decl), TYPE_QUAL_VOLATILE);
      TREE_TYPE (decl) = new;
    }

However, variables which are in COMMON blocks have their symbols generated in
trans-common.c.

One should try to mark as little as possible as VOLATILE to allow more
optimization. (Fortran 2003 allows a use/host-associated variable to be marked
as volatile, i.e. if it is used elsewhere it is not volatile.)

As vendor extension (incl. g77) one can mark also a complete common block as
volatile: PR 34928.

EQUIVALENT should be checked as well.


-- 

burnus at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu dot
                   |                            |org
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   GCC host triplet|i686-pc-cygwin              |
 GCC target triplet|i686-pc-cygwin              |
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2008-01-31 13:59:37
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[fxcoudert at gcc dot gnu dot org]

------- Comment #3 from fxcoudert at gcc dot gnu dot org  2008-02-01 22:54 -------
I've tried the following "trivial" patch (which should handle both common
blocks and equivalences):

Index: trans-common.c
===================================================================
--- trans-common.c      (revision 131944)
+++ trans-common.c      (working copy)
@@ -678,6 +678,14 @@ create_common (gfc_common_head *com, seg
       /* This is a fake variable just for debugging purposes.  */
       TREE_ASM_WRITTEN (var_decl) = 1;

+      if (s->sym->attr.volatile_)
+       {
+         tree new;
+         TREE_THIS_VOLATILE (var_decl) = 1;
+         new = build_qualified_type (TREE_TYPE (var_decl),
TYPE_QUAL_VOLATILE);
+         TREE_TYPE (var_decl) = new;
+       }
+
       if (com)
        var_decl = pushdecl_top_level (var_decl);
       else


But it looks like it doesn't work. Actually, even without the common block, it
doesn't seem to work:

      subroutine wait4it ()
        logical event
        volatile event
        do
          if (event) exit
        end do
      end subroutine

But I'm useless at reading assembly (the tree dump doesn't have these
attributes written out) and I'm not too sure if the code above is legal, so I'd
be glad for someone to help.


-- 

fxcoudert at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fxcoudert at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[burnus at gcc dot gnu dot org]

------- Comment #4 from burnus at gcc dot gnu dot org  2008-02-04 20:58 -------
> But I'm useless at reading assembly (the tree dump doesn't have these
> attributes written out)

Try something like the following with -O3 -fdump-tree-optimized:

      subroutine wait4it ()
        logical event
        volatile event
!        common /dd/ event
        event = .false.
        do
          if (event) print *, 'gfhzjf'
        end do
      end subroutine

I did not test your patch, but without the common line, the dump contains
"gfhzjf" until you remove the volatile. This saves you from reading assembler.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[fxcoudert at gcc dot gnu dot org]

------- Comment #5 from fxcoudert at gcc dot gnu dot org  2008-02-04 22:51 -------
(In reply to comment #4)
> I did not test your patch, but without the common line, the dump contains
> "gfhzjf" until you remove the volatile. This saves you from reading assembler.

Thanks! This allowed me to realize that my patch didn't work: I was marking the
"fake" variable we create for debugging purposes, not the field of the
structure/union. The following patch works, this time:

Index: trans-common.c
===================================================================
--- trans-common.c      (revision 131944)
+++ trans-common.c      (working copy)
@@ -318,6 +318,15 @@ build_field (segment_info *h, tree union
       GFC_DECL_ASSIGN_ADDR (field) = pushdecl_top_level (addr);
     }

+  /* If this field is volatile, mark it.  */
+  if (h->sym->attr.volatile_)
+    {
+      tree new;
+      TREE_THIS_VOLATILE (field) = 1;
+      new = build_qualified_type (TREE_TYPE (field), TYPE_QUAL_VOLATILE);
+      TREE_TYPE (field) = new;
+    }
+
   h->field = field;
 }


It makes the variable volatile in the following examples (common and
equivalence, since they are handled commonly by the codepath I modified):

      subroutine wait4it ()
        logical event
        volatile event
        common /dd/ event
        event = .false.
        do
          if (event) print *, 'gfhzjf'
        end do
      end subroutine

      subroutine wait4it2 ()
        logical event
        integer i
        volatile event
        equivalence(event,i)
        event = .false.
        do
          if (event) print *, 'gfhzjf'
        end do
      end subroutine

We don't, however, handle the case where we mark volatile the other variable
involved in the equivalence:

      subroutine wait4it3 ()
        logical event
        integer i
        volatile i
        equivalence(event,i)
        event = .false.
        do
          if (event) print *, 'gfhzjf'
        end do
      end subroutine

Do we need it? I'm not sure. I've checked what other compilers do: Intel and
Sun do not optimize out the PRINT statement if variable "i" is marked as
volatile; g95 seems to never optimize out variables in equivalences. I've also
checked what GCC does for C code, and it is does behave the same as my patch
above, ie the following is indeed optimized out:

void wait4it ()
{
  union { int event; volatile int i; } dd;

  dd.event = 0;
  while (1)
  {
    if (dd.event)
      __builtin_puts ("gfhzjf");
  }
}

>From the standard point of view, when you equivalence A and B, you're not
supposed to read B after having written to A, so I would think the behaviour I
propose here is standard-conforming. I've asked for opinions on c.l.f
(http://groups.google.com/group/comp.lang.fortran/browse_thread/thread/9602660696e0170e).


-- 

fxcoudert at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |fxcoudert at gcc dot gnu dot
                   |dot org                     |org
             Status|NEW                         |ASSIGNED
  GCC build triplet|i686-pc-cygwin              |
           Keywords|                            |patch
   Last reconfirmed|2008-01-31 13:59:37         |2008-02-04 22:51:44
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[fxcoudert at gcc dot gnu dot org]

------- Comment #6 from fxcoudert at gcc dot gnu dot org  2008-02-05 00:42 -------
(In reply to comment #5)
> We don't, however, handle the case where we mark volatile the other variable
> involved in the equivalence:

OK, Richard Maine and Steve Lionel confirmed that equivalence shouldn't
interfere with the volatile attribute, so I'll submit my patch when 4.4
branches. Or is it 4.3 material? (it certainly is of limited impact, and fixes
a wrong-code bug, but it's not a regression).


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[w6ws at earthlink dot net]

------- Comment #7 from w6ws at earthlink dot net  2008-02-05 01:25 -------
Subject: Re:  VOLATILE attribute not being honored with
 common block variable

Gosh - one learns something everyday.  The bit with EQUIVALENCE is an
interesting twist!

It seems that F2003 would allow maximum flexibility.  One of my
complaints about VOLATILE has been that it is too 'strong'.  The
attribute lasts over the entire scope of the variable - whereas one
might only need to ensure coherency with an external process at a
specific point in the routine.  (E.g., the spin loop in the example.)

With the F2003 definition, one can use the name with the volatile
attribute at that point, yet use its alias for maximum optimization
at other places. So I actually like the F2003 definition and would
prefer that the non-VOLATILEd name did not get the attribute.

Walter


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[fxcoudert at gcc dot gnu dot org]

------- Comment #8 from fxcoudert at gcc dot gnu dot org  2008-02-05 21:07 -------
Subject: Bug 35037

Author: fxcoudert
Date: Tue Feb  5 21:06:32 2008
New Revision: 132129

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132129
Log:
        PR fortran/35037

        * trans-common.c (build_field): Mark fields as volatile when needed.

        * gfortran.dg/volatile11.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/volatile11.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-common.c
    trunk/gcc/testsuite/ChangeLog


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037

[Bug fortran/35037] VOLATILE attribute not being honored with common block variable

[fxcoudert at gcc dot gnu dot org]

------- Comment #9 from fxcoudert at gcc dot gnu dot org  2008-02-05 21:08 -------
This is fixed on 4.3.


-- 

fxcoudert at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.3.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35037