Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 11 Jun 2012 23:29:08 +0200, Mark Kettenis wrote:
> > On Tue, 27 Mar 2012 20:53:37 +0200, Mark Kettenis wrote:
> > > Also, if i386_push_dummy_call() doesn't preserve alignment (and it sure
> > > looks like it doesn't), then aligning the stack here doesn't help.
> > 
> > The patch helps (it makes working cases which did not work before), I am
> > unable to find a user visible problem with it.
> 
> Sorry, but that must be sheer luck.

Here is no luck, your patch also does not do anything good with the alignment
as the new KFAIL tdep/14222 proves.  16 bytes shift is still better than
1 byte shift (as was in amd64-dicos-tdep.c and i386-dicos-tdep.c) as 1 byte
shift always breaks SSE inferiors.  But whether 16 bytes subtraction or
16 bytes modulo works is just the "sheer luck" depending on this or that case.

I will check GCC parameters alignment and fix PR tdep/14222 first otherwise
the code does not make sense.


> Like I said, that probably requires i386_push_dummy_call() to properly
> align the stack just like amd64_push_dummy_call() does.

Yes, PR tdep/14222.


> > It is again about different coding style.
> 
> Well, KISS is an important engineering principle if you ask me.

(a) The previous code in amd64-dicos-tdep.c and i386-dicos-tdep.c already used
    gdbarch_breakpoint_from_pc.  So moving a code is more simple patch than
    moving + changing the code.
(b) Your code:
      *bp_addr = sp - 1;
    I find definitely less readable than
      gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);
    What is "- 1" there?  There is no comment for it.  If anything you should
    have written:
      /* 1 is size of the i386 breakpoint instruction.  */
      *bp_addr = sp - 1;
    But in such case I find more obvious it is more readable to put there just:
      gdbarch_breakpoint_from_pc (gdbarch, &bppc, &bplen);


Jan