On Sat, 22 Nov 2008 12:30:04 +0100, Eli Zaretskii wrote: > > gdb/doc/ > > This part is approved, thanks. OK, thanks. On Sat, 22 Nov 2008 19:15:25 +0100, Joel Brobecker wrote: > > > + instr_breakpoint = slotN_contents (bundle_mem, slotnum); > > > + if (instr_breakpoint != IA64_BREAKPOINT) > > > + warning (_("Breakpoint removal cannot find the placed breakpoint at %s"), > > > + paddr_nz (bp_tgt->placed_address)); > > > > Can this happen as part of normal GDB behavior? If not, we should > > make this internal_error, I think. If indeed this is a warning, we > > should tell users what to do with such a warning, either as part of > > the message or at least in the manual. [...] > I would agree that some extra documentation explaining this warning > might be useful. That being said, I'm not opposed to changing the > warning into an internal_error, if you still think it's better. As the remote target memory may change arbitrarily across time I do not find there internal error appropriate. 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. target_write_memory result was not checked and thus failed write to readonly memory still made ia64_memory_insert_breakpoint to succeed. The write fails in the testcases for VDSO memory range which is not writable on ia64: kernel-xen-2.6.18-92.el5.ia64 a000000000000000-a000000000020000 r-xp 00000000 00:00 0 [vdso] ptrace(PTRACE_PEEKTEXT, 30592, 0xa000000000000000, NULL) = 282584257676671 ptrace(PTRACE_PEEKTEXT, 30592, 0xa000000000000008, NULL) = 0 ptrace(PTRACE_POKEDATA, 30592, 0xa000000000000000, 0x100006666601f) = -1 EIO (Input/output error) ptrace(PTRACE_POKETEXT, 30592, 0xa000000000000000, 0x100006666601f) = -1 EIO (Input/output error) But on x86_64 (using PTRACE_POKE* error checking mem-break.c) it is writable: kernel-2.6.25.10-86.fc9.x86_64 7ffff7ffd000-7ffff7fff000 r-xp 7ffff7ffd000 00:00 0 [vdso] ptrace(PTRACE_PEEKTEXT, 2761, 0x7ffff7ffd7a0, [0x8bfffffd41058b48]) = 0 ptrace(PTRACE_POKEDATA, 2761, 0x7ffff7ffd7a0, 0x8bfffffd41058bcc) = 0 ptrace(PTRACE_PEEKTEXT, 2761, 0x7ffff7ffd7a0, [0x8bfffffd41058bcc]) = 0 The attached patch changes on ia64 the breakpoints to readonly memory to fail: (gdb) b *0xa000000000000000 Breakpoint 2 at 0xa000000000000000 Warning: Cannot insert breakpoint 2. Error accessing memory address 0xa000000000000000: Input/output error. (gdb) info break Num Type Disp Enb Address What 2 breakpoint keep y 0xa000000000000000 (gdb) delete 2 (gdb) Unaware how to invoke the ia64_memory_remove_breakpoint error of (instr_breakpoint != IA64_BREAKPOINT) but by patching GDB code one can get: (gdb) b *0xa000000000000000 Breakpoint 2 at 0xa000000000000000 (gdb) info break Num Type Disp Enb Address What 2 breakpoint keep y 0xa000000000000000 (gdb) delete 2 warning: Error removing breakpoint 2 (gdb) info break No breakpoints or watchpoints. For the ia64-specific code: > > > + instr_breakpoint = slotN_contents (bundle_mem, slotnum); > > > + if (instr_breakpoint != IA64_BREAKPOINT) > > > + warning (_("Breakpoint removal cannot find the placed breakpoint at %s"), > > > + paddr_nz (bp_tgt->placed_address)); -> + instr_breakpoint = slotN_contents (bundle_mem, slotnum); + if (instr_breakpoint != IA64_BREAKPOINT) + return -1; I am open to a possibility to also give a warning() there besides the current failure (-1) return. Requesting another approval due to this change although the patch covers now more ia64 parts than it was not intended for. Thanks for pinpointing this another ia64-tdep.c bug, Jan ia64 testsuite changes (no regression) with this patch: -FAIL: gdb.base/advance.exp: advance line number +PASS: gdb.base/advance.exp: advance line number -FAIL: gdb.base/advance.exp: advance func +PASS: gdb.base/advance.exp: advance func -FAIL: gdb.base/advance.exp: advance function not called by current frame +PASS: gdb.base/advance.exp: advance function not called by current frame -FAIL: gdb.base/advance.exp: continue to call to func3 in main (the program is no longer running) +PASS: gdb.base/advance.exp: continue to call to func3 in main -FAIL: gdb.base/advance.exp: advance with no argument +PASS: gdb.base/advance.exp: advance with no argument -FAIL: gdb.base/sigaltstack.exp: finish from catch LEAF +FAIL: gdb.base/sigaltstack.exp: finish from catch LEAF (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish to throw INNER +FAIL: gdb.base/sigaltstack.exp: finish to throw INNER (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish to catch INNER +FAIL: gdb.base/sigaltstack.exp: finish to catch INNER (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish from catch INNER +FAIL: gdb.base/sigaltstack.exp: finish from catch INNER (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish to OUTER +FAIL: gdb.base/sigaltstack.exp: finish to OUTER (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish to catch MAIN +FAIL: gdb.base/sigaltstack.exp: finish to catch MAIN (could not set breakpoint) -FAIL: gdb.base/sigaltstack.exp: finish to MAIN +FAIL: gdb.base/sigaltstack.exp: finish to MAIN (could not set breakpoint) -FAIL: gdb.base/sigstep.exp: finish from handleri; leave handler (hit breakpoint again) -FAIL: gdb.base/sigstep.exp: finish from handleri; leave signal trampoline +FAIL: gdb.base/sigstep.exp: finish from handleri; leave handler (could not set breakpoint) +Running ../.././gdb/testsuite/gdb.base/breakpoint-shadow.exp ... +PASS: gdb.base/breakpoint-shadow.exp: set breakpoint always-inserted on +PASS: gdb.base/breakpoint-shadow.exp: show breakpoint always-inserted +PASS: gdb.base/breakpoint-shadow.exp: disassembly without breakpoints +PASS: gdb.base/breakpoint-shadow.exp: First breakpoint placed +PASS: gdb.base/breakpoint-shadow.exp: Second breakpoint placed +PASS: gdb.base/breakpoint-shadow.exp: disassembly with breakpoints