[PATCH,fortran]: fix bind(c) common block alignment

[PATCH,fortran]: fix bind(c) common block alignment

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 656 bytes --]

hi all,

as mentioned previously on the list by Jack Howarth, there is a mismatch 
in alignment with bind(c) common blocks.  the Fortran compiler simply sets 
the alignment of all common blocks to BIGGEST_ALIGN, while the C compiler 
tries to align based on the size of the fields in the interoperable C 
struct.  the attached patch should fix the alignment difference by 
changing the alignment setting of bind(c) common blocks.

bootstrapped and regtested on x86 linux with no new failures.

ChangeLog entry:

2007-08-06  Christopher D. Rickett  <crickett@lanl.gov>

 	* trans-common.c (build_common_decl): Fix the alignment for
 	BIND(C) common blocks.

[-- Attachment #2: Type: TEXT/plain, Size: 1103 bytes --]

Index: gcc/fortran/trans-common.c
===================================================================
--- gcc/fortran/trans-common.c	(revision 127182)
+++ gcc/fortran/trans-common.c	(working copy)
@@ -413,7 +413,20 @@ build_common_decl (gfc_common_head *com,
       SET_DECL_ASSEMBLER_NAME (decl, gfc_sym_mangled_common_id (com));
       TREE_PUBLIC (decl) = 1;
       TREE_STATIC (decl) = 1;
-      DECL_ALIGN (decl) = BIGGEST_ALIGNMENT;
+      if (!com->is_bind_c)
+	DECL_ALIGN (decl) = BIGGEST_ALIGNMENT;
+      else
+        {
+	  /* Do not set the alignment for bind(c) common blocks to
+	     BIGGEST_ALIGNMENT because that won't match what C does.  Also,
+	     for common blocks with one element, the alignment must be
+	     that of the field within the common block in order to match
+	     what C will do.  */
+	  tree field = NULL_TREE;
+	  field = TYPE_FIELDS (TREE_TYPE (decl));
+	  if (TREE_CHAIN (field) == NULL_TREE)
+	    DECL_ALIGN (decl) = TYPE_ALIGN (TREE_TYPE (field));
+	}
       DECL_USER_ALIGN (decl) = 0;
       GFC_DECL_COMMON_OR_EQUIV (decl) = 1;
 

Re: [PATCH,fortran]: fix bind(c) common block alignment

[Tobias Schlüter]

Christopher D. Rickett wrote:
> as mentioned previously on the list by Jack Howarth, there is a mismatch 
> in alignment with bind(c) common blocks.  the Fortran compiler simply 
> sets the alignment of all common blocks to BIGGEST_ALIGN, while the C 
> compiler tries to align based on the size of the fields in the 
> interoperable C struct.  the attached patch should fix the alignment 
> difference by changing the alignment setting of bind(c) common blocks.

How often does this change something?  It is a fairly widespread 
assumption that COMMON blocks can be mapped to C structs (e.g. I recall 
there being an example of doing this in the ROOT manual, and a quick 
websearch reveals lots of pages that describe this as a fact).  I.e. that
   COMMON /X/A,B,I(7),C
maps to
   struct { float a, b; int i[7]; float c; }
etc.
Since this seems to be really common, I'm surprised that we weren't hit 
by this before.

I think we should choose C compatible alignments everywhere possible.

A testcase that triggers on i86 would also be a good idea.

Cheers,
- Tobi

> bootstrapped and regtested on x86 linux with no new failures.
> 
> ChangeLog entry:
> 
> 2007-08-06  Christopher D. Rickett  <crickett@lanl.gov>
> 
>     * trans-common.c (build_common_decl): Fix the alignment for
>     BIND(C) common blocks.

Re: [PATCH,fortran]: fix bind(c) common block alignment

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2047 bytes --]

hi Tobias,

currently, the patch only changes the alignment if the common block is 
bind(c), which means the compiler should have already verified that the 
common block fields interop with C.

as to why we weren't seeing it before, i do not know.  the 
bind_c_coms.f90/bind_c_coms_driver.c illustrates on x86 linux that there 
is a difference in the alignment.  the generated assembly shows it, but 
the linker didn't complain.

if it is acceptable for all common blocks to choose a C compatible 
alignment, then the patch is trivial to change to do this.  is that what 
people prefer?

Chris

On Mon, 6 Aug 2007, Tobias Schlüter wrote:

> Christopher D. Rickett wrote:
>> as mentioned previously on the list by Jack Howarth, there is a mismatch in 
>> alignment with bind(c) common blocks.  the Fortran compiler simply sets the 
>> alignment of all common blocks to BIGGEST_ALIGN, while the C compiler tries 
>> to align based on the size of the fields in the interoperable C struct. 
>> the attached patch should fix the alignment difference by changing the 
>> alignment setting of bind(c) common blocks.
>
> How often does this change something?  It is a fairly widespread assumption 
> that COMMON blocks can be mapped to C structs (e.g. I recall there being an 
> example of doing this in the ROOT manual, and a quick websearch reveals lots 
> of pages that describe this as a fact).  I.e. that
>  COMMON /X/A,B,I(7),C
> maps to
>  struct { float a, b; int i[7]; float c; }
> etc.
> Since this seems to be really common, I'm surprised that we weren't hit by 
> this before.
>
> I think we should choose C compatible alignments everywhere possible.
>
> A testcase that triggers on i86 would also be a good idea.
>
> Cheers,
> - Tobi
>
>> bootstrapped and regtested on x86 linux with no new failures.
>> 
>> ChangeLog entry:
>> 
>> 2007-08-06  Christopher D. Rickett  <crickett@lanl.gov>
>>
>>     * trans-common.c (build_common_decl): Fix the alignment for
>>     BIND(C) common blocks.
>

Re: [PATCH,fortran]: fix bind(c) common block alignment

[François-Xavier Coudert]

> if it is acceptable for all common blocks to choose a C compatible
> alignment, then the patch is trivial to change to do this.  is that what
> people prefer?

I think we certainly do not want to rush that decision. I bit of
investigation, seeing what g77 did, what other compilers do, etc. is
certainly necessary before changing an established behaviour.

FX

Re: [PATCH,fortran]: fix bind(c) common block alignment

[Christopher D. Rickett]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 614 bytes --]

