[patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil]

On Sun, 22 Jul 2012 21:00:55 +0200, Philippe Waroquiers wrote:
> The problem with the above technique is that there is no valid
> instruction at the ON_STACK breakpoint address, and the valgrind
> translator does not like this.

Therefore is it enough for valgrind to fix it by the patch below?
It would be a good GDB user convenience fix anyway.

Former:
(gdb) up
#1  <function called from gdb>
(gdb) x/i $pc
=> 0x455210 <_start>:	xor    %ebp,%ebp

Current:
(gdb) up
#1  <function called from gdb>
(gdb) x/i $pc
=> 0x7fffffffda8f:	add    %al,(%rax)

Current patched:
(gdb) up
#1  <function called from gdb>
(gdb) x/i $pc
=> 0x7fffffffda0f:	hlt    
(gdb) 


Thanks,
Jan


gdb/
2012-07-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* i386-tdep.c (i386_push_dummy_code): New variable hlt.  Call
	write_memoryg for it.

diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 84e9794..712f0ff 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -2340,10 +2340,17 @@ i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
 		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
 		      struct regcache *regcache)
 {
+  /* This hlt instruction is never executed.  */
+  static const bfd_byte hlt = 0xf4;
+
   /* Use 0xcc breakpoint - 1 byte.  */
   *bp_addr = sp - 1;
   *real_pc = funaddr;
 
+  /* While inferior execution will trap on the 0xcc int3 instruction user
+     investigating the memory from GDB could see uninitialized bytes.  */
+  write_memory (*bp_addr, &hlt, sizeof (hlt));
+
   /* Keep the stack aligned.  */
   return sp - 16;
 }

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Joel Brobecker]

> 2012-07-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* i386-tdep.c (i386_push_dummy_code): New variable hlt.  Call
> 	write_memoryg for it.

Not really a review (not the maintainer), but it looks like a good
idea. It even seems to me that this should be done on all platforms,
no? If agreed, perhaps this should be a gdbarch-specific part of the
infcall sequence. And instead of writing an instruction of the
arch's choosing, why not write the breakpoint trap instruction?

In the meantime, a quick fix like yours seems like a good first step.

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 84e9794..712f0ff 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -2340,10 +2340,17 @@ i386_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, CORE_ADDR funaddr,
>  		      CORE_ADDR *real_pc, CORE_ADDR *bp_addr,
>  		      struct regcache *regcache)
>  {
> +  /* This hlt instruction is never executed.  */
> +  static const bfd_byte hlt = 0xf4;

Why make it static? Isn't that going to force the compiler to make
that variable global (put into RO section)?

> +  /* While inferior execution will trap on the 0xcc int3 instruction user
> +     investigating the memory from GDB could see uninitialized bytes.  */
> +  write_memory (*bp_addr, &hlt, sizeof (hlt));

I suggest merging the two comments into one at the point where the
intruction is written.

     /* Write an legitimate instruction at the point where the infcall
        breakpoint is going to be inserted.  While this instruction
        is never going to be executed, a user investigating the memory
        from GDB would see this instruction instead of random
        uninitialized bytes.  */

-- 
Joel

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil]

On Mon, 23 Jul 2012 17:59:51 +0200, Joel Brobecker wrote:
> It even seems to me that this should be done on all platforms, no?

Yes; just looking at the other archs it was not trivial to me so I wanted to
be sure it at least really helps valgrind.


> And instead of writing an instruction of the
> arch's choosing, why not write the breakpoint trap instruction?

It depends on the opinion.  I wanted to make clear there is a real GDB
breakpoint on top of it and in normal GDB control flow the 'hlt' instruction
is never executed by CPU.  There are other reasons why to put int3 there,
maybe it would be more clear to the users that way when I think about it now.

It also does not matter much, users do not probably disassemble
the <function called from gdb> frame.


> > +  /* This hlt instruction is never executed.  */
> > +  static const bfd_byte hlt = 0xf4;
> 
> Why make it static? Isn't that going to force the compiler to make
> that variable global (put into RO section)?

It is a nitpick but 'static const' is more effective than just 'const'.
In the latter case compiler has to create the storage for variable on stack
(probably to at least ensure its unique address), in the former case it just
takes an address of .rodata variable, which is generally cheaper.

const
   4:	48 8d 7c 24 08       	lea    0x8(%rsp),%rdi
   9:	c7 44 24 08 2a 00 00 	movl   $0x2a,0x8(%rsp)
  10:	00 
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  ---- no .rodata
vs.
static const
   4:	bf 00 00 00 00       	mov    $0x0,%edi
			5: R_X86_64_32	.rodata
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 5] .rodata           PROGBITS        0000000000000000 00005c 000004 00   A  0   0  4


> I suggest merging the two comments into one at the point where the
> intruction is written.
> 
>      /* Write an legitimate instruction at the point where the infcall
>         breakpoint is going to be inserted.  While this instruction
>         is never going to be executed, a user investigating the memory
>         from GDB would see this instruction instead of random
>         uninitialized bytes.  */

OK, I will update the patch later for check-in.


Thanks,
Jan

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Mon, 2012-07-23 at 18:35 +0200, Jan Kratochvil wrote:
> On Mon, 23 Jul 2012 17:59:51 +0200, Joel Brobecker wrote:
> > It even seems to me that this should be done on all platforms, no?
> 
> Yes; just looking at the other archs it was not trivial to me so I wanted to
> be sure it at least really helps valgrind.

Yes, that will help.

To avoid the need for the "grow" guess, Valgrind gdbsrv will need both
the Z0 packet (so as to have the breakpoint helperc inserted at
translation time) and the breakpoint trap instruction (to avoid
encountering random instruction when translating the instructions
on the stack).

Valgrind decoder stops decoding when it encounters the trap
instruction. So, writing the trap instruction + Z0 packet is good
enough, there is no need for an hlt instruction (but I see no problem
of having this hlt instruction).

Note that the trap instruction should only be written by the
push_dummy_code function : for Normal breakpoints, only a Z0 packet
should be done, as Valgrind will not allow to modify the guest
executable code (it is not mapped writable). 

I will currently not commit the "grow guess" patch in Valgrind, waiting
to see if the above approach is done in GDB (as this is a lot cleaner
that the "grow guess", which is a somewhat fragile heuristic kludge).

Thanks for all that,

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil]

On Mon, 23 Jul 2012 22:07:27 +0200, Philippe Waroquiers wrote:
> Note that the trap instruction should only be written by the
> push_dummy_code function : for Normal breakpoints, only a Z0 packet
> should be done, as Valgrind will not allow to modify the guest
> executable code (it is not mapped writable). 

I do not understand now what is and what is not allowed for valgrind to write.

For the inferior call to work at all you have to create the stack frame for
it, otherwise it cannot work, at least for parameters passed by stack.

So the GDB patch is no longer needed when you have fixed valgrind to put 0xcc
during Z0? Why valgrind cannot write 0xcc into stack memory when it already
has to write there to create the stack frame / parameters passed by stack?

(Yes, I should read valgrind source code instead.)

IIUC the 'hlt' cleanup patch can go only for 7.6 as 7.5 should not regress
with new/fixed valgrind.


Thanks,
Jan

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Mon, 2012-07-23 at 22:16 +0200, Jan Kratochvil wrote:
> On Mon, 23 Jul 2012 22:07:27 +0200, Philippe Waroquiers wrote:
> > Note that the trap instruction should only be written by the
> > push_dummy_code function : for Normal breakpoints, only a Z0 packet
> > should be done, as Valgrind will not allow to modify the guest
> > executable code (it is not mapped writable). 
> 
> I do not understand now what is and what is not allowed for valgrind to write.

The file mapped code (main program, shared libs) is not writable, and
so cannot be modified by Valgrind gdbsrv.

But Valgrind gdbsrv can modify all the memory which is writable.
So, a.o. it can modify the stack.

> 
> For the inferior call to work at all you have to create the stack frame for
> it, otherwise it cannot work, at least for parameters passed by stack.
> 
> So the GDB patch is no longer needed when you have fixed valgrind to put 0xcc
> during Z0? Why valgrind cannot write 0xcc into stack memory when it already
> has to write there to create the stack frame / parameters passed by stack?
Effectively, I have a patch which fixes the problem.
But the patch is a kludge which heuristically guesses that GDB is
pushing an infcall.

> 
> (Yes, I should read valgrind source code instead.)
> 
> IIUC the 'hlt' cleanup patch can go only for 7.6 as 7.5 should not regress
> with new/fixed valgrind.
infcall Valgrind gdbsrv tests are (currently) regressing with 7.4.91
It would be nice to have it fixed in 7.5 (so that no user can encounter
the nasty error message output by Valgrind) but this is not a critical
blocking problem.
So, up to you to see in which GDB release it can go.

If there will be a clean solution in GDB (7.5 or 7.6), then I will not
commit the kludge in Valgrind.

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Joel Brobecker]

> If there will be a clean solution in GDB (7.5 or 7.6), then I will not
> commit the kludge in Valgrind.

I also think that it would be friendlier to all valgrind users if
we tried to fix the problem on the GDB side before 7.5 gets out.
It seems like it would be easy enough, or is something else other
than Jan's patch necessary?

-- 
Joel

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Wed, 2012-07-25 at 07:48 -0700, Joel Brobecker wrote:
> > If there will be a clean solution in GDB (7.5 or 7.6), then I will not
> > commit the kludge in Valgrind.
> 
> I also think that it would be friendlier to all valgrind users if
> we tried to fix the problem on the GDB side before 7.5 gets out.
> It seems like it would be easy enough, or is something else other
> than Jan's patch necessary?
IIUC, Jan's patch was to have GDB storing a hlt after the place
where GDBSERVER will put 0xCC.
What Valgrind needs is to have GDB storing the 0xCC
(the hlt will then have no positive nor negative impact on Valgrind).

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil]

On Wed, 25 Jul 2012 22:04:01 +0200, Philippe Waroquiers wrote:
> IIUC, Jan's patch was to have GDB storing a hlt after the place
> where GDBSERVER will put 0xCC.

Not after, all the storage happens for the same byte.

hlt is stored there by memory write, int3 is stored there by Z0.


> What Valgrind needs is to have GDB storing the 0xCC
> (the hlt will then have no positive nor negative impact on Valgrind).

I will put 0xcc == int3 there although I believe valgrind could decode hlt (or
even nop) the same way.


