[Bug libfortran/37839] New: st_parameter_dt has unwanted padding, is out of sync with compiler

[Bug libfortran/37839] New: st_parameter_dt has unwanted padding, is out of sync with compiler

[sje at cup dot hp dot com]

On systems where GFC_IO_INT is 8 bytes and requires 8 byte alignment the
st_parameter_dt structure may be out of sync between the fortran library and
compiler because the common field of st_parameter_dt is 28 bytes long (if
pointers and ints are 4 bytes each) and the next field (rec) is of type
GFC_IO_INT which is 8 bytes if 8 byte ints are supported.  The padding between
common and rec is not accounted for in the compiler when accessing the
structure.

On ia64-*-hpux* this cause gfortran.dg/f2003_io_[14567].f03 to fail in 32 bit
mode.


-- 
           Summary: st_parameter_dt has unwanted padding, is out of sync
                    with compiler
           Product: gcc
           Version: 4.4.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: libfortran
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: sje at cup dot hp dot com
GCC target triplet: ia64-*-hpux*


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[janis at gcc dot gnu dot org]

------- Comment #1 from janis at gcc dot gnu dot org  2008-10-15 23:09 -------
The tests listed in the submitter's description also fail (according to
archived testresults) on powerpc-unknown-linux-gnu, hppa-unknown-linux-gnu, and
powerpc-ibm-aix5.3.0.0.


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[burnus at gcc dot gnu dot org]

------- Comment #2 from burnus at gcc dot gnu dot org  2008-10-16 10:01 -------
The same as PR 37827.

Richard writes there:
"You cannot touch the library part, but you certainly can align what the
compiler does to the library part of the structure definition.  Thus, if the
behavior is wrong (as in wrong-code) at the moment, changing the "ABI" only in
parts that affect the wrong-code bug certainly is ok."


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[sje at cup dot hp dot com]

------- Comment #3 from sje at cup dot hp dot com  2008-10-16 15:23 -------
While changing the compiler would not break the ABI it seems like it would be
pretty difficult.  It looks to me like the compiler is building the
offsets/fields based on the types in ioparm.def and assumes no padding.  I want
to add padding but only in 32 bit mode and only on some platforms.


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[dominiq at lps dot ens dot fr]

------- Comment #4 from dominiq at lps dot ens dot fr  2008-10-16 15:32 -------
> I want to add padding but only in 32 bit mode and only on some platforms.

Are not these information provided by the config steps, hence available when
building gfortran?


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jvdelisle at gcc dot gnu dot org]

------- Comment #5 from jvdelisle at gcc dot gnu dot org  2008-10-17 03:40 -------
What if we do something like this:

typedef struct st_parameter_common
{
  GFC_INTEGER_4 flags;
  GFC_INTEGER_4 unit;
  const char *filename;
  GFC_INTEGER_4 line;
  CHARACTER2 (iomsg);
  GFC_INTEGER_4 *iostat;
#ifdef NEED_TO_PAD
  GFC_INTEGER_4 padit;
#endif
}
st_parameter_common;

Defining NEED_TO_PAD based on platform and whether or not -m32 ?


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jvdelisle at gcc dot gnu dot org]

------- Comment #6 from jvdelisle at gcc dot gnu dot org  2008-10-19 04:00 -------
*** Bug 37842 has been marked as a duplicate of this bug. ***


-- 

jvdelisle at gcc dot gnu dot org changed:

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


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[burnus at gcc dot gnu dot org]

------- Comment #7 from burnus at gcc dot gnu dot org  2008-10-19 12:01 -------
*** Bug 37827 has been marked as a duplicate of this bug. ***


-- 

burnus at gcc dot gnu dot org changed:

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


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[sje at cup dot hp dot com]

------- Comment #8 from sje at cup dot hp dot com  2008-10-20 15:02 -------
With respect to comment #5, the problem isn't changing the library side.  It is
changing the compiler side.  The compiler, as near as I can tell, doesn't
declare the structure the way the library does but builds offsets based on the
type information in ioparm.def.  Is is these offsets that need to change and
ioparm.def can't be ifdef'ed as is.  We would need to add an explicit
preprocessing of the file or add code to the compiler to adjust the offsets
after creating them from ioparm.def


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[sje at cup dot hp dot com]

------- Comment #9 from sje at cup dot hp dot com  2008-10-28 18:26 -------
Further investigation shows that it is not the size of common that is the
problem.  The bug is related to the new union of st_parameter_43 and
st_parameter_44.  Specifically, st_parameter_44 contains pos which is type
GFC_IO_INT which is 8 bytes on IA64 and requires an 8 byte alignment.  This
causes the entire st_parameter_44 structure to require 8 byte alignment and the
union that it is part of to require 8 byte alignment.  This changes the
layout/offset of fields in the p and q parts of the st_parameter_dt structure.

The compiler definition of st_parameter_dt, created from ioparm.def,  does not
know anything about the union that is used in the library and just lays out the
fields as if there was no *_44 structure and no union and this layout is then
different then the library layout.


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jvdelisle at gcc dot gnu dot org]

