On Tue, 25 Nov 2008 07:12:35 +0100, Joel Brobecker wrote:
> > Still I rather found another defect compared to mem-break.c in the ia64
> > breakpoints code.  It is another problem unrelated to the original intention
> > of this ia64 patch.
> 
> Jan, please don't take it personally, I really appreciate the way
> you are trying to make your patch better and better.  But since
> this was unrelated, I really wished that you committed the first part
> as approved, and then sent a followup patch that dealt with that part
> on its own.

As I got also this mail before the commit
On Sat, 22 Nov 2008 12:30:04 +0100, Eli Zaretskii wrote:
> However, I'm slightly worried by these parts in your code:

I considered it as a valid veto of your approval until Eli's concerns get
resolved.  I appreciate your time although I do not see I would make mistakes
in the unwritten formal approval process I try to follow.


> With the approach you took, I had to fish out the previous version of the
> patch, and then compare the two patches to see exactly what changed.

Using this VIM macro for reviewing "metadiffs":
  noremap <Esc>D :set hlsearch<cr>/^[+-][+-]\([^+-].*\\|\)$<cr>

The committed patch has these changes against the last post here:
ia64_memory_insert_breakpoint: make_show_memory_breakpoints_cleanup (0 -> 1);
 - A real Bug of my former patch (although it was not a regression).
ia64_memory_insert_breakpoint: == IA64_BREAKPOINT => internal_error
 - New sanity check, unaware it would be possible to reach it as a user.


> I propose a warning saying something like this:
> 
>    cannot remove breakpoint at address %s, no break instruction at such address.

Therefore included this warning again:
ia64_memory_remove_breakpoint: != IA64_BREAKPOINT => warning
 - Happens for: gdb.base/checkpoint.exp gdb.base/foll-fork.exp
 - IMO such sanity check is missing even in default_memory_remove_breakpoint
   and these warnings would be then present also on x86.  Not checked, though.



Thanks,
Jan