Thanks,
Jan

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Wed, 2012-07-25 at 22:11 +0200, Jan Kratochvil wrote:
> On Wed, 25 Jul 2012 22:04:01 +0200, Philippe Waroquiers wrote:
> > IIUC, Jan's patch was to have GDB storing a hlt after the place
> > where GDBSERVER will put 0xCC.
> 
> Not after, all the storage happens for the same byte.
Oops, I misunderstood that.
> 
> hlt is stored there by memory write, int3 is stored there by Z0.
> 
> 
> > What Valgrind needs is to have GDB storing the 0xCC
> > (the hlt will then have no positive nor negative impact on Valgrind).
> 
> I will put 0xcc == int3 there although I believe valgrind could decode hlt (or
> even nop) the same way.
I have not found the decoding of the hlt instruction in Valgrind, so
I suspect it might not be supported.

Any valid instruction that will terminate a "Valgrind block" will be ok
there. For sure, the 0xcc is ok. There are other instructions which also
ensures Valgrind stops translating the block (e.g. others "int"
instructions). I think "nop" does not stop the block translation.

So, inserting 0xcc looks more straightforward/natural/safer.

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Pedro Alves]

On 07/23/2012 09:36 PM, Philippe Waroquiers wrote:

>> So the GDB patch is no longer needed when you have fixed valgrind to put 0xcc
>> during Z0? Why valgrind cannot write 0xcc into stack memory when it already
>> has to write there to create the stack frame / parameters passed by stack?
> Effectively, I have a patch which fixes the problem.
> But the patch is a kludge which heuristically guesses that GDB is
> pushing an infcall.

Why do you have to guess that, rather than just detecting a breakpoint is
being set on a stack (or non text) address?  If something sets a breakpoint
in a data address, it is basically telling valgrind "this is actually code".

-- 
Pedro Alves

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Wed, 2012-07-25 at 15:58 +0100, Pedro Alves wrote:
> On 07/23/2012 09:36 PM, Philippe Waroquiers wrote:
> 
> >> So the GDB patch is no longer needed when you have fixed valgrind to put 0xcc
> >> during Z0? Why valgrind cannot write 0xcc into stack memory when it already
> >> has to write there to create the stack frame / parameters passed by stack?
> > Effectively, I have a patch which fixes the problem.
> > But the patch is a kludge which heuristically guesses that GDB is
> > pushing an infcall.
> 
> Why do you have to guess that, rather than just detecting a breakpoint is
> being set on a stack (or non text) address?  If something sets a breakpoint
> in a data address, it is basically telling valgrind "this is actually code".

This is explained by the way Valgrind gdbsrv (must) implement
breakpoints.
(this is a little bit tricky, as it is linked to Valgrind internals).

Valgrind translates guest code instructions in small blocks.
As part of the translation, if there is a breakpoint at addr XXXX
then the translation of address XXXX will start with a call to a
helper function which reports to GDB that a breakpoint has been
encountered. This function then reads/executes protocol packets till a
continue packet is received.
The translated block is then continued <<< This is the critical info !!!

There is no way to re-translate the block currently being executed :
Valgrind has no way to "drop" the translated block it is currently
executing.
So, a breakpoint cannot be translated using a 0xCC because when GDB
tells to continue after the breakpoint, there is no way to retranslate
the original instructions (without the 0xCC) as long as the block
is being executed.
So, for normal breakpoints, Valgrind gdbsrv cannot insert 0xCC, as this
would just not work.

"Normal" breakpoints on the stack (trampoline code or whatever) or
JITted code or ... must be handled the same way: V gdbsrv cannot
touch the code to handle breakpoints.

The only special case in which Valgrind gdbsrv can insert a 0xCC is
when it is sure that this code will *not* be executed.
This is the case for the 0xcc for the push_dummy_code.
This code will not be executed because GDB will change the pc register.
When the continue packet is received, the execution of the block is
then not continued, instead the continue will cause a jump to the
"original pc" (the one before the infcall).

So, in summary:
   * for all normal breakpoints, Valgrind gdbsrv cannot insert a 0xcc
   * for "dummy inferior call return breakpoint", Valgrind gdbsrv can
     insert a 0xcc, as GDB will ensure this piece of code is not
     executed, and so there is no need to re-translate it without the
     0xcc

So, the kludge patch I have done tries to guess if the breakpoint
on the stack is a normal breakpoint (so, V gdbsrv cannot touch it)
or is a "infcall" breakpoint (then the kludge patch inserts a 0xcc).

It would be much cleaner to have GDB inserting the 0xCC, as GDB *knows*
it is doing a infcall, and so knows it can safely insert a 0xCC.
(Valgrind gdbsrv needs in any case the Z0 packet, as this is what
will ensure the 0xCC is not executed).

Not too sure if the above explanations are clear, it is somewhat tricky.

So, if it is easy to change GDB to insert 0xcc (for x86 and amd84)
and the equivalent breakpoint instr for mips32, then that avoids
the kludgy patch in Valgrind, which is for sure fragile.

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Joel Brobecker]

> The translated block is then continued <<< This is the critical info !!!

I am having trouble understanding why the translated block would
be continued in the case of an inferior function call, since
the code is not to be executed (thanks to resetting the PC to its
original value prior to the inferior function call when reaching
the Z0 breakpoint we inserted). But...

> So, if it is easy to change GDB to insert 0xcc (for x86 and amd84)
> and the equivalent breakpoint instr for mips32, then that avoids
> the kludgy patch in Valgrind, which is for sure fragile.

... if I still understand correctly, as long as we write a valid
instruction at the point where we place the infcall breakpoint,
we should be fine, right? Jan selected the hlt instruction, but
we could go with the breakpoint trap instruction as well (0xcc),
which I think should be simpler to generalize in the future.
It should not matter which instruction is chosen as long as it
fits the available space, since that instruction will never be
executed (thanks to the Z0 packet introducing an official breakpoint
there).

-- 
Joel

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Wed, 2012-07-25 at 14:26 -0700, Joel Brobecker wrote:
> > The translated block is then continued <<< This is the critical info !!!
> 
> I am having trouble understanding why the translated block would
> be continued in the case of an inferior function call, since
> the code is not to be executed (thanks to resetting the PC to its
> original value prior to the inferior function call when reaching
> the Z0 breakpoint we inserted). But...
The "continued" explanation above is only for normal breakpoints.

The "infcall breakpoints" are effectively *not* continued, thanks
to GDB changing the program counter. This is what allows the
kludgy Valgrind patch to work.

> 
> > So, if it is easy to change GDB to insert 0xcc (for x86 and amd84)
> > and the equivalent breakpoint instr for mips32, then that avoids
> > the kludgy patch in Valgrind, which is for sure fragile.
> 
> ... if I still understand correctly, as long as we write a valid
> instruction at the point where we place the infcall breakpoint,
> we should be fine, right? Jan selected the hlt instruction, but
> we could go with the breakpoint trap instruction as well (0xcc),
> which I think should be simpler to generalize in the future.
> It should not matter which instruction is chosen as long as it
> fits the available space, since that instruction will never be
> executed (thanks to the Z0 packet introducing an official breakpoint
> there).
A valid instruction is not enough. We need a valid instruction
that will cause Valgrind to terminate block translation.
The breakpoint trap instruction is ok for that.
(0xcc for x86 and amd64, 0x0005000d for mips32).


Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Joel Brobecker]

> A valid instruction is not enough. We need a valid instruction
> that will cause Valgrind to terminate block translation.
> The breakpoint trap instruction is ok for that.
> (0xcc for x86 and amd64, 0x0005000d for mips32).

I think it is fine to update GDB to insert the breakpoint instruction
instead of leaving random bytes at the breakpoint location. But it
sounds like this is forcing GDB to have insider knowledge of valgrind.
It would seem better if, in parallel to our efforts, something was
done on the valgrind side as well to make it work without the GDB
workaround. For instance, couldn't valgrind figure out that the
block translation should stop at a Z0 address if the instruction
underneath is illegal?

What I am trying to do, is make sure that new GDB versions work well
with older versions of valgrind (although, isn't gdbserver support
relatively recent?), while at the same time trying to make future
versions of valgrind more robust. I don't know how long we are going
to be able to keep the workaround. What if other tools implementing
the remote protocol had the same problem, and they required us to
insert a different instruction instead?

-- 
Joel

[patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Thu, 26 Jul 2012 00:39:33 +0200, Joel Brobecker wrote:
> > A valid instruction is not enough. We need a valid instruction
> > that will cause Valgrind to terminate block translation.
> > The breakpoint trap instruction is ok for that.
> > (0xcc for x86 and amd64, 0x0005000d for mips32).
> 
> I think it is fine to update GDB to insert the breakpoint instruction
> instead of leaving random bytes at the breakpoint location.

Attached.  I no longer find it useful as arch-dependent code, it would do the
same in each arch.

No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.

Philippe, do you have an easy enough way to regression test it on mips when
you was asking for the mips fix?  mips has many execution modes as I see.

I still have to write a testcase for it using valgrind.


> But it sounds like this is forcing GDB to have insider knowledge of
> valgrind.

As you were asking to put there 'int3' (and not 'hlt') the user convenience
coincidentally matches here with what Philippe asks for valgrind.


I am only concerned a bit about this change arcross all archs for 7.5.
Maybe 7.5 could limit this patch only for i386/x86_64 which is well
understood.


Thanks,
Jan


gdb/
2012-07-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infcall.c (call_function_by_hand): Move BP_ADDR comment to
	AT_ENTRY_POINT.
	(call_function_by_hand) <ON_STACK>: Call write_memory with
	gdbarch_breakpoint_from_pc, if possible.
	(call_function_by_hand) <AT_ENTRY_POINT>: The BP_ADDR comment is moved
	here.

gdb/doc/
2012-07-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdbint.texinfo (Defining Other Architecture Features): Clarify *pcptr
	encoding for gdbarch_breakpoint_from_pc, bp_addr for
	gdbarch_push_dummy_call and bp_addr for gdbarch_push_dummy_code.

diff --git a/gdb/doc/gdbint.texinfo b/gdb/doc/gdbint.texinfo
index 5e00f1f..b66f80b 100644
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -4540,8 +4540,10 @@ contents and size of a breakpoint instruction.  It returns a pointer to
 a static string of bytes that encode a breakpoint instruction, stores the
 length of the string to @code{*@var{lenptr}}, and adjusts the program
 counter (if necessary) to point to the actual memory location where the
-breakpoint should be inserted.  May return @code{NULL} to indicate that
-software breakpoints are not supported.
+breakpoint should be inserted.  The program counter (@code{*@var{pcptr}}
+is inferior PC register encoded on the input and it is a plain address on the
+output.  Function may return @code{NULL} to indicate that software breakpoints
+are not supported.
 
 Although it is common to use a trap instruction for a breakpoint, it's
 not required; for instance, the bit pattern could be an invalid
@@ -4821,7 +4823,7 @@ instead of value.
 @anchor{gdbarch_push_dummy_call} Define this to push the dummy frame's call to
 the inferior function onto the stack.  In addition to pushing @var{nargs}, the
 code should push @var{struct_addr} (when @var{struct_return} is non-zero), and
-the return address (@var{bp_addr}).
+the return address (@var{bp_addr}, in inferior PC register encoding).
 
 @var{function} is a pointer to a @code{struct value}; on architectures that use
 function descriptors, this contains the function descriptor value.
@@ -4835,12 +4837,14 @@ instruction sequence (including space for a breakpoint) to which the
 called function should return.
 
 Set @var{bp_addr} to the address at which the breakpoint instruction
-should be inserted, @var{real_pc} to the resume address when starting
-the call sequence, and return the updated inner-most stack address.
+should be inserted (in inferior PC register encoding), @var{real_pc} to the
+resume address when starting the call sequence, and return the updated
+inner-most stack address.
 
 By default, the stack is grown sufficient to hold a frame-aligned
 (@pxref{frame_align}) breakpoint, @var{bp_addr} is set to the address
-reserved for that breakpoint, and @var{real_pc} set to @var{funaddr}.
+reserved for that breakpoint (in inferior PC register encoding), and
+@var{real_pc} set to @var{funaddr}.
 
 This method replaces @w{@code{gdbarch_call_dummy_location (@var{gdbarch})}}.
 
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 51cd118..6ac6624 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -618,15 +618,37 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      not just the breakpoint but also an extra word containing the
      size (?) of the structure being passed.  */
 
-  /* The actual breakpoint (at BP_ADDR) is inserted separatly so there
-     is no need to write that out.  */
-
   switch (gdbarch_call_dummy_location (gdbarch))
     {
     case ON_STACK:
-      sp = push_dummy_code (gdbarch, sp, funaddr,
-				args, nargs, target_values_type,
-				&real_pc, &bp_addr, get_current_regcache ());
+      {
+	const gdb_byte *bp_bytes;
+	CORE_ADDR bp_addr_as_address;
+	int bp_size;
+
+	/* Be careful BP_ADDR is in inferior PC encoding while
+	   BP_ADDR_AS_ADDRESS is a plain memory address.  */
+
+	sp = push_dummy_code (gdbarch, sp, funaddr, args, nargs,
+			      target_values_type, &real_pc, &bp_addr,
+			      get_current_regcache ());
+
+	/* Write a legitimate instruction at the point where the infcall
+	   breakpoint is going to be inserted.  While this instruction
+	   is never going to be executed, a user investigating the
+	   memory from GDB would see this instruction instead of random
+	   uninitialized bytes.  We chose the breakpoint instruction
+	   just because it may look as the most logical one to the user.
+
+	   If software breakpoints are unsupported for this target we
+	   leave the user visible memory content uninitialized.  */
+
+	bp_addr_as_address = bp_addr;
+	bp_bytes = gdbarch_breakpoint_from_pc (gdbarch, &bp_addr_as_address,
+					       &bp_size);
+	if (bp_bytes != NULL)
+	  write_memory (bp_addr_as_address, bp_bytes, bp_size);
+      }
       break;
     case AT_ENTRY_POINT:
       {
@@ -634,8 +656,12 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 
 	real_pc = funaddr;
 	dummy_addr = entry_point_address ();
+
 	/* A call dummy always consists of just a single breakpoint, so
-	   its address is the same as the address of the dummy.  */
+	   its address is the same as the address of the dummy.
+
+	   The actual breakpoint is inserted separatly so there is no need to
+	   write that out.  */
 	bp_addr = dummy_addr;
 	break;
       }

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Philippe Waroquiers]

On Thu, 2012-07-26 at 23:23 +0200, Jan Kratochvil wrote:
> On Thu, 26 Jul 2012 00:39:33 +0200, Joel Brobecker wrote:
> > > A valid instruction is not enough. We need a valid instruction
> > > that will cause Valgrind to terminate block translation.
> > > The breakpoint trap instruction is ok for that.
> > > (0xcc for x86 and amd64, 0x0005000d for mips32).
> > 
> > I think it is fine to update GDB to insert the breakpoint instruction
> > instead of leaving random bytes at the breakpoint location.
> 
> Attached.  I no longer find it useful as arch-dependent code, it would do the
> same in each arch.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.
> 
> Philippe, do you have an easy enough way to regression test it on mips when
> you was asking for the mips fix?  mips has many execution modes as I see.
Thanks for the patch.
For mips, I have an access to gcc compile farm (gcc49).
So, I can check it works on at least the execution mode of this machine.

> 
> I still have to write a testcase for it using valgrind.
IIUC, this implies to write a new 'gdbserver board file' (or something
like that) which will allow to access the Valgrind gdbsrv.

Note that this is covered by the Valgrind regression tests
(that is how the change of behaviour with 7.4.91 was detected).

Philippe

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Thu, 26 Jul 2012 23:49:58 +0200, Philippe Waroquiers wrote:
> On Thu, 2012-07-26 at 23:23 +0200, Jan Kratochvil wrote:
> > I still have to write a testcase for it using valgrind.
> 
> IIUC, this implies to write a new 'gdbserver board file' (or something
> like that) which will allow to access the Valgrind gdbsrv.

There is valgrind board file (without gdbsrv).  But I have found running the
testsuite nightly with valgrind is not feasible, it is too costly resulting in
various testsuite issues.

I have added just regular testfile like there was already:
	gdb.base/valgrind-db-attach.exp 


> Note that this is covered by the Valgrind regression tests
> (that is how the change of behaviour with 7.4.91 was detected).

Somehow I was not notified soon enough in Fedora Rawhide, probably because
Fedora valgrind maintenance is now affected by the dwz incompatibility.



On Fri, 27 Jul 2012 17:20:22 +0200, Pedro Alves wrote:
> FWIW, the patch looks good to me, and I don't think that
> limiting to x86 is necessary:

OK, thanks for review.  I will yet wait for the Maciej's MIPS test, not sure
what is the next 7.5 snapshot release.


> IMO, this comment should also mention that in addition to being nice for
> the user, this is actually _necessary_ for at least Valgrind v XXX.YYY.

Done.


Thanks,
Jan


gdb/
2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

        * infcall.c (call_function_by_hand): Move BP_ADDR comment to
        AT_ENTRY_POINT.
        (call_function_by_hand) <ON_STACK>: Call write_memory with
        gdbarch_breakpoint_from_pc, if possible.
        (call_function_by_hand) <AT_ENTRY_POINT>: The BP_ADDR comment is moved
        here.

gdb/doc/
2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

        * gdbint.texinfo (Defining Other Architecture Features): Clarify *pcptr
        encoding for gdbarch_breakpoint_from_pc, bp_addr for
        gdbarch_push_dummy_call and bp_addr for gdbarch_push_dummy_code.

gdb/testsuite/
2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/valgrind-db-attach.exp: Do not run in remote mode.
	* gdb.base/valgrind-infcall.c: New file.
	* gdb.base/valgrind-infcall.exp: New file.

diff --git a/gdb/doc/gdbint.texinfo b/gdb/doc/gdbint.texinfo
index 5e00f1f..b66f80b 100644
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -4540,8 +4540,10 @@ contents and size of a breakpoint instruction.  It returns a pointer to
 a static string of bytes that encode a breakpoint instruction, stores the
 length of the string to @code{*@var{lenptr}}, and adjusts the program
 counter (if necessary) to point to the actual memory location where the
-breakpoint should be inserted.  May return @code{NULL} to indicate that
-software breakpoints are not supported.
+breakpoint should be inserted.  The program counter (@code{*@var{pcptr}}
+is inferior PC register encoded on the input and it is a plain address on the
+output.  Function may return @code{NULL} to indicate that software breakpoints
+are not supported.
 
 Although it is common to use a trap instruction for a breakpoint, it's
 not required; for instance, the bit pattern could be an invalid
@@ -4821,7 +4823,7 @@ instead of value.
 @anchor{gdbarch_push_dummy_call} Define this to push the dummy frame's call to
 the inferior function onto the stack.  In addition to pushing @var{nargs}, the
 code should push @var{struct_addr} (when @var{struct_return} is non-zero), and
-the return address (@var{bp_addr}).
+the return address (@var{bp_addr}, in inferior PC register encoding).
 
 @var{function} is a pointer to a @code{struct value}; on architectures that use
 function descriptors, this contains the function descriptor value.
@@ -4835,12 +4837,14 @@ instruction sequence (including space for a breakpoint) to which the
 called function should return.
 
 Set @var{bp_addr} to the address at which the breakpoint instruction
-should be inserted, @var{real_pc} to the resume address when starting
-the call sequence, and return the updated inner-most stack address.
+should be inserted (in inferior PC register encoding), @var{real_pc} to the
+resume address when starting the call sequence, and return the updated
+inner-most stack address.
 
 By default, the stack is grown sufficient to hold a frame-aligned
 (@pxref{frame_align}) breakpoint, @var{bp_addr} is set to the address
-reserved for that breakpoint, and @var{real_pc} set to @var{funaddr}.
+reserved for that breakpoint (in inferior PC register encoding), and
+@var{real_pc} set to @var{funaddr}.
 
 This method replaces @w{@code{gdbarch_call_dummy_location (@var{gdbarch})}}.
 
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 51cd118..1b2c3d6 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -618,15 +618,38 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      not just the breakpoint but also an extra word containing the
      size (?) of the structure being passed.  */
 
-  /* The actual breakpoint (at BP_ADDR) is inserted separatly so there
-     is no need to write that out.  */
-
   switch (gdbarch_call_dummy_location (gdbarch))
     {
     case ON_STACK:
-      sp = push_dummy_code (gdbarch, sp, funaddr,
-				args, nargs, target_values_type,
-				&real_pc, &bp_addr, get_current_regcache ());
+      {
+	const gdb_byte *bp_bytes;
+	CORE_ADDR bp_addr_as_address;
+	int bp_size;
+
+	/* Be careful BP_ADDR is in inferior PC encoding while
+	   BP_ADDR_AS_ADDRESS is a plain memory address.  */
+
+	sp = push_dummy_code (gdbarch, sp, funaddr, args, nargs,
+			      target_values_type, &real_pc, &bp_addr,
+			      get_current_regcache ());
+
+	/* Write a legitimate instruction at the point where the infcall
+	   breakpoint is going to be inserted.  While this instruction
+	   is never going to be executed, a user investigating the
+	   memory from GDB would see this instruction instead of random
+	   uninitialized bytes.  We chose the breakpoint instruction
+	   as it may look as the most logical one to the user and also
+	   valgrind 3.7.0 needs it for proper vgdb inferior calls.
+
+	   If software breakpoints are unsupported for this target we
+	   leave the user visible memory content uninitialized.  */
+
+	bp_addr_as_address = bp_addr;
+	bp_bytes = gdbarch_breakpoint_from_pc (gdbarch, &bp_addr_as_address,
+					       &bp_size);
+	if (bp_bytes != NULL)
+	  write_memory (bp_addr_as_address, bp_bytes, bp_size);
+      }
       break;
     case AT_ENTRY_POINT:
       {
@@ -634,8 +657,12 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 
 	real_pc = funaddr;
 	dummy_addr = entry_point_address ();
+
 	/* A call dummy always consists of just a single breakpoint, so
-	   its address is the same as the address of the dummy.  */
+	   its address is the same as the address of the dummy.
+
+	   The actual breakpoint is inserted separatly so there is no need to
+	   write that out.  */
 	bp_addr = dummy_addr;
 	break;
       }
diff --git a/gdb/testsuite/gdb.base/valgrind-db-attach.exp b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
index b14401f..313e4e0 100644
--- a/gdb/testsuite/gdb.base/valgrind-db-attach.exp
+++ b/gdb/testsuite/gdb.base/valgrind-db-attach.exp
@@ -13,6 +13,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+if [is_remote target] {
+    # The test always runs locally.
+    return 0
+}
+
 set test valgrind-db-attach
 set srcfile $test.c
 set executable $test
diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.c b/gdb/testsuite/gdb.base/valgrind-infcall.c
new file mode 100644
index 0000000..c119b7e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.c
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+static volatile int infcall_var;
+
+static int
+gdb_test_infcall (void)
+{
+  return ++infcall_var;
+}
+
+int
+main (void)
+{
+  void *p;
+
+  gdb_test_infcall ();
+  p = malloc (1);
+  if (p == NULL)
+    return 1;
+  free (p);
+  free (p);	/* double-free */
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.exp b/gdb/testsuite/gdb.base/valgrind-infcall.exp
new file mode 100644
index 0000000..ede26f4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
@@ -0,0 +1,115 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if [is_remote target] {
+    # The test always runs locally.
+    return 0
+}
+
+set test valgrind-infcall
+set srcfile $test.c
+set executable $test
+set binfile ${objdir}/${subdir}/${executable}
+if {[build_executable $test.exp $executable $srcfile {debug}] == -1} {
+    return -1
+}
+
+set test "spawn valgrind"
+set cmd "valgrind --vgdb-error=0 $binfile"
+set res [remote_spawn host $cmd];
+if { $res < 0 || $res == "" } {
+    verbose -log "Spawning $cmd failed."
+    unsupported $test
+    return -1
+}
+pass $test
+# Declare GDB now as running.
+set gdb_spawn_id -1
+
+# GDB started by vgdb stops already after the startup is executed, like with
+# non-extended gdbserver.  It is also not correct to run/attach the inferior.
+set use_gdb_stub 1
+
+set test "valgrind started"
+# The trailing '.' differs for different memcheck versions.
+gdb_test_multiple "" $test {
+    -re "Memcheck, a memory error detector\\.?\r\n" {
+	pass $test
+    }
+    -re "valgrind: failed to start tool 'memcheck' for platform '.*': No such file or directory" {
+	unsupported $test
+	return -1
+    }
+    -re "valgrind: wrong ELF executable class" {
+	unsupported $test
+	return -1
+    }
+    -re "command not found" {
+	# The spawn succeeded, but then valgrind was not found - e.g. if
+	# we spawned SSH to a remote system.
+	unsupported $test
+	return -1
+    }
+    -re "valgrind: Bad option '--vgdb-error=0'" {
+	# valgrind is not >= 3.7.0.
+	unsupported $test
+	return -1
+    }
+}
+
+set test "vgdb prompt"
+# The trailing '.' differs for different memcheck versions.
+gdb_test_multiple "" $test {
+    -re "  (target remote | \[^\r\n\]*/vgdb \[^\r\n\]*)\r\n" {
+	set vgdbcmd $expect_out(1,string)
+	pass $test
+    }
+}
+
+# Do not kill valgrind.
+unset gdb_spawn_id
+set board [host_info name]
+unset_board_info fileid
+
+clean_restart $executable
+
+gdb_test "$vgdbcmd" " in _start .*" "target remote for vgdb"
+
+gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
+
+set continue_count 1
+while 1 {
+    set test "continue #$continue_count"
+    gdb_test_multiple "continue" "" {
+	-re "Invalid free\\(\\).*: main .*\r\n$gdb_prompt $" {
+	    pass $test
+	    break
+	}
+	-re "\r\n$gdb_prompt $" {
+	    pass "$test (false warning)"
+	}
+    }
+    set continue_count [expr $continue_count + 1]
+}
+
+set test "p gdb_test_infcall ()"
+gdb_test_multiple $test $test {
+    -re "unhandled instruction bytes.*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "Continuing \\.\\.\\..*\r\n\\\$1 = 2\r\n$gdb_prompt $" {
+	pass $test
+    }
+}

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Eli Zaretskii]

> Date: Fri, 27 Jul 2012 20:46:33 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org,        "Maciej W. Rozycki" <macro@codesourcery.com>
> 
> -breakpoint should be inserted.  May return @code{NULL} to indicate that
> -software breakpoints are not supported.
> +breakpoint should be inserted.  The program counter (@code{*@var{pcptr}}
> +is inferior PC register encoded on the input and it is a plain address on the
> +output.  Function may return @code{NULL} to indicate that software breakpoints
> +are not supported.

I suggest a slight rewording:

 On input, the program counter (@code{*@var{pcptr}} is the encoded
 inferior's PC register.  The function returns the PC's plain address
 in this argument, or @code{NULL} if software breakpoints are not
 supported.

Does this catch the intent correctly?

> -the return address (@var{bp_addr}).
> +the return address (@var{bp_addr}, in inferior PC register encoding).
                                         ^^^^^^^^
"inferior's"

> -should be inserted, @var{real_pc} to the resume address when starting
> -the call sequence, and return the updated inner-most stack address.
> +should be inserted (in inferior PC register encoding), @var{real_pc} to the
                          ^^^^^^^^
Likewise.

> -reserved for that breakpoint, and @var{real_pc} set to @var{funaddr}.
> +reserved for that breakpoint (in inferior PC register encoding), and
                                    ^^^^^^^^
Likewise.

OK with these changes.

Thanks.

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Sat, 28 Jul 2012 09:28:02 +0200, Eli Zaretskii wrote:
> > Date: Fri, 27 Jul 2012 20:46:33 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > Cc: Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org,        "Maciej W. Rozycki" <macro@codesourcery.com>
> > 
> > -breakpoint should be inserted.  May return @code{NULL} to indicate that
> > -software breakpoints are not supported.
> > +breakpoint should be inserted.  The program counter (@code{*@var{pcptr}}
> > +is inferior PC register encoded on the input and it is a plain address on the
> > +output.  Function may return @code{NULL} to indicate that software breakpoints
> > +are not supported.
> 
> I suggest a slight rewording:
> 
>  On input, the program counter (@code{*@var{pcptr}} is the encoded
>  inferior's PC register.  The function returns the PC's plain address
>  in this argument, or @code{NULL} if software breakpoints are not
>  supported.
> 
> Does this catch the intent correctly?

I do not think that 'pcptr' and 'NULL' should be combined in one sentence,
these are two topics.

BTW I did not change any function behavior, I just try to better document
behavior of the existing functions.

One issue is that:
	Function may return @code{NULL} to indicate that software breakpoints
	are not supported.
This is returned as C function return value.  If NULL is returned other
parameters are irrelevant/unspecified.  And NULL is definitely unrelated to
pcptr.

The other issue, I try to better document the meaning of '*pcptr' input vs.
output value.  This is a value passed by reference.  Also it is not really the
PC register, it is just arbitrary location used for a breakpoint (therefore
pointing at code instructions).

I tried to reword it again and I ended up similar to my former proposal:
	On input, the program counter (@code{*@var{pcptr}} is encoded like the
	inferior's PC register, on output is is encoded as a plain address.
	Function may return @code{NULL} to indicate that software breakpoints
	are not supported.


Thanks,
Jan

[commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 27 Jul 2012 20:46:33 +0200, Jan Kratochvil wrote:
> gdb/
> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         * infcall.c (call_function_by_hand): Move BP_ADDR comment to
>         AT_ENTRY_POINT.
>         (call_function_by_hand) <ON_STACK>: Call write_memory with
>         gdbarch_breakpoint_from_pc, if possible.
>         (call_function_by_hand) <AT_ENTRY_POINT>: The BP_ADDR comment is moved
>         here.
> 
> gdb/testsuite/
> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/valgrind-infcall.c: New file.
> 	* gdb.base/valgrind-infcall.exp: New file.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2012-07/msg00256.html
and for 7.5:
	http://sourceware.org/ml/gdb-cvs/2012-07/msg00257.html

without these in fact unrelated parts:

> gdb/doc/
> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         * gdbint.texinfo (Defining Other Architecture Features): Clarify *pcptr
>         encoding for gdbarch_breakpoint_from_pc, bp_addr for
>         gdbarch_push_dummy_call and bp_addr for gdbarch_push_dummy_code.

> 	* gdb.base/valgrind-db-attach.exp: Do not run in remote mode.


Thanks,
Jan

[commit#2+7.5] testsuite: valgrind-infcall.exp UNSUPPORTED update [Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address]

[Jan Kratochvil]

On Tue, 31 Jul 2012 09:36:43 +0200, Jan Kratochvil wrote:
> On Fri, 27 Jul 2012 20:46:33 +0200, Jan Kratochvil wrote:
> > 	* gdb.base/valgrind-infcall.c: New file.
> > 	* gdb.base/valgrind-infcall.exp: New file.

Also checked in:
	http://sourceware.org/ml/gdb-cvs/2012-08/msg00006.html
and for 7.5:
	http://sourceware.org/ml/gdb-cvs/2012-08/msg00007.html

as with valgrind-3.6.0 on RHEL-6 the error message is slightly different
causing:
	Running gdb/testsuite/gdb.base/valgrind-infcall.exp ...
	PASS: gdb.base/valgrind-infcall.exp: spawn valgrind
	ERROR: Process no longer exists
	UNRESOLVED: gdb.base/valgrind-infcall.exp: valgrind started


Jan


http://sourceware.org/ml/gdb-cvs/2012-08/msg00006.html

--- src/gdb/testsuite/ChangeLog	2012/07/31 07:35:17	1.3314
+++ src/gdb/testsuite/ChangeLog	2012/08/01 09:02:50	1.3315
@@ -1,3 +1,8 @@
+2012-08-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/valgrind-infcall.exp: Relax the UNSUPPORTED check for more
+	valgrind versions.
+
 2012-07-31  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.base/valgrind-infcall.c: New file.
--- src/gdb/testsuite/gdb.base/valgrind-infcall.exp	2012/07/31 07:33:16	1.1
+++ src/gdb/testsuite/gdb.base/valgrind-infcall.exp	2012/08/01 09:02:50	1.2
@@ -62,7 +62,7 @@
 	unsupported $test
 	return -1
     }
-    -re "valgrind: Bad option '--vgdb-error=0'" {
+    -re "valgrind: Bad option.*--vgdb-error=0" {
 	# valgrind is not >= 3.7.0.
 	unsupported $test
 	return -1

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 07/31/2012 04:36 AM, Jan Kratochvil wrote:
>> gdb/testsuite/
>> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>
>> 	* gdb.base/valgrind-infcall.c: New file.
>> 	* gdb.base/valgrind-infcall.exp: New file.

I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:

...
(gdb) PASS: gdb.base/valgrind-infcall.exp: continue #1 (false warning)
continue
Continuing.
==7541== Invalid free() / delete / delete[] / realloc()
==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
==7541==    by 0x10000763: main (valgrind-infcall.c:38)
==7541==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
==7541==    by 0x10000757: main (valgrind-infcall.c:37)
==7541== 
==7541== (action on error) vgdb me ... 

Program received signal SIGTRAP, Trace/breakpoint trap.
../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
A problem internal to GDB has been detected,
FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
further debugging may prove unreliable.
Quit this debugging session? (y or n) n

With this error, gdb connection is closed and the testsuite gets stuck at this point.

-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Sergio Durigan Junior]

On Thursday, August 02 2012, Edjunior Barbosa Machado wrote:

> On 07/31/2012 04:36 AM, Jan Kratochvil wrote:
>>> gdb/testsuite/
>>> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>>
>>> 	* gdb.base/valgrind-infcall.c: New file.
>>> 	* gdb.base/valgrind-infcall.exp: New file.
>
> I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:
>
> ...
> (gdb) PASS: gdb.base/valgrind-infcall.exp: continue #1 (false warning)
> continue
> Continuing.
> ==7541== Invalid free() / delete / delete[] / realloc()
> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
> ==7541==    by 0x10000763: main (valgrind-infcall.c:38)
> ==7541==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
> ==7541==    by 0x10000757: main (valgrind-infcall.c:37)
> ==7541== 
> ==7541== (action on error) vgdb me ... 
>
> Program received signal SIGTRAP, Trace/breakpoint trap.
> ../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
> A problem internal to GDB has been detected,
> FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) n
>
> With this error, gdb connection is closed and the testsuite gets stuck
> at this point.

(Adding Tom to CC list).

Thanks for the report.

Just as an FYI (or For Our Information, rather),
http://sourceware.org/ml/gdb-patches/2012-08/msg00075.html clearly fixes
the bug.  I will keep an eye on this since I am interested as well.

Thanks,

-- 
Sergio

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/02/2012 08:09 PM, Sergio Durigan Junior wrote:
> On Thursday, August 02 2012, Edjunior Barbosa Machado wrote:
> 
>> On 07/31/2012 04:36 AM, Jan Kratochvil wrote:
>>>> gdb/testsuite/
>>>> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>>>
>>>> 	* gdb.base/valgrind-infcall.c: New file.
>>>> 	* gdb.base/valgrind-infcall.exp: New file.
>>
>> I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:
>>
>> ...
>> (gdb) PASS: gdb.base/valgrind-infcall.exp: continue #1 (false warning)
>> continue
>> Continuing.
>> ==7541== Invalid free() / delete / delete[] / realloc()
>> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
>> ==7541==    by 0x10000763: main (valgrind-infcall.c:38)
>> ==7541==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
>> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
>> ==7541==    by 0x10000757: main (valgrind-infcall.c:37)
>> ==7541== 
>> ==7541== (action on error) vgdb me ... 
>>
>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> ../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
>> A problem internal to GDB has been detected,
>> FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
>> further debugging may prove unreliable.
>> Quit this debugging session? (y or n) n
>>
>> With this error, gdb connection is closed and the testsuite gets stuck
>> at this point.
> 
> (Adding Tom to CC list).
> 
> Thanks for the report.
> 
> Just as an FYI (or For Our Information, rather),
> http://sourceware.org/ml/gdb-patches/2012-08/msg00075.html clearly fixes
> the bug.  I will keep an eye on this since I am interested as well.
> 
> Thanks,
> 

I've run the full testsuite with this patch and it indeed eliminates the internal-error on gdb.base/valgrind-infcall.exp, but now there are new failures on gdb.threads/watchpoint-fork.exp (the first and the third were already failing with the same internal-error from valgrind-infcall.exp):

...
  FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the first fork       
  FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the first fork       
  FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the second fork      
  FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the second fork      
  FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: finish
...

Here's the detailed output:

...
continue
Continuing.
[New process 28316]
parent1: 28316
Error in re-setting breakpoint 1: reading register r31 (#31): No such process.
Error in re-setting breakpoint -1: reading register r31 (#31): No such process.
Error in re-setting breakpoint 2: reading register r31 (#31): No such process.
Error in re-setting breakpoint 3: reading register r31 (#31): No such process.
Error in re-setting breakpoint 4: reading register r31 (#31): No such process.
reading register r31 (#31): No such process.
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the first fork
continue
Continuing.
reading register r31 (#31): No such process.
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the first fork
continue
Continuing.
reading register r31 (#31): No such process.
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the second fork
continue
Continuing.
reading register r31 (#31): No such process.
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the second fork
continue
Continuing.
reading register r31 (#31): No such process.
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: finish

-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 03 Aug 2012 02:14:34 +0200, Edjunior Barbosa Machado wrote:
>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the first fork       
>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the first fork       
>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the second fork      
>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the second fork      
>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: finish

most probably it is due to ppc-linux-nat.c and s390-nat.c changes still left
on archer-jankratochvil-watchpoint3 and not yet upstreamed with the x86* post
	[patch 1/2] Fix watchpoints across fork #2
	http://sourceware.org/ml/gdb-patches/2012-01/msg00748.html

It is not a regression, I think it is OK for 7.6 (+Red Hat branches can get
a backport for 7.5).


Sorry,
Jan

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/03/2012 08:22 AM, Jan Kratochvil wrote:
> On Fri, 03 Aug 2012 02:14:34 +0200, Edjunior Barbosa Machado wrote:
>>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the first fork       
>>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the first fork       
>>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the second fork      
>>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the second fork      
>>   FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: finish
> 
> most probably it is due to ppc-linux-nat.c and s390-nat.c changes still left
> on archer-jankratochvil-watchpoint3 and not yet upstreamed with the x86* post
> 	[patch 1/2] Fix watchpoints across fork #2
> 	http://sourceware.org/ml/gdb-patches/2012-01/msg00748.html
> 
> It is not a regression, I think it is OK for 7.6 (+Red Hat branches can get
> a backport for 7.5).
> 
> 
> Sorry,
> Jan
> 

Thanks Jan for pointing this out, but actually, this looks a bit different without the patch fixing PR 14100:

FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the first fork (GDB internal error)
PASS: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the first fork
FAIL: gdb.threads/watchpoint-fork.exp: child: singlethreaded: breakpoint after the second fork (GDB internal error)
PASS: gdb.threads/watchpoint-fork.exp: child: singlethreaded: watchpoint after the second fork
PASS: gdb.threads/watchpoint-fork.exp: child: singlethreaded: finish

-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Pedro Alves]

On 08/03/2012 12:09 AM, Sergio Durigan Junior wrote:
> On Thursday, August 02 2012, Edjunior Barbosa Machado wrote:
> 
>> On 07/31/2012 04:36 AM, Jan Kratochvil wrote:
>>>> gdb/testsuite/
>>>> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>>>
>>>> 	* gdb.base/valgrind-infcall.c: New file.
>>>> 	* gdb.base/valgrind-infcall.exp: New file.
>>
>> I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:
>>
>> ...
>> (gdb) PASS: gdb.base/valgrind-infcall.exp: continue #1 (false warning)
>> continue
>> Continuing.
>> ==7541== Invalid free() / delete / delete[] / realloc()
>> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
>> ==7541==    by 0x10000763: main (valgrind-infcall.c:38)
>> ==7541==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
>> ==7541==    at 0x40458BC: free (vg_replace_malloc.c:427)
>> ==7541==    by 0x10000757: main (valgrind-infcall.c:37)
>> ==7541== 
>> ==7541== (action on error) vgdb me ... 
>>
>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> ../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
>> A problem internal to GDB has been detected,
>> FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
>> further debugging may prove unreliable.
>> Quit this debugging session? (y or n) n
>>
>> With this error, gdb connection is closed and the testsuite gets stuck
>> at this point.
> 
> (Adding Tom to CC list).
> 
> Thanks for the report.
> 
> Just as an FYI (or For Our Information, rather),
> http://sourceware.org/ml/gdb-patches/2012-08/msg00075.html clearly fixes
> the bug.  I will keep an eye on this since I am interested as well.

Curious.  But is the cause the same?  If something sending C-c to gdb at the "wrong" time?

-- 
Pedro Alves

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Sergio Durigan Junior]

On Thursday, August 02 2012, Pedro Alves wrote:

>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>> ../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
>>> A problem internal to GDB has been detected,
>>> FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
>>> further debugging may prove unreliable.
>>> Quit this debugging session? (y or n) n
>>>
>>> With this error, gdb connection is closed and the testsuite gets stuck
>>> at this point.
>> 
>> (Adding Tom to CC list).
>> 
>> Thanks for the report.
>> 
>> Just as an FYI (or For Our Information, rather),
>> http://sourceware.org/ml/gdb-patches/2012-08/msg00075.html clearly fixes
>> the bug.  I will keep an eye on this since I am interested as well.
>
> Curious.  But is the cause the same?  If something sending C-c to gdb at the "wrong" time?

Not sure, but apparently not.  I just wanted to make the relation
between the patch and the problem clear.  It is also worth noting that
the patch fixes only the internal error, but some failures still occur,
which is something else to be investigated.

-- 
Sergio

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/02/2012 10:47 PM, Sergio Durigan Junior wrote:
> On Thursday, August 02 2012, Pedro Alves wrote:
> 
>>>> Program received signal SIGTRAP, Trace/breakpoint trap.
>>>> ../../gdb.git/gdb/frame.c:2396: internal-error: frame_cleanup_after_sniffer: Assertion `frame->prologue_cache == NULL' failed.
>>>> A problem internal to GDB has been detected,
>>>> FAIL: gdb.base/valgrind-infcall.exp: continue (GDB internal error)
>>>> further debugging may prove unreliable.
>>>> Quit this debugging session? (y or n) n
>>>>
>>>> With this error, gdb connection is closed and the testsuite gets stuck
>>>> at this point.
>>>
>>> (Adding Tom to CC list).
>>>
>>> Thanks for the report.
>>>
>>> Just as an FYI (or For Our Information, rather),
>>> http://sourceware.org/ml/gdb-patches/2012-08/msg00075.html clearly fixes
>>> the bug.  I will keep an eye on this since I am interested as well.
>>
>> Curious.  But is the cause the same?  If something sending C-c to gdb at the "wrong" time?
> 
> Not sure, but apparently not.  I just wanted to make the relation
> between the patch and the problem clear.  It is also worth noting that
> the patch fixes only the internal error, but some failures still occur,
> which is something else to be investigated.
> 

Indeed, there is still a couple of unexpected failures in gdb.base/valgrind-infcall.exp on ppc64 even with the mentioned patch. One of the failures actually is just the missing `.' prefixing _start() which should be acceptable by the testcase on ppc64 (patch below), but the other seems to be a real problem:

...
continue
Continuing.
==11976== Invalid free() / delete / delete[] / realloc()
==11976==    at 0x40458BC: free (vg_replace_malloc.c:427)
==11976==    by 0x10000763: main (valgrind-infcall.c:38)
==11976==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
==11976==    at 0x40458BC: free (vg_replace_malloc.c:427)
==11976==    by 0x10000757: main (valgrind-infcall.c:37)
==11976== 
==11976== (action on error) vgdb me ...  

Program received signal SIGTRAP, Trace/breakpoint trap.
(gdb) PASS: gdb.base/valgrind-infcall.exp: continue #2
p gdb_test_infcall ()
Remote 'g' packet reply is too long: 000000003807eb6800000007ff00d4400000000004068818000000000000000000000007ff00d4b000000007ff00d9f800000007ff00dfb800000000000000000000000000001102000000000000000000000007ff00d4b00000008095169ae0000000002400004200000080950ad5600000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000809509fdf800000080950a19f00000000004060940000000000407004000000007ff00d44000000000040700400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000!
 0000000000
0000000000000000000000000000000000000000000000000000000000000000000000040458bc000000000000000024000044000000003807eb680000008095169ae0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000!
 0000000000
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
(gdb) FAIL: gdb.base/valgrind-infcall.exp: p gdb_test_infcall ()
testcase ../../../gdb.git/gdb/testsuite/gdb.base/valgrind-infcall.exp completed in 1 seconds

-- 
Edjunior

diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.exp b/gdb/testsuite/gdb.base/valgrind-infcall.exp
index f2aa1ad..df2fb5c 100644
--- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
@@ -85,7 +85,7 @@ unset_board_info fileid
 
 clean_restart $executable
 
-gdb_test "$vgdbcmd" " in _start .*" "target remote for vgdb"
+gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
 
 gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
 

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Philippe Waroquiers]

On Thu, 2012-08-02 at 23:29 -0300, Edjunior Barbosa Machado wrote:
> ==11976== Invalid free() / delete / delete[] / realloc()
> ==11976==    at 0x40458BC: free (vg_replace_malloc.c:427)
> ==11976==    by 0x10000763: main (valgrind-infcall.c:38)
> ==11976==  Address 0x4070040 is 0 bytes inside a block of size 1 free'd
> ==11976==    at 0x40458BC: free (vg_replace_malloc.c:427)
> ==11976==    by 0x10000757: main (valgrind-infcall.c:37)
> ==11976== 
> ==11976== (action on error) vgdb me ...  
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> (gdb) PASS: gdb.base/valgrind-infcall.exp: continue #2
> p gdb_test_infcall ()
> Remote 'g' packet reply is too long: 
> 000000003807eb6800000007ff00d4400000000004068818000000000000000000000007ff00d4b000000007ff00d9f800000007ff00dfb800000000000000000000000000001102000000000000000000000007ff00d4b00000008095169ae0000000002400004200000080950ad5600000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000809509fdf800000080950a19f00000000004060940000000000407004000000007ff00d44000000000040700400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000!
> 0000000000
> 0000000000000000000000000000000000000000000000000000000000000000000000040458bc000000000000000024000044000000003807eb680000008095169ae0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000!
> 0000000000
> > 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
>(gdb) FAIL: gdb.base/valgrind-infcall.exp: p gdb_test_infcall ()
> testcase ../../../gdb.git/gdb/testsuite/gdb.base/valgrind-infcall.exp > completed in 1 seconds

Are you sure the above is a ppc64 executable and not a ppc32 executable ?

If that is a ppc32, the problem above might be due to a bug in Valgrind 3.7.0 ppc32 :
some xml files were missing in the install. So, GDB does not get the register description
for the Valgrind ppc32 simulated CPU. This might then cause the above 'packet too long'.
Testing in 64 bits and/or testing with the (not yet released) Valgrind 3.8.0 SVN should
be ok then. 

If this is not the explanation, then some more info might help to understand
what goes wrong (e.g. full log of Valgrind -v -v -v -d -d -d when doing the above test).


Thanks

Philippe

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Curious.  But is the cause the same?  If something sending C-c to gdb
Pedro> at the "wrong" time?

It could be any exception thrown at the wrong spot in any sniffer.
It would be interesting to know exactly where the original exception is
thrown.  I am still not sure that my fix is the correct one.  Maybe it
really is better to fix the sniffers.  Or maybe do both.

Tom

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 03 Aug 2012 16:22:47 +0200, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Curious.  But is the cause the same?  If something sending C-c to gdb
> Pedro> at the "wrong" time?
> 
> It could be any exception thrown at the wrong spot in any sniffer.
> It would be interesting to know exactly where the original exception is
> thrown.  I am still not sure that my fix is the correct one.  Maybe it
> really is better to fix the sniffers.  Or maybe do both.

Yes, IMO it would be better.  During the sniffers entryval virtual tailcall
frames path evaluation gets in effect which heavily throws exceptions and
successfully TRY_CATCHes them.  I am not aware of any problem there, just the
exceptions are common nowadays due to it during unwinding.


Thanks,
Jan

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/03/2012 11:30 AM, Jan Kratochvil wrote:
> On Fri, 03 Aug 2012 16:22:47 +0200, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> Curious.  But is the cause the same?  If something sending C-c to gdb
>> Pedro> at the "wrong" time?
>>
>> It could be any exception thrown at the wrong spot in any sniffer.
>> It would be interesting to know exactly where the original exception is
>> thrown.  I am still not sure that my fix is the correct one.  Maybe it
>> really is better to fix the sniffers.  Or maybe do both.
> 
> Yes, IMO it would be better.  During the sniffers entryval virtual tailcall
> frames path evaluation gets in effect which heavily throws exceptions and
> successfully TRY_CATCHes them.  I am not aware of any problem there, just the
> exceptions are common nowadays due to it during unwinding.
> 
> 
> Thanks,
> Jan
> 

Hi,
the patch below aborts the testcase if vgdb remote connection is closed, avoiding the testsuite to get stuck in the `continue' loop that happens when occurs the internal-error. It also considers the leading `.' for _start(), which is expected in ppc64.

Ok?

Thanks,
-- 
Edjunior

gdb/testsuite/
2012-08-03  Edjunior Machado  <emachado@linux.vnet.ibm.com>

	* gdb.base/valgrind-infcall.exp: Expect leading `.' on ppc64's
	symbols. Abort if vgdb remote connection is closed.

diff --git a/gdb/testsuite/gdb.base/valgrind-infcall.exp b/gdb/testsuite/gdb.base/valgrind-infcall.exp
index f2aa1ad..5faa8c8 100644
--- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
+++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
@@ -85,7 +85,7 @@ unset_board_info fileid
 
 clean_restart $executable
 
-gdb_test "$vgdbcmd" " in _start .*" "target remote for vgdb"
+gdb_test "$vgdbcmd" " in \\.?_start .*" "target remote for vgdb"
 
 gdb_test "monitor v.set gdb_output" "valgrind output will go to gdb.*"
 
@@ -97,6 +97,10 @@ while 1 {
 	    pass $test
 	    break
 	}
+	-re "Remote connection closed.*\r\n$gdb_prompt $" {
+	    fail "Remote connection closed. Testcase aborted"
+	    return -1
+	}
 	-re "\r\n$gdb_prompt $" {
 	    pass "$test (false warning)"
 	}

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 03 Aug 2012 17:01:56 +0200, Edjunior Barbosa Machado wrote:
> the patch below aborts the testcase if vgdb remote connection is closed,
> avoiding the testsuite to get stuck in the `continue' loop that happens when
> occurs the internal-error. It also considers the leading `.' for _start(),
> which is expected in ppc64.

OK with the change below, so that the same testcase name appears during diffs
of various testsuite results.


> --- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
> +++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
[...]
> @@ -97,6 +97,10 @@ while 1 {
>  	    pass $test
>  	    break
>  	}
> +	-re "Remote connection closed.*\r\n$gdb_prompt $" {
> +	    fail "Remote connection closed. Testcase aborted"

fail "$test (remote connection closed)"

> +	    return -1
> +	}
>  	-re "\r\n$gdb_prompt $" {
>  	    pass "$test (false warning)"
>  	}


Thanks,
Jan

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/03/2012 12:07 PM, Jan Kratochvil wrote:
> On Fri, 03 Aug 2012 17:01:56 +0200, Edjunior Barbosa Machado wrote:
>> the patch below aborts the testcase if vgdb remote connection is closed,
>> avoiding the testsuite to get stuck in the `continue' loop that happens when
>> occurs the internal-error. It also considers the leading `.' for _start(),
>> which is expected in ppc64.
> 
> OK with the change below, so that the same testcase name appears during diffs
> of various testsuite results.
> 
> 
>> --- a/gdb/testsuite/gdb.base/valgrind-infcall.exp
>> +++ b/gdb/testsuite/gdb.base/valgrind-infcall.exp
> [...]
>> @@ -97,6 +97,10 @@ while 1 {
>>  	    pass $test
>>  	    break
>>  	}
>> +	-re "Remote connection closed.*\r\n$gdb_prompt $" {
>> +	    fail "Remote connection closed. Testcase aborted"
> 
> fail "$test (remote connection closed)"
> 
>> +	    return -1
>> +	}
>>  	-re "\r\n$gdb_prompt $" {
>>  	    pass "$test (false warning)"
>>  	}
> 
> 
> Thanks,
> Jan
> 

Thanks!
Checked in with the aforementioned change:
	http://sourceware.org/ml/gdb-cvs/2012-08/msg00036.html

Btw, is it OK for 7.5 branch too?

-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 03 Aug 2012 18:42:47 +0200, Edjunior Barbosa Machado wrote:
> Checked in with the aforementioned change:
> 	http://sourceware.org/ml/gdb-cvs/2012-08/msg00036.html
> 
> Btw, is it OK for 7.5 branch too?

Yes, this testcase change really is.


Thanks,
Jan

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/03/2012 01:45 PM, Jan Kratochvil wrote:
> On Fri, 03 Aug 2012 18:42:47 +0200, Edjunior Barbosa Machado wrote:
>> Checked in with the aforementioned change:
>> 	http://sourceware.org/ml/gdb-cvs/2012-08/msg00036.html
>>
>> Btw, is it OK for 7.5 branch too?
> 
> Yes, this testcase change really is.

Checked in for 7.5:

	http://sourceware.org/ml/gdb-cvs/2012-08/msg00037.html

Thanks,
-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

Hi Edjunior,

On Fri, 03 Aug 2012 00:49:08 +0200, Edjunior Barbosa Machado wrote:
> I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:

FYI I got access to Fedora 17 ppc64 and even there the internal error is not
reproducible for me:
	kernel-3.3.4-5.fc17.ppc64
	valgrind-3.7.0-4.fc17.ppc64

(It works except for the _start vs. ._start problem.)


Thanks,
Jan

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Edjunior Barbosa Machado]

On 08/03/2012 05:22 PM, Jan Kratochvil wrote:
> Hi Edjunior,
> 
> On Fri, 03 Aug 2012 00:49:08 +0200, Edjunior Barbosa Machado wrote:
>> I've faced this internal-error when running this testcase on Fedora17 (which has valgrind-3.7.0) on ppc64:
> 
> FYI I got access to Fedora 17 ppc64 and even there the internal error is not
> reproducible for me:
> 	kernel-3.3.4-5.fc17.ppc64
> 	valgrind-3.7.0-4.fc17.ppc64
> 
> (It works except for the _start vs. ._start problem.)
> 
> 
> Thanks,
> Jan
> 

As discussed with Jan in the irc channel, the culprit here was the XML support missing. After install expat-devel, valgrind-infcall.exp passed with no failure.
Btw, is there some way to check whether XML support is enabled in order to avoid this on the testcases?

Thanks,
-- 
Edjunior

Re: [commit+7.5] [patchv2] Write bpt at the ON_STACK bpt address

[Tom Tromey]

>>>>> "Edjunior" == Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com> writes:

Edjunior> Btw, is there some way to check whether XML support is enabled in
Edjunior> order to avoid this on the testcases?

I think gdb.exp:gdb_skip_xml_test does what you want.

Tom

[commit] valgrind-db-attach.exp: Do not run in remote mode [Re: [patchv2] Write bpt at the ON_STACK bpt address]

[Jan Kratochvil]

On Fri, 27 Jul 2012 20:46:33 +0200, Jan Kratochvil wrote:
> gdb/testsuite/
> 2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.base/valgrind-db-attach.exp: Do not run in remote mode.

Checked in also this part (not for 7.5).


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2012-07/msg00258.html

--- src/gdb/testsuite/ChangeLog	2012/07/31 07:33:15	1.3313
+++ src/gdb/testsuite/ChangeLog	2012/07/31 07:35:17	1.3314
@@ -1,8 +1,10 @@
-2012-07-27  Jan Kratochvil  <jan.kratochvil@redhat.com>
+2012-07-31  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* gdb.base/valgrind-infcall.c: New file.
 	* gdb.base/valgrind-infcall.exp: New file.
 
+	* gdb.base/valgrind-db-attach.exp: Do not run in remote mode.
+
 2012-07-30  Doug Evans  <dje@google.com>
 
 	* gdb.dwarf2/fission-reread.S: Use .data instead of .bss.
--- src/gdb/testsuite/gdb.base/valgrind-db-attach.exp	2012/01/30 06:48:08	1.11
+++ src/gdb/testsuite/gdb.base/valgrind-db-attach.exp	2012/07/31 07:35:18	1.12
@@ -13,6 +13,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+if [is_remote target] {
+    # The test always runs locally.
+    return 0
+}
+
 set test valgrind-db-attach
 set srcfile $test.c
 set executable $test

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Maciej W. Rozycki]

On Thu, 26 Jul 2012, Jan Kratochvil wrote:

> Philippe, do you have an easy enough way to regression test it on mips when
> you was asking for the mips fix?  mips has many execution modes as I see.

 Thanks for cc-ing me, I'll push it through regression testing on MIPS16 
and microMIPS targets that are usually the most interesting (in addition 
to some regular MIPS ones).  Please note that this will be with the 
ISA-bit patch being reviewed applied; it does not make much sense to do 
any MIPS16/microMIPS testing without it as the results are too poor to 
accept, including in particular manual function calls being considered, 
and it also takes insane amounts of time for all the failing test cases to 
get past the timeouts.

 Does it need native testing or is remote testing enough to cover it?

 As a side note I'll try to find some time in the next couple of days to 
give 7.4.91 some testing too -- to see if there wasn't anything that may 
have escaped at the last moment.  Regrettably I've been swamped with other 
stuff recently.

  Maciej

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 27 Jul 2012 01:14:29 +0200, Maciej W. Rozycki wrote:
>  Does it need native testing or is remote testing enough to cover it?

I do not think it matters, infcalls work the same for both.

As Philippe has already tested mips compatibility with valgrind only GDB
testsuite in some of the appropriate MIPS ISAs would be nice before I check it
in also for 7.5.


Thanks,
Jan

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Maciej W. Rozycki]

On Fri, 27 Jul 2012, Jan Kratochvil wrote:

> >  Does it need native testing or is remote testing enough to cover it?
> 
> I do not think it matters, infcalls work the same for both.

 Breakpoints may be inserted differently on some remote targets, but 
that's more a matter of Linux vs bare-iron targets.  Anyway, thanks for 
confirming that no native testing is required, this is always problematic.

> As Philippe has already tested mips compatibility with valgrind only GDB
> testsuite in some of the appropriate MIPS ISAs would be nice before I check it
> in also for 7.5.

 I have tested your change now, applied on top of current trunk and that 
caused no regressions throughout all the configurations involved, so it 
looks good to go in as far as I'm concerned.

 Sadly there are some regressions in the baseline I used, compared to the 
last previous test results I obtained a couple weeks ago.  Specifically 
these:

FAIL: gdb.base/dprintf.exp: 1st dprintf, gdb
FAIL: gdb.base/dprintf.exp: 2nd dprintf, gdb

FAIL: gdb.base/list.exp: list range; filename:line1,filename:line2
FAIL: gdb.base/list.exp: list range; line1,line2
FAIL: gdb.base/list.exp: list range; upper bound past EOF
FAIL: gdb.base/list.exp: list range; both bounds past EOF
FAIL: gdb.base/list.exp: list range, must be same files

FAIL: gdb.linespec/ls-errs.exp: break 3:
FAIL: gdb.linespec/ls-errs.exp: break +10:
FAIL: gdb.linespec/ls-errs.exp: break -10:
FAIL: gdb.linespec/ls-errs.exp: break 3:
FAIL: gdb.linespec/ls-errs.exp: break +10:
FAIL: gdb.linespec/ls-errs.exp: break -10:
FAIL: gdb.linespec/ls-errs.exp: break 3	:
FAIL: gdb.linespec/ls-errs.exp: break +10	:
FAIL: gdb.linespec/ls-errs.exp: break -10	:
FAIL: gdb.linespec/ls-errs.exp: break 3	:
FAIL: gdb.linespec/ls-errs.exp: break +10	:
FAIL: gdb.linespec/ls-errs.exp: break -10	:

(notice the test message duplication in the last series -- that's a bug by 
itself).

 Do any of these ring anyone's bell?  They appear across both Linux and 
bare-iron configurations and all multilibs so must be pretty generic and 
from the log files they look genuine.  Regrettably I haven't yet grown 
myself a regular overnight testing facility so that I could catch such 
issues early.

  Maciej

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Sat, 28 Jul 2012 02:10:23 +0200, Maciej W. Rozycki wrote:
>  I have tested your change now, applied on top of current trunk and that 
> caused no regressions throughout all the configurations involved, so it 
> looks good to go in as far as I'm concerned.

OK, I will check it in.


>  Sadly there are some regressions in the baseline I used, compared to the 
> last previous test results I obtained a couple weeks ago.  Specifically 
> these:
> 
> FAIL: gdb.base/dprintf.exp: 1st dprintf, gdb
> FAIL: gdb.base/dprintf.exp: 2nd dprintf, gdb
[ and other FAILs ]

It was a very recent regression lasting only one day, fixed by Keith:
	Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers
	http://sourceware.org/ml/gdb-patches/2012-07/msg00593.html

commit 5134bbd001531c5c6d02573d4d7fb86b17a549ec
Author: Keith Seitz <keiths@redhat.com>
Date:   Thu Jul 26 16:22:44 2012 +0000
	* linespec.c (linespec_lexer_lex_number): The input
	is also a valid number if the next character is a comma
	or colon.

Thanks,
Jan

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Maciej W. Rozycki]

On Sat, 28 Jul 2012, Jan Kratochvil wrote:

> >  Sadly there are some regressions in the baseline I used, compared to the 
> > last previous test results I obtained a couple weeks ago.  Specifically 
> > these:
> > 
> > FAIL: gdb.base/dprintf.exp: 1st dprintf, gdb
> > FAIL: gdb.base/dprintf.exp: 2nd dprintf, gdb
> [ and other FAILs ]
> 
> It was a very recent regression lasting only one day, fixed by Keith:
> 	Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers
> 	http://sourceware.org/ml/gdb-patches/2012-07/msg00593.html

 OK, thanks, now gone indeed.

  Maciej

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Philippe Waroquiers]

On Thu, 2012-07-26 at 23:23 +0200, Jan Kratochvil wrote:

> Attached.  I no longer find it useful as arch-dependent code, it would do the
> same in each arch.
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu.
Tested the patch on f12/x86 with Valgrind 3.7.0 and Valgrind 3.8.0 SVN,
+ tested on gcc49(mips32) with Valgrind 3.8.0 SVN (mips support appears
in 3.8.0)

No ugly error messages produced by Valgrind anymore.

Thanks

Philippe

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Jan Kratochvil]

On Fri, 27 Jul 2012 01:15:45 +0200, Philippe Waroquiers wrote:
> No ugly error messages produced by Valgrind anymore.

That is it no longer prints on each infcall:
	vex amd64->IR: unhandled instruction bytes: 0xFF 0xFE 0x7 0x0 0x0 0x0 0x9 0x0


Thanks,
Jan

Re: [patchv2] Write bpt at the ON_STACK bpt address

[Pedro Alves]

On 07/26/2012 10:23 PM, Jan Kratochvil wrote:
>> But it sounds like this is forcing GDB to have insider knowledge of
>> > valgrind.
> As you were asking to put there 'int3' (and not 'hlt') the user convenience
> coincidentally matches here with what Philippe asks for valgrind.
> 
> 
> I am only concerned a bit about this change arcross all archs for 7.5.
> Maybe 7.5 could limit this patch only for i386/x86_64 which is well
> understood.

FWIW, the patch looks good to me, and I don't think that
limiting to x86 is necessary: the instruction should never
be executed; and we're already writing to the stack using functions
that throw, one more write should be any different.  If that fails,
the other writes would too.

> +	/* Write a legitimate instruction at the point where the infcall
> +	   breakpoint is going to be inserted.  While this instruction
> +	   is never going to be executed, a user investigating the
> +	   memory from GDB would see this instruction instead of random
> +	   uninitialized bytes.  We chose the breakpoint instruction
> +	   just because it may look as the most logical one to the user.
> +
> +	   If software breakpoints are unsupported for this target we
> +	   leave the user visible memory content uninitialized.  */
> +

IMO, this comment should also mention that in addition to being nice for
the user, this is actually _necessary_ for at least Valgrind v XXX.YYY.

-- 
Pedro Alves

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Wed, 2012-07-25 at 15:39 -0700, Joel Brobecker wrote:

> What I am trying to do, is make sure that new GDB versions work well
> with older versions of valgrind (although, isn't gdbserver support
> relatively recent?), while at the same time trying to make future
> versions of valgrind more robust. I don't know how long we are going
> to be able to keep the workaround. What if other tools implementing
> the remote protocol had the same problem, and they required us to
> insert a different instruction instead?
Valgrind gdbsrv appeared in the last Valgrind version (3.7.0).
If GDB modifies the stack to put a 0xCC, that should work with old
Valgrind (I will test in any case).

Otherwise, effectively, it is always possible that another "strange"
gdbserver must have another instruction.
I imagine however that a breakpoint trap instruction will be the
most likely common denominator amongst "strange" gdbserver.
The other "strange" gdbsrv I know about is the qemu one.
Just tried to build the last qemu to see how it works,
but build failed.

Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Thu, 2012-07-26 at 23:56 +0200, Philippe Waroquiers wrote:
> I imagine however that a breakpoint trap instruction will be the
> most likely common denominator amongst "strange" gdbserver.
> The other "strange" gdbsrv I know about is the qemu one.
> Just tried to build the last qemu to see how it works,
> but build failed.
Finally succeeded to build qemu on i386, but its gdbserver encounters
an error when executing an inferior call (at least on i386).
Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Jan Kratochvil]

On Wed, 25 Jul 2012 23:46:15 +0200, Philippe Waroquiers wrote:
> A valid instruction is not enough. We need a valid instruction
> that will cause Valgrind to terminate block translation.
> The breakpoint trap instruction is ok for that.
> (0xcc for x86 and amd64, 0x0005000d for mips32).

I understand now that int3 is enough for valgrind.
I still do not understand whether hlt is enough for valgrind.
I will try how hlt behaves with a testcase.

Still as you both (with Joel) seem to prefer int3 over hlt I will put int3
there.  IIUC your goals this will make false feeling to the user s/he
understands what is going which seems to be better than if the user sees hlt
which may be more unclear to him/her.


Thanks,
Jan

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Pedro Alves]

On 07/25/2012 09:24 PM, Philippe Waroquiers wrote:
> On Wed, 2012-07-25 at 15:58 +0100, Pedro Alves wrote:
>> On 07/23/2012 09:36 PM, Philippe Waroquiers wrote:
>>
>>>> So the GDB patch is no longer needed when you have fixed valgrind to put 0xcc
>>>> during Z0? Why valgrind cannot write 0xcc into stack memory when it already
>>>> has to write there to create the stack frame / parameters passed by stack?
>>> Effectively, I have a patch which fixes the problem.
>>> But the patch is a kludge which heuristically guesses that GDB is
>>> pushing an infcall.
>>
>> Why do you have to guess that, rather than just detecting a breakpoint is
>> being set on a stack (or non text) address?  If something sets a breakpoint
>> in a data address, it is basically telling valgrind "this is actually code".
> 
> This is explained by the way Valgrind gdbsrv (must) implement
> breakpoints.
> (this is a little bit tricky, as it is linked to Valgrind internals).
> 
> Valgrind translates guest code instructions in small blocks.
> As part of the translation, if there is a breakpoint at addr XXXX
> then the translation of address XXXX will start with a call to a
> helper function which reports to GDB that a breakpoint has been
> encountered. This function then reads/executes protocol packets till a
> continue packet is received.
> The translated block is then continued <<< This is the critical info !!!
> 
> There is no way to re-translate the block currently being executed :
> Valgrind has no way to "drop" the translated block it is currently
> executing.

So if you interrupt valgrind, and then set a breakpoint at or near the
address currently being executed, that breakpoint will be ignored?  I'm guessing
there's some mechanism to re-translate and hook a new block to handle that case.

> So, a breakpoint cannot be translated using a 0xCC because when GDB
> tells to continue after the breakpoint, there is no way to retranslate
> the original instructions (without the 0xCC) as long as the block
> is being executed.

Which would sound like a similar issue.  Is this a current limitation,
or something that Will Never Work?

> So, for normal breakpoints, Valgrind gdbsrv cannot insert 0xCC, as this
> would just not work.
> 
> "Normal" breakpoints on the stack (trampoline code or whatever) or
> JITted code or ... must be handled the same way: V gdbsrv cannot
> touch the code to handle breakpoints.
> 
> The only special case in which Valgrind gdbsrv can insert a 0xCC is
> when it is sure that this code will *not* be executed.
> This is the case for the 0xcc for the push_dummy_code.
> This code will not be executed because GDB will change the pc register.

"this code" is a bit ambiguous in this sentence.  You mean, the code that
was there if we didn't put a 0xcc in place, I presume.

> When the continue packet is received, the execution of the block is
> then not continued, instead the continue will cause a jump to the
> "original pc" (the one before the infcall).
>
> So, if it is easy to change GDB to insert 0xcc (for x86 and amd84)
> and the equivalent breakpoint instr for mips32, then that avoids
> the kludgy patch in Valgrind, which is for sure fragile.

It adds a kludgy patch in GDB, for what sounds like a current Valgrind
limitation, so I'd like to explore all possibilities.

Why doesn't Valgrind trigger a translation of blocks with breakpoints
as soon as a Z0 is inserted?  That way, when the forced infcall returns,
it'd find a translated breakpoint already, even without a 0xcc inserted,
instead of valgrind finding that the block hadn't been translated yet,
and ending up translating a random, possibly invalid instruction.

-- 
Pedro Alves

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Philippe Waroquiers]

On Thu, 2012-07-26 at 13:48 +0100, Pedro Alves wrote:
> On 07/25/2012 09:24 PM, Philippe Waroquiers wrote:
> >
> > There is no way to re-translate the block currently being executed :
> > Valgrind has no way to "drop" the translated block it is currently
> > executing.
> 
> So if you interrupt valgrind, and then set a breakpoint at or near the
> address currently being executed, that breakpoint will be ignored?  I'm guessing
> there's some mechanism to re-translate and hook a new block to handle that case.
Yes, such breakpoints will be ignored (in some cases). It depends
how GDB regained control (interrupt while running, or due to Valgrind
reporting an error, or interrupted while all threads are in a syscall).

I would have liked to find a reasonable solution working in all
cases, but could not find one.
=> due to this lack, the option to to activate the Valgrind gdbsrv
has 3 values:
   --vgdb=no   (gdbsrv is not active)
   --vgdb=yes  (default value, neglectible overhead compared to =no)
   --vgdb=full (a lot slower == 500% slower,
                it always puts a helper function call for
                each instruction).
                With this, breakpoints will not be ignored
                and debugging will be as precise as possible.
See
http://www.valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.gdbserver-limitations


> 
> > So, a breakpoint cannot be translated using a 0xCC because when GDB
> > tells to continue after the breakpoint, there is no way to retranslate
> > the original instructions (without the 0xCC) as long as the block
> > is being executed.
> 
> Which would sound like a similar issue.  Is this a current limitation,
> or something that Will Never Work?
Yes, it is the same issue. Currently, it is in state Will Never Work,
as I have no idea how to properly attack this without deep changes
in Valgrind core.

> Why doesn't Valgrind trigger a translation of blocks with breakpoints
> as soon as a Z0 is inserted?  That way, when the forced infcall returns,
> it'd find a translated breakpoint already, even without a 0xcc inserted,
> instead of valgrind finding that the block hadn't been translated yet,
> and ending up translating a random, possibly invalid instruction.
When a Z0 is received, Valgrind gdbsrv immediately marks that the
address in the Z0 has to be re-translated.
But that will not happen as long as the control is not returned to the
Valgrind "scheduler" (kind of main loop translating and executing the
translated blocks).


Philippe

Re: [patch] [i386] Put hlt at the ON_STACK breakpoint [Re: GDB 7.4.91 available for testing]

[Pedro Alves]

On 07/26/2012 11:21 PM, Philippe Waroquiers wrote:

>> Which would sound like a similar issue.  Is this a current limitation,
>> or something that Will Never Work?
> Yes, it is the same issue. Currently, it is in state Will Never Work,
> as I have no idea how to properly attack this without deep changes
> in Valgrind core.

Oh well.  Let's go with GDB writing a breakpoint then.

Thanks for the explanations.

-- 
Pedro Alves