This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PR/14262 (ABI corner case confusion)


Jim Wilson wrote:
> True, but we should still be cautious.  The calling convention code is
> very complicated, and it is very easy to break it without realizing it.

Sure, I agree.

> It is a little more complicated than that.  FUNCTION_ARG_PADDING is used
> for arguments passed in memory, but it is not consistently used for
> arguments passed in registers.  It would be very unwise to change this
> except as part of a major gcc revision, e.g. gcc-4, because of the risk
> of silent ABI changes for some targets whose ABI is defined by gcc.
> 
> As of gcc-3.4, it will be used for args in regs if BLOCK_REG_PADDING is
> defined.  This is why I mentioned BLOCK_REG_PADDING as a possible
> solution.  This is a new invention, that does use FUNCTION_ARG_PADDING. 
> This is deliberately a new macro, so that there was no risk of breaking
> old targets.

What I meant is that if you check all the locations that test for
BLOCK_REG_PADDING: if it is not defined, they will behave just as
if it were defined identical to DEFAULT_FUNCTION_ARG_PADDING.
The only exception to this rule was the one place my patch fixes.

> There is a corresponding change to assign_parms.  assign_parms is
> supposed to write the whole register when storing a BLKmode arg to the
> stack.  It calls move_block_from_reg in gcc-3.2 and earlier.  In
> gcc-2.5.8 it writes the whole register.  In gcc-2.6.3 it does not. 
> Lovely, this one is my fault.  It was a patch I wrote to try to fix
> problems with the mips calling convention, which also passes BLKmode
> args in register, and which had ugly hacks int the mips backend to try
> to make this work.  This seems to be the whole cause of the mess,
> because this includes a locate_and_pad_parm change which prompted the
> later changes I described above which further confused the situation.

OK, I see.

> So based on that analysis, I would say that your original
> load_register_parameters patch is OK.

Thanks, I've committed the patch now.

> Except for the problem that the current code in load_register_parms can
> read past the end of memory.  The code could be changed to call
> mode_for_size and load the object directly without shifting if
> possible.  That solves part of the problem.  Otherwise, we probably have
> to use the bitfield insert/extract trick here, but at the moment we have
> no proof that the code is broken, so this can probably be deferred until
> later.

It is broken: the modified test case appended below segfaults 
because of the problem you point out.  I'll see if I can come
up with a patch to change load_register_parms as you suggest.

Bye,
Ulrich

#include <sys/mman.h>

typedef char   ACS;
typedef char   LSM;
typedef char   PANEL;
typedef char   DRIVE;
typedef struct {
    ACS             acs;
    LSM             lsm;
} LSMID;
typedef struct {
    LSMID           lsm_id;
    PANEL           panel;
} PANELID;
typedef struct {
    PANELID         panel_id;
    DRIVE           drive;
} DRIVEID;

void sub (DRIVEID driveid)
{
  if (driveid.drive != 1)
    abort ();
  if (driveid.panel_id.panel != 2)
    abort ();
  if (driveid.panel_id.lsm_id.lsm != 3)
    abort ();
  if (driveid.panel_id.lsm_id.acs != 4)
    abort ();
}

int main (void)
{
  DRIVEID *p;

  p = (DRIVEID *)mmap (0, 8192, PROT_READ|PROT_WRITE, 
		       MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
  if (p == (DRIVEID *)MAP_FAILED)
    return 0;
  mprotect (p + 1024, 4096, PROT_NONE);

  p[1023].drive = 1;
  p[1023].panel_id.panel = 2;
  p[1023].panel_id.lsm_id.lsm = 3;
  p[1023].panel_id.lsm_id.acs = 4;
  sub (p[1023]);

  return 0;
}


-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]