that sounds like a good idea.  however, for the case of bind(c), the patch 
should probably be considered since there the alignment mismatch is a 
bigger issue.

Chris

On Mon, 6 Aug 2007, François-Xavier Coudert wrote:

>> if it is acceptable for all common blocks to choose a C compatible
>> alignment, then the patch is trivial to change to do this.  is that what
>> people prefer?
>
> I think we certainly do not want to rush that decision. I bit of
> investigation, seeing what g77 did, what other compilers do, etc. is
> certainly necessary before changing an established behaviour.
>
> FX
>

Re: [PATCH,fortran]: fix bind(c) common block alignment

[H.J. Lu]

On Mon, Aug 06, 2007 at 11:17:01AM -0600, Christopher D. Rickett wrote:
> hi Tobias,
> 
> currently, the patch only changes the alignment if the common block is 
> bind(c), which means the compiler should have already verified that the 
> common block fields interop with C.
> 
> as to why we weren't seeing it before, i do not know.  the 
> bind_c_coms.f90/bind_c_coms_driver.c illustrates on x86 linux that there 
> is a difference in the alignment.  the generated assembly shows it, but 
> the linker didn't complain.
> 

The Linux linker can handle different alignments on the same common
symbol without problems. Linker will complain only if a definition of
a smaller alignment overrides a common symbol with a bigger alignment.
We have linker testcases to verify those. 


H.J.

Re: [PATCH,fortran]: fix bind(c) common block alignment

[Steve Kargl]

On Mon, Aug 06, 2007 at 11:17:01AM -0600, Christopher D. Rickett wrote:
> 
> currently, the patch only changes the alignment if the common block is 
> bind(c), which means the compiler should have already verified that the 
> common block fields interop with C.
> 
> as to why we weren't seeing it before, i do not know.  the 
> bind_c_coms.f90/bind_c_coms_driver.c illustrates on x86 linux that there 
> is a difference in the alignment.  the generated assembly shows it, but 
> the linker didn't complain.
> 
> if it is acceptable for all common blocks to choose a C compatible 
> alignment, then the patch is trivial to change to do this.  is that what 
> people prefer?
> 

One thing to keep in mind is derived types that appear in
common blocks.  These have SEQUENCE, and I suspect the 
current alignment guarantees that SEQUENCE gives expected
results. 

-- 
Steve

Re: [PATCH,fortran]: fix bind(c) common block alignment

[Christopher D. Rickett]

On Mon, 6 Aug 2007, Steve Kargl wrote:

> On Mon, Aug 06, 2007 at 11:17:01AM -0600, Christopher D. Rickett wrote:
>>
>> currently, the patch only changes the alignment if the common block is
>> bind(c), which means the compiler should have already verified that the
>> common block fields interop with C.
>>
>> as to why we weren't seeing it before, i do not know.  the
>> bind_c_coms.f90/bind_c_coms_driver.c illustrates on x86 linux that there
>> is a difference in the alignment.  the generated assembly shows it, but
>> the linker didn't complain.
>>
>> if it is acceptable for all common blocks to choose a C compatible
>> alignment, then the patch is trivial to change to do this.  is that what
>> people prefer?
>>
>
> One thing to keep in mind is derived types that appear in
> common blocks.  These have SEQUENCE, and I suspect the
> current alignment guarantees that SEQUENCE gives expected
> results.
>

in the case of bind(c) common blocks, however, this would not be possible 
since derived types with SEQUENCE are not interoperable (C1501).

Chris