Re: [patch] ia64: Fix breakpoints memory shadow

Joel Brobecker
2008-11-25T06:13:24Z

> 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. 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. And since I've always found that a diff of diff is some
kind of mental gymnastics, and the whole process is more work than just
looking at the subsequent patch, I had to delay the review until I had
30min of quiet time.

Anyway, back to the one important change:

> - memcpy (&instr, bp_tgt->shadow_contents, sizeof instr);
> - replace_slotN_contents (bundle, instr, slotnum);
> + instr_breakpoint = slotN_contents (bundle_mem, slotnum);
> + if (instr_breakpoint != IA64_BREAKPOINT)
> + return -1;

I think we need to give the user a way to understand the error
message that returning -1 will cause. Otherwise, it's going to be
hard for the user to have a clue of why the debugger failed to
de-insert the breakpoint. I propose a warning saying something
like this:

cannot remove breakpoint at address %s, no break instruction at such address.

Cheers,
--
Joel