------- Comment #10 from jvdelisle at gcc dot gnu dot org  2008-11-01 12:17 -------
Steve, if I knew how to fix this, I would have done so by now.  I plan to
remove st43 and st44 in 4.5, this was merely a bandaid for the ABI breakage.


-- 

jvdelisle at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2008-11-01 12:17:18
               date|                            |


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[dominiq at lps dot ens dot fr]

------- Comment #11 from dominiq at lps dot ens dot fr  2008-11-06 14:03 -------
Now that the patch in http://gcc.gnu.org/ml/fortran/2008-11/msg00022.html has
been committed I get in 32 bit mode on powerpc-apple-darwin9:

XPASS: gfortran.dg/f2003_io_1.f03  -O0  execution test
...
XPASS: gfortran.dg/f2003_io_7.f03  -Os  execution test

i.e., the tests pass on powerpc-apple-darwin9. Does anybody understand why they
fail on non Darwin ppc, but not on Darwin?


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jakub at gcc dot gnu dot org]

------- Comment #12 from jakub at gcc dot gnu dot org  2008-11-20 15:08 -------
I think the primary question is, is libgfortran in 4.4 supposed to stay at
libgfortran.so.3?  If yes, then it must be backwards compatible with 4.3.

Looking at the 4.3 to 4.4 io.h changes, I see several problems.
The st_parameter_open changes look good to me.
st_parameter_inquire changes are wrong, io.h has:
@@ -299,6 +339,15 @@ typedef struct
   CHARACTER1 (write);
   CHARACTER2 (readwrite);
   CHARACTER1 (convert);
+  GFC_INTEGER_4 flags2;
+  CHARACTER1 (asynchronous);
+  CHARACTER2 (decimal);
+  CHARACTER1 (encoding);
+  CHARACTER2 (pending);
+  CHARACTER1 (round);
+  CHARACTER2 (sign);
+  GFC_INTEGER_4 *size;
+  GFC_INTEGER_4 *id;
 }
 st_parameter_inquire;

but ioparm.def has:
@@ -54,6 +59,17 @@ IOPARM (inquire, read,               1 << 27, char2)
 IOPARM (inquire, write,                1 << 28, char1)
 IOPARM (inquire, readwrite,    1 << 29, char2)
 IOPARM (inquire, convert,       1 << 30, char1)
+IOPARM (inquire, flags2,       1 << 31, int4)
+IOPARM (inquire, asynchronous, 1 << 0,  char1)
+IOPARM (inquire, decimal,      1 << 1,  char2)
+IOPARM (inquire, encoding,     1 << 2,  char1)
+IOPARM (inquire, pending,      1 << 3,  pint4)
+IOPARM (inquire, round,                1 << 4,  char1)
+IOPARM (inquire, sign,         1 << 5,  char2)
+IOPARM (inquire, size,         1 << 6,  pint4)
+IOPARM (inquire, id,           1 << 7,  pint4)
+IOPARM (wait,    common,       0,       common)
+IOPARM (wait,    id,           1 << 7,  pint4)
 #ifndef IOPARM_dt_list_format
 #define IOPARM_dt_list_format          (1 << 7)
 #define IOPARM_dt_namelist_read_mode   (1 << 8)

Look at pending, the types don't match.  Either it is a char2 (i.e. int length
followed by char *), but then it should be char2 in both places, or it is pint4
(i.e. a int *), but then it should be pint4 in both places and everything
adjusted, so that there aren't unneeded gaps.

st_parameter_dt changes are unfortunately worse.  Putting the FE filled new
fields into the union is definitely a bad idea, and as gfc_transfer_init clears
the whole u.pad, it is ABI incompatible anyway (4.3 compiled programs reserve
just 192 (32-bit) resp. 256 (64-bit) bytes for the padding, it will result in
writes after end of allocated st_parameter_dt variables in 4.3 running against
4.4 libgfortran.  The fix is easy.  Either move the newly added fields at the
end of the structure and decrease size of pad back to 16 * sizeof (char *) + 32
* sizeof (int), after all, there is plenty of space for new stuff left in there
and apparently no new libgfortran internal fields were added to u.p during 4.4.
 (this is my preferred solution) or, move the newly added FE filled fields
before the union and decrease size of the u.pad accordingly, then find out at
runtime where to put say the u.p.value field (it could overlap the newly added
FE filled fields if none of them are used, or if they are used, could be after
the current padding.

I'll work on a patch, but would like to hear first 1) whether 4.4 libgfortran
is really meant to be backward compatible with 4.3 (I hope so) and 2) what type
should pending in INQUIRE have.


-- 

jakub at gcc dot gnu dot org changed:

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


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[howarth at nitro dot med dot uc dot edu]

------- Comment #13 from howarth at nitro dot med dot uc dot edu  2008-11-21 12:43 -------
Can we change all of the gfortran.dg/f2003_io_*.f03 test cases from...

! { dg-xfail-run-if "PR37839" { { ia64-*-hpux* hppa*-*-* powerpc*-*-*
sparc*-*-* } && ilp32 } { "*" }  { "" } }

to

! { dg-xfail-run-if "PR37839" { { ia64-*-hpux* hppa*-*-*
powerpc*-*-[aix,linux]* sparc*-*-* } && ilp32 } { "*" }  { "" } }


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jakub at gcc dot gnu dot org]

