This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 12 Jun 2012 09:36:54 +0200
- Subject: Re: ping: [patch 2/2] Fix gdb.cp/gdb2495.exp regression with gcc-4.7 #5
- References: <20120309210117.GB30432@host2.jankratochvil.net> <20120326190414.GB11001@host2.jankratochvil.net> <201203271853.q2RIrbWf024897@glazunov.sibelius.xs4all.nl> <20120611191008.GA29811@host2.jankratochvil.net> <201206112129.q5BLT8ck007831@glazunov.sibelius.xs4all.nl>
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