C++ structure layout breakage

Jim Wilson wilson@cygnus.com
Fri Oct 20 15:11:00 GMT 2000


Your large March 25 change
	* Rework fields used to describe positions of bitfields and
	modify sizes to be unsigned and use HOST_WIDE_INT.
includes changes to compute_record_mode in stor-layout.c.
	 (compute_record_mode): Rework to simplify and to use new DECL fields.

Part of this change included changing this old code:
!         if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
!           mode = DECL_MODE (field);
to this new code:
!       if (field == TYPE_FIELDS (type) && TREE_CHAIN (field) == 0)
!       mode = DECL_MODE (field);

The new code does not work for C++, because we can't assume that a field
covers the entire struct if it is the only field.  There could be constants,
or method functions, or other stuff in the structure.  So the old code worked,
and the new code doesn't.  I don't see any reason for this change.  The
patch claims to be a cleanup, but this is a semantic change.  The reasonable
assumption here is that this was an error.

This problem was further compliated by another change Aug 18, that changed
the code to
      if (field == TYPE_FIELDS (type) && TREE_CHAIN (field) == 0
	  && int_mode_for_mode (DECL_MODE (field)) != BLKmode)
	    mode = DECL_MODE (field);
I believe this would not be necessary if the original code was used, since the
size of the structure would not match the size of the field if it wasn't an
integer mode.  Also, this seems to make it impossible to use XFmode for
structures containing a 80/96 long double, and this is undesirable.

This problem was again further complicated by another change today (Oct 20)
that changed the code again to
      if (field == TYPE_FIELDS (type) && TREE_CHAIN (field) == 0
	  && int_mode_for_mode (DECL_MODE (field)) != BLKmode
	  && operand_equal_p (DECL_SIZE (field), TYPE_SIZE (type), 1))
	    mode = DECL_MODE (field);
Now you've readded the DECL_SIZE/TYPE_SIZE check, but you wrote it differently
and the first two bad TYPE_FIELDS/TREE_CHAIN tests are still there which means
it still won't work for C++.

I propose to partially revert your March 25, Aug 18, and Oct 20 changes so as
to restore the original working code.  I will do so unless you can provide an
explanation of why the new code is necessary.  The corrected code will look
like this
	if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field))
	  mode = DECL_MODE (field);

This problem was noticed on the IA-64 port, because some C++ programs were
getting aborts in emit_move_insn because some C++ structures had the wrong
mode.  Here is a trivial example.

class foo
{
  double bar;
} foobar;

class foo sub (void) { return foobar; }

Before March 25, class foo was DFmode.  After March, it was DImode.  This
affects the C++ ABI, since DFmode/DImode values will be passed/returned
differently, so it is important to get this right.

The IA-64 backend tries to return this value in a DFmode FP register, and
this fails because we can't copy a DImode structure into a DFmode register.
I can workaround this by returning a PARALLEL instead of a DFmode reg, but
I believe fixing compute_record_mode is the correct fix here.

Jim


More information about the Gcc-patches mailing list