------- Comment #14 from jakub at gcc dot gnu dot org  2008-11-21 12:44 -------
Testing a patch, so far it worked on dg.exp=f2003*.f03 including a new testcase
for F2003 INQUIRE on x86_64-linux (-m64, -m32) and powerpc64-linux (-m32,
-m64).


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |jakub at gcc dot gnu dot org
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2008-11-01 12:17:18         |2008-11-21 12:44:12
               date|                            |


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jvdelisle at gcc dot gnu dot org]

------- Comment #15 from jvdelisle at gcc dot gnu dot org  2008-11-22 05:21 -------
The PENDING= specifier needs to be a pointer to integer.  It is used to assign
a logical value of TRUE or FALse to the variable given value.

Thus:

LOGICAL :: alogicalvalue
INQUIRE(10,pending = alogicalvalue)
PRINT *, alogicalvalue ! This would print 'T' if IO was pending on UNIT 10

Thanks for helping with this mess. I am checking your patch.


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jakub at gcc dot gnu dot org]

------- Comment #16 from jakub at gcc dot gnu dot org  2008-11-22 08:12 -------
Subject: Bug 37839

Author: jakub
Date: Sat Nov 22 08:10:41 2008
New Revision: 142111

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142111
Log:
        PR libfortran/37839
        * trans-io.c (gfc_build_io_library_fndecls): Decrease pad size back
        to 16 pointers plus 32 integers.  Don't use max integer kind
        alignment, only gfc_intio_kind's alignment.
        (gfc_trans_inquire): Only set flags2 if mask2 is non-zero.
        * ioparm.def: Fix order, bitmasks and types of inquire round, sign
        and pending fields.  Move u in dt before id.
        * io.c (gfc_free_inquire): Free decimal and size exprs.
        (match_inquire_element): Match size instead of matching blank twice.
        (gfc_resolve_inquire): Resolve size.

        * gfortran.dg/f2003_inquire_1.f03: New test.
        * gfortran.dg/f2003_io_1.f03: Remove xfail.
        * gfortran.dg/f2003_io_4.f03: Likewise.
        * gfortran.dg/f2003_io_5.f03: Likewise.
        * gfortran.dg/f2003_io_6.f03: Likewise.
        * gfortran.dg/f2003_io_7.f03: Likewise.

        * io/io.h (IOPARM_INQUIRE_HAS_ROUND, IOPARM_INQUIRE_HAS_SIGN,
        IOPARM_INQUIRE_HAS_PENDING): Adjust values.
        (st_parameter_inquire): Reorder and fix types of round, sign and
        pending fields.
        (st_parameter_43, st_parameter_44): Removed.
        (st_parameter_dt): Put back struct definition directly to u.p
        declaration.  Change type of u.p.size_used from gfc_offset to
        GFC_IO_INT.  Decrease back size of u.pad to 16 pointers and
        32 ints.  Put id, pos, asynchronous, blank, decimal, delim,
        pad, round and sign fields after the union.
        * io/inquire.c (inquire_via_unit, inquire_via_filename): Only read
        flags2 if it is defined.
        * io/transfer.c (read_sf, read_block_form, write_block): Cast
        additions to size_used to GFC_IO_INT instead of gfc_offset.
        (data_transfer_init): Clear whole u.p struct.  Adjust
        for moving id, pos, asynchronous, blank, decimal, delim, pad,
        round and sign fields from u.p directly into st_parameter_dt.
        (finalize_transfer): Don't cast size_used to GFC_IO_INT.
        * io/file_pos.c (st_endfile): Clear whole u.p struct.

Added:
    trunk/gcc/testsuite/gfortran.dg/f2003_inquire_1.f03
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/io.c
    trunk/gcc/fortran/ioparm.def
    trunk/gcc/fortran/trans-io.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/f2003_io_1.f03
    trunk/gcc/testsuite/gfortran.dg/f2003_io_4.f03
    trunk/gcc/testsuite/gfortran.dg/f2003_io_5.f03
    trunk/gcc/testsuite/gfortran.dg/f2003_io_6.f03
    trunk/gcc/testsuite/gfortran.dg/f2003_io_7.f03
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/io/file_pos.c
    trunk/libgfortran/io/inquire.c
    trunk/libgfortran/io/io.h
    trunk/libgfortran/io/transfer.c


-- 


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

[Bug libfortran/37839] st_parameter_dt has unwanted padding, is out of sync with compiler

[jakub at gcc dot gnu dot org]

------- Comment #17 from jakub at gcc dot gnu dot org  2008-11-22 08:30 -------
Fixed.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


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