Bug 23290 - [4.0 Regression] Layout changed for structure with single complex field
Summary: [4.0 Regression] Layout changed for structure with single complex field
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.1.0
: P2 normal
Target Milestone: 4.1.1
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI, wrong-code
Depends on:
Blocks:
 
Reported: 2005-08-08 19:05 UTC by Jorn Wolfgang Rennecke
Modified: 2007-02-03 15:36 UTC (History)
3 users (show)

See Also:
Host:
Target: sh-*
Build:
Known to work: 4.1.0 4.1.1 4.1.2
Known to fail: 4.0.0
Last reconfirmed: 2005-12-27 00:24:58


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jorn Wolfgang Rennecke 2005-08-08 19:05:59 UTC
The patch for PR17112:

2004-09-26  Roger Sayle  <roger@eyesopen.com>
            Giovanni Bajo  <giovannibajo@gcc.gnu.org>

        PR middle-end/17112
        * stor-layout.c (compute_record_mode): For records with a single
        field, only use the field's mode if its size matches what we'd
        have choosen for the record ourselves.  This forces the use of
        BLKmode for packed records that don't completely fill a mode.

has changed the way structures are laid out, and as a consequence, the ABI.
I have become aware of this issue when I investigated an sh-elf -m4 regression:
FAIL: gcc.dg/compat/struct-by-value-15 c_compat_y_tst.o compile
sh_gimplify_va_arg_expr replaces a record type with its only element because
that used to be the way we passed these structs; with the unscheduled ABI change,
we end up copying a CDImode variable into a BLKmode one, and ICE.  The ICE
could be avoided in sh_gimplify_va_arg_expr by checking the mode of the type,
and I could also use the original type when building the statements, but the
real issue here is the ABI change.

The structure in question is:

typedef struct
{
  _Complex long long a;
} Scll1;

The mode for field a is computed as

mode_for_size (128, MODE_COMPLEX_INT, 0),

giving CDImode.  This is also the mode that we used to use for SCll1,
thus allowing the struct to be passed as a function argument in registers
(for the SH1..4, this can happen if it was the first or only argument
 elegible to be passed in integer registers).

With the change to compute_record_mode that was made on accounbt of PR17112,
we first compute an integer mode for the size, and only use the member
mode if it matches the integer mode in size.  This integer mode is calculated
layout_type with:

mode_for_size (128, MODE_INT, 1)

I.e. limit is set to 1, meaning that no mode larger than MAX_FIXED_MODE_SIZE
will be returned.  For SH1..SH4, MAX_FIXED_MODE_SIZE is 64, thus mode_for_size
returns BLKmode, the comparison fails, and Scll1 ends up as BLKmode.
The BLKmode layout forces Scll1 values on the stack.
I think for MODE_COMPLEX_INT and MODE_COMPLEX_FLOAT, we should only check
that size of the record matches the mode.

From looking at the code, I also see that vectors are also laied out using
a limit of 0, so a similar problem arises there.
Comment 1 Giovanni Bajo 2005-08-09 23:12:00 UTC
So, using limit 0 for when calculating the integer mode for the size would fix 
the regression on sh? 
Comment 2 Jorn Wolfgang Rennecke 2005-08-15 14:14:36 UTC
(In reply to comment #1)
> So, using limit 0 for when calculating the integer mode for the size would fix 
> the regression on sh? 

Yes, it would fix the problem with CDImode, however, the mode_for_size_tree
call in compute_record_mode is not only used to determine if any mode found
from a single member would be suitable, but also the what mode to use otherwise.
You don't really want to change what you use for the latter.
For vector modes, there is also the added complication that there might be no
integer mode at all wide enough to match the size of the vector mode. 

Comment 3 GCC Commits 2005-09-12 13:50:16 UTC
Subject: Bug 23290

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	amylaar@gcc.gnu.org	2005-09-12 13:50:03

Modified files:
	gcc            : ChangeLog stor-layout.c 

Log message:
	PR middle-end/23290
	* stor-layout.c (compute_record_mode): For records with a single
	field, actually check the field's mode size against the type size.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.9937&r2=2.9938
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/stor-layout.c.diff?cvsroot=gcc&r1=1.241&r2=1.242

Comment 4 Andrew Pinski 2005-09-12 14:52:40 UTC
Confirmed, only a 4.0 regression now.
Comment 5 Gabriel Dos Reis 2007-02-03 15:36:27 UTC
Fixed in GCC-4.1.1 and higher.