From 0f26cec1fd54c0428fda6c93c0375400e1bca738 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 5 Mar 2014 14:18:28 +0000 Subject: [PATCH] PR gdb/16575: stale breakpoint instructions in the code cache In non-stop mode, or rather, breakpoints always-inserted mode, the code cache can easily end up with stale breakpoint instructions: All it takes is filling a cache line when breakpoints already exist in that memory region, and then delete the breakpoint. Vis. (from the new test): (gdb) set breakpoint always-inserted on (gdb) b 23 Breakpoint 2 at 0x400540: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 23. (gdb) b 24 Breakpoint 3 at 0x400547: file ../../../src/gdb/testsuite/gdb.base/breakpoint-shadow.c, line 24. disass main Dump of assembler code for function main: 0x000000000040053c <+0>: push %rbp 0x000000000040053d <+1>: mov %rsp,%rbp => 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp) 0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp) 0x000000000040054e <+18>: mov $0x0,%eax 0x0000000000400553 <+23>: pop %rbp 0x0000000000400554 <+24>: retq End of assembler dump. So far so good. Now flush the code cache: (gdb) set code-cache off (gdb) set code-cache on Requesting a disassembly works as expected, breakpoint shadowing is applied: (gdb) disass main Dump of assembler code for function main: 0x000000000040053c <+0>: push %rbp 0x000000000040053d <+1>: mov %rsp,%rbp => 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp) 0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp) 0x000000000040054e <+18>: mov $0x0,%eax 0x0000000000400553 <+23>: pop %rbp 0x0000000000400554 <+24>: retq End of assembler dump. However, now delete the breakpoints: (gdb) delete Delete all breakpoints? (y or n) y And disassembly shows the old breakpoint instructions: (gdb) disass main Dump of assembler code for function main: 0x000000000040053c <+0>: push %rbp 0x000000000040053d <+1>: mov %rsp,%rbp => 0x0000000000400540 <+4>: int3 0x0000000000400541 <+5>: rex.RB cld 0x0000000000400543 <+7>: add %eax,(%rax) 0x0000000000400545 <+9>: add %al,(%rax) 0x0000000000400547 <+11>: int3 0x0000000000400548 <+12>: rex.RB cld 0x000000000040054a <+14>: add (%rax),%al 0x000000000040054c <+16>: add %al,(%rax) 0x000000000040054e <+18>: mov $0x0,%eax 0x0000000000400553 <+23>: pop %rbp 0x0000000000400554 <+24>: retq End of assembler dump. Those breakpoint instructions are no longer installed in target memory they're stale in the code cache. Easily confirmed by just disabling the code cache: (gdb) set code-cache off (gdb) disass main Dump of assembler code for function main: 0x000000000040053c <+0>: push %rbp 0x000000000040053d <+1>: mov %rsp,%rbp => 0x0000000000400540 <+4>: movl $0x1,-0x4(%rbp) 0x0000000000400547 <+11>: movl $0x2,-0x4(%rbp) 0x000000000040054e <+18>: mov $0x0,%eax 0x0000000000400553 <+23>: pop %rbp 0x0000000000400554 <+24>: retq End of assembler dump. I stumbled upon this when writing a patch to infrun.c, that made handle_inferior_event & co fill in the cache before breakpoints were removed from the target. Recall that wait_for_inferior flushes the dcache for every event. So in that case, always-inserted mode was not necessary to trigger this. It's just a convenient way to expose the issue. The dcache works at the raw memory level. We need to update it whenever memory is written, no matter what kind of target memory object was originally passed down by the caller. The issue is that the dcache update code isn't reached when a caller explicitly writes raw memory. Breakpoint insertion/removal is one such case -- mem-break.c uses target_write_read_memory/target_write_raw_memory. The fix is to move the dcache update code from memory_xfer_partial_1 to raw_memory_xfer_partial so that it's always reachable. When we do that, we can actually simplify a series of things. memory_xfer_partial_1 no longer needs to handle writes for any kind of memory object, and therefore dcache_xfer_memory no longer needs to handle writes either. So the latter (dcache_xfer_memory) and its callees can be simplified to only care about reads. While we're touching dcache_xfer_memory's prototype, might as well rename it to reflect that fact that it only handles reads, and make it follow the new target_xfer_status/xfered_len style. This made me notice that dcache_xfer_memory loses the real error status if a memory read fails: we could have failed to read due to TARGET_XFER_E_UNAVAILABLE, for instance, but we always return TARGET_XFER_E_IO, hence the FIXME note. I felt that fixing that fell out of the scope of this patch. Currently dcache_xfer_memory handles the case of a write failing. The whole cache line is invalidated when that happens. However, dcache_update, the sole mechanism for handling writes that will remain after the patch, does not presently handle that scenario. That's a bug. The patch makes it handle that, by passing down the target_xfer_status status from the caller, so that it can better decide what to do itself. While I was changing the function's prototype, I constified the myaddr parameter, getting rid of the need for the cast as seen in its existing caller. Tested on x86_64 Fedora 17, native and gdbserver. gdb/ 2014-03-05 Pedro Alves PR gdb/16575 * dcache.c (dcache_poke_byte): Constify ptr parameter. Return void. Update comment. (dcache_xfer_memory): Delete. (dcache_read_memory_partial): New, based on the read bits of dcache_xfer_memory. (dcache_update): Add status parameter. Use ULONGEST for len, and adjust. Discard cache lines if the reason for the update was error. * dcache.h (dcache_xfer_memory): Delete declaration. (dcache_read_memory_partial): New declaration. (dcache_update): Update prototype. * target.c (raw_memory_xfer_partial): Update the dcache here. (memory_xfer_partial_1): Don't handle dcache writes here. gdb/testsuite/ 2014-03-05 Pedro Alves PR gdb/16575 * gdb.base/breakpoint-shadow.exp (compare_disassembly): New procedure. (top level): Adjust to use it. Add tests that exercise breakpoint interaction with the code-cache. --- gdb/ChangeLog | 17 +++++ gdb/dcache.c | 100 ++++++++++++--------------- gdb/dcache.h | 15 ++-- gdb/target.c | 53 ++++++-------- gdb/testsuite/ChangeLog | 8 +++ gdb/testsuite/gdb.base/breakpoint-shadow.exp | 38 +++++++--- 6 files changed, 127 insertions(+), 104 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 8108e50..b9ac372 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,20 @@ +2014-03-05 Pedro Alves + + PR gdb/16575 + * dcache.c (dcache_poke_byte): Constify ptr parameter. Return + void. Update comment. + (dcache_xfer_memory): Delete. + (dcache_read_memory_partial): New, based on the read bits of + dcache_xfer_memory. + (dcache_update): Add status parameter. Use ULONGEST for len, and + adjust. Discard cache lines if the reason for the update was + error. + * dcache.h (dcache_xfer_memory): Delete declaration. + (dcache_read_memory_partial): New declaration. + (dcache_update): Update prototype. + * target.c (raw_memory_xfer_partial): Update the dcache here. + (memory_xfer_partial_1): Don't handle dcache writes here. + 2014-03-05 Mike Frysinger * remote-sim.c (gdbsim_load): Add const to prog. diff --git a/gdb/dcache.c b/gdb/dcache.c index ea51f5b..d3b546b 100644 --- a/gdb/dcache.c +++ b/gdb/dcache.c @@ -413,24 +413,20 @@ dcache_peek_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr) /* Write the byte at PTR into ADDR in the data cache. - The caller is responsible for also promptly writing the data - through to target memory. + The caller should have written the data through to target memory + already. - If addr is not in cache, this function does nothing; writing to - an area of memory which wasn't present in the cache doesn't cause - it to be loaded in. + If ADDR is not in cache, this function does nothing; writing to an + area of memory which wasn't present in the cache doesn't cause it + to be loaded in. */ - Always return 1 (meaning success) to simplify dcache_xfer_memory. */ - -static int -dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr) +static void +dcache_poke_byte (DCACHE *dcache, CORE_ADDR addr, const gdb_byte *ptr) { struct dcache_block *db = dcache_hit (dcache, addr); if (db) db->data[XFORM (dcache, addr)] = *ptr; - - return 1; } static int @@ -467,26 +463,17 @@ dcache_init (void) } -/* Read or write LEN bytes from inferior memory at MEMADDR, transferring - to or from debugger address MYADDR. Write to inferior if SHOULD_WRITE is - nonzero. +/* Read LEN bytes from dcache memory at MEMADDR, transferring to + debugger address MYADDR. If the data is presently cached, this + fills the cache. Arguments/return are like the target_xfer_partial + interface. */ - Return the number of bytes actually transfered, or -1 if the - transfer is not supported or otherwise fails. Return of a non-negative - value less than LEN indicates that no further transfer is possible. - NOTE: This is different than the to_xfer_partial interface, in which - positive values less than LEN mean further transfers may be possible. */ - -int -dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache, - CORE_ADDR memaddr, gdb_byte *myaddr, - int len, int should_write) +enum target_xfer_status +dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache, + CORE_ADDR memaddr, gdb_byte *myaddr, + ULONGEST len, ULONGEST *xfered_len) { - int i; - int res; - int (*xfunc) (DCACHE *dcache, CORE_ADDR addr, gdb_byte *ptr); - - xfunc = should_write ? dcache_poke_byte : dcache_peek_byte; + ULONGEST i; /* If this is a different inferior from what we've recorded, flush the cache. */ @@ -497,35 +484,27 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache, dcache->ptid = inferior_ptid; } - /* Do write-through first, so that if it fails, we don't write to - the cache at all. */ - - if (should_write) - { - res = target_write (ops, TARGET_OBJECT_RAW_MEMORY, - NULL, myaddr, memaddr, len); - if (res <= 0) - return res; - /* Update LEN to what was actually written. */ - len = res; - } - for (i = 0; i < len; i++) { - if (!xfunc (dcache, memaddr + i, myaddr + i)) + if (!dcache_peek_byte (dcache, memaddr + i, myaddr + i)) { /* That failed. Discard its cache line so we don't have a partially read line. */ dcache_invalidate_line (dcache, memaddr + i); - /* If we're writing, we still wrote LEN bytes. */ - if (should_write) - return len; - else - return i; + break; } } - - return len; + + if (i == 0) + { + /* FIXME: We lose the real error status. */ + return TARGET_XFER_E_IO; + } + else + { + *xfered_len = i; + return TARGET_XFER_OK; + } } /* FIXME: There would be some benefit to making the cache write-back and @@ -537,17 +516,26 @@ dcache_xfer_memory (struct target_ops *ops, DCACHE *dcache, "logically" connected but not actually a single call to one of the memory transfer functions. */ -/* Just update any cache lines which are already present. This is called - by memory_xfer_partial in cases where the access would otherwise not go - through the cache. */ +/* Just update any cache lines which are already present. This is + called by the target_xfer_partial machinery when writing raw + memory. */ void -dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr, int len) +dcache_update (DCACHE *dcache, enum target_xfer_status status, + CORE_ADDR memaddr, const gdb_byte *myaddr, + ULONGEST len) { - int i; + ULONGEST i; for (i = 0; i < len; i++) - dcache_poke_byte (dcache, memaddr + i, myaddr + i); + if (status == TARGET_XFER_OK) + dcache_poke_byte (dcache, memaddr + i, myaddr + i); + else + { + /* Discard the whole cache line so we don't have a partially + valid line. */ + dcache_invalidate_line (dcache, memaddr + i); + } } /* Print DCACHE line INDEX. */ diff --git a/gdb/dcache.h b/gdb/dcache.h index 780dc30..020abd6 100644 --- a/gdb/dcache.h +++ b/gdb/dcache.h @@ -32,12 +32,13 @@ DCACHE *dcache_init (void); /* Free a DCACHE. */ void dcache_free (DCACHE *); -/* Simple to call from _xfer_memory. */ - -int dcache_xfer_memory (struct target_ops *ops, DCACHE *cache, CORE_ADDR mem, - gdb_byte *my, int len, int should_write); - -void dcache_update (DCACHE *dcache, CORE_ADDR memaddr, gdb_byte *myaddr, - int len); +enum target_xfer_status + dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache, + CORE_ADDR memaddr, gdb_byte *myaddr, + ULONGEST len, ULONGEST *xfered_len); + +void dcache_update (DCACHE *dcache, enum target_xfer_status status, + CORE_ADDR memaddr, const gdb_byte *myaddr, + ULONGEST len); #endif /* DCACHE_H */ diff --git a/gdb/target.c b/gdb/target.c index 6cd9741..f7868c0 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1074,6 +1074,23 @@ raw_memory_xfer_partial (struct target_ops *ops, gdb_byte *readbuf, } while (ops != NULL); + /* The cache works at the raw memory level. Make sure the cache + gets updated with raw contents no matter what kind of memory + object was originally being written. Note we do write-through + first, so that if it fails, we don't write to the cache contents + that never made it to the target. */ + if (writebuf != NULL + && !ptid_equal (inferior_ptid, null_ptid) + && target_dcache_init_p () + && (stack_cache_enabled_p () || code_cache_enabled_p ())) + { + DCACHE *dcache = target_dcache_get (); + + /* Note that writing to an area of memory which wasn't present + in the cache doesn't cause it to be loaded in. */ + dcache_update (dcache, res, memaddr, writebuf, *xfered_len); + } + return res; } @@ -1225,6 +1242,7 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, inf = NULL; if (inf != NULL + && readbuf != NULL /* The dcache reads whole cache lines; that doesn't play well with reading from a trace buffer, because reading outside of the collected memory range fails. */ @@ -1234,23 +1252,9 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, || (code_cache_enabled_p () && object == TARGET_OBJECT_CODE_MEMORY))) { DCACHE *dcache = target_dcache_get_or_init (); - int l; - if (readbuf != NULL) - l = dcache_xfer_memory (ops, dcache, memaddr, readbuf, reg_len, 0); - else - /* FIXME drow/2006-08-09: If we're going to preserve const - correctness dcache_xfer_memory should take readbuf and - writebuf. */ - l = dcache_xfer_memory (ops, dcache, memaddr, (void *) writebuf, - reg_len, 1); - if (l <= 0) - return TARGET_XFER_E_IO; - else - { - *xfered_len = (ULONGEST) l; - return TARGET_XFER_OK; - } + return dcache_read_memory_partial (ops, dcache, memaddr, readbuf, + reg_len, xfered_len); } /* If none of those methods found the memory we wanted, fall back @@ -1266,23 +1270,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object, res = raw_memory_xfer_partial (ops, readbuf, writebuf, memaddr, reg_len, xfered_len); - /* Make sure the cache gets updated no matter what - if we are writing - to the stack. Even if this write is not tagged as such, we still need - to update the cache. */ - - if (res == TARGET_XFER_OK - && inf != NULL - && writebuf != NULL - && target_dcache_init_p () - && !region->attrib.cache - && ((stack_cache_enabled_p () && object != TARGET_OBJECT_STACK_MEMORY) - || (code_cache_enabled_p () && object != TARGET_OBJECT_CODE_MEMORY))) - { - DCACHE *dcache = target_dcache_get (); - - dcache_update (dcache, memaddr, (void *) writebuf, reg_len); - } - /* If we still haven't got anything, return the last error. We give up. */ return res; diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 835338f..65b4f95 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,11 @@ +2014-03-05 Pedro Alves + + PR gdb/16575 + * gdb.base/breakpoint-shadow.exp (compare_disassembly): New + procedure. + (top level): Adjust to use it. Add tests that exercise breakpoint + interaction with the code-cache. + 2014-02-26 Ludovic Courtès * gdb.guile/scm-value.exp (test_value_in_inferior): Add diff --git a/gdb/testsuite/gdb.base/breakpoint-shadow.exp b/gdb/testsuite/gdb.base/breakpoint-shadow.exp index 74f7c8f..1414ec0 100644 --- a/gdb/testsuite/gdb.base/breakpoint-shadow.exp +++ b/gdb/testsuite/gdb.base/breakpoint-shadow.exp @@ -44,14 +44,36 @@ gdb_test_multiple "disass main" $test { gdb_test "b [gdb_get_line_number "break-first"]" "Breakpoint \[0-9\] at .*" "First breakpoint placed" gdb_test "b [gdb_get_line_number "break-second"]" "Breakpoint \[0-9\] at .*" "Second breakpoint placed" -set test "disassembly with breakpoints" -gdb_test_multiple "disass main" $test { - -re $match { - set got $expect_out(1,string) - if [string equal -nocase $orig $got] { - pass $test - } else { - fail $test +# Disassemble main, and compare the output to the original output +# before breakpoints were inserted. TEST is used as test message. + +proc test_disassembly {test} { + global match orig + + gdb_test_multiple "disass main" $test { + -re $match { + set got $expect_out(1,string) + if [string equal -nocase $orig $got] { + pass $test + } else { + fail $test + } } } } + +test_disassembly "disassembly with breakpoints" + +# Now check the interaction between the code cache and breakpoint +# always-inserted mode. + +# Recreate the code cache when breakpoints are already inserted. +gdb_test_no_output "set code-cache off" +gdb_test_no_output "set code-cache on" + +test_disassembly "disassembly with breakpoints, fresh code cache" + +# Delete breakpoints. This should update the code cache as well. +delete_breakpoints + +test_disassembly "disassembly without breakpoints, no stale breakpoints" -- 2.7.4