Re: [PATCH,v3] [PR gdb/29272] Make sure a copy_insn_closure is available when we have a match in copy_insn_closure_by_addr

[Simon Marchi <simon.marchi@efficios.com>]

On 11/11/22 04:32, Luis Machado wrote:
> v2: Add try/catch block
> v3: Let it throw after all, and make the rollback cleaner.
> 
> PR gdb/29272
> 
> Investigating PR29272, it was mentioned a particular test used to work on
> GDB 10, but it started failing with GDB 11 onwards. I tracked it down to
> some displaced stepping improvements on commit
> 187b041e2514827b9d86190ed2471c4c7a352874.
> 
> In particular, one of the corner cases using copy_insn_closure_by_addr got
> silently broken. It is hard to spot because it doesn't have any good tests
> for it, and the situation is quite specific to the Arm target.
> 
> Essentially, the change from the displaced stepping improvements made it so
> we could still invoke copy_insn_closure_by_addr correctly to return the
> pointer to a copy_insn_closure, but it always returned nullptr due to
> the order of the statements in displaced_step_buffer::prepare.
> 
> The way it is now, we first write the address of the displaced step buffer
> to PC and then save the copy_insn_closure pointer.
> 
> The problem is that writing to PC for the Arm target requires figuring
> out if the new PC is thumb mode or not.
> 
> With no copy_insn_closure data, the logic to determine the thumb mode
> during displaced stepping doesn't work, and gives random results that
> are difficult to track (SIGILL, SIGSEGV etc).
> 
> Fix this by reordering the PC write in displaced_step_buffer::prepare
> and, for safety, add an assertion to
> displaced_step_buffer::copy_insn_closure_by_addr so GDB stops right
> when it sees this invalid situation. If this gets broken again in the
> future, it will be easier to spot.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29272

LGTM, thanks:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon