From patchwork Wed May 21 09:45:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 1046 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id C459836001D for ; Wed, 21 May 2014 02:45:32 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14314964) id 681F341BD4A52; Wed, 21 May 2014 02:45:32 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 3D3E441B84A55 for ; Wed, 21 May 2014 02:45:32 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=kfgVh7gDiHsRfK8B SfjTDd6KhzJfVLWoWsBpTgqnT2rUGPa5AXv0ddpq/+fkJHdsuftWTiEocp0JBwUV wN9/k2NqIbb0wphIqfA7Dv92JMpdFv5TvFCiuugPjBIVHnY/CEl9tUbOuAIKMO7l IFn/NZWVRJZ4RWk9Nj019TWQkfw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=pFid5FItzqI9eGZaIww/WJ MFRpE=; b=ksFdLuVu7TGvP0SnV/POdot15aGhbbCUFZgbOoAxg6AGhkW6IgexCW NF10fMvXNMmWwhzwhxwh2uTj/yognEg8BzSWmDWYZ/r8OB5m0asR/e+uYJj1ovXv sl6w7oTJ75+vJXsDVmpbI21rbr6lLVPW4Pa3GZwpMHzbBQ/uInbTw= Received: (qmail 31337 invoked by alias); 21 May 2014 09:45:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 31327 invoked by uid 89); 21 May 2014 09:45:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 May 2014 09:45:27 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4L9jOWP026191 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 21 May 2014 05:45:24 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s4L9jMPF007123; Wed, 21 May 2014 05:45:23 -0400 Message-ID: <537C75B2.9060504@redhat.com> Date: Wed, 21 May 2014 10:45:22 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: "Metzger, Markus T" CC: "gdb-patches@sourceware.org" Subject: [PATCH v2] Make the dcache (code/stack cache) handle line reading errors better References: <1396601586-24380-1-git-send-email-markus.t.metzger@intel.com> <53760BDF.2080500@redhat.com> <537A7A80.3050801@redhat.com> <537B389C.4080901@redhat.com> <537B8DA6.4080601@redhat.com> In-Reply-To: X-DH-Original-To: gdb@patchwork.siddhesh.in On 05/21/2014 07:51 AM, Metzger, Markus T wrote: >> -----Original Message----- >> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- >> owner@sourceware.org] On Behalf Of Pedro Alves >> Sent: Tuesday, May 20, 2014 7:15 PM > >> 8<--------------- >> From: Pedro Alves >> Date: Tue, 20 May 2014 12:26:01 +0100 >> Subject: [PATCH] Make the dcache (code/stack cache) handle line reading >> errors >> better. >> >> The dcache (code/stack cache) is supposed to be transparent, but it's >> actually not in one case. dcache tries to read chunks (cache lines) >> at a time off of the target. This may end up trying to read >> unaccessible or unavailable memory. Currently the caller an xfer error > > 'gets' is missing. Thanks, fixed. I've fixed a couple typos in the test .c file as well. > >> in this case. But if the specific bits of memory the caller actually >> wanted are available and accessible, then the caller should get the >> memory it wanted, not an error. > > >> + Copyright 2012-2014 Free Software Foundation, Inc. > > Why 2012-? Because the file was originally copied from gdb.base/find-unmapped.c and it still retains bits under 2012-2014 copyright. > > >> +void *first_mapped_page; >> +void *first_unmapped_page; >> +void *last_mapped_page; >> +void *last_unmapped_page; > > The _unmapped_page variants seem not to be used. Ah, they were needed in an earlier version. Thanks, removed. > > >> + /* Disassembling 0s should behave on all targets. */ >> + memset (p, 0, pg_count * pg_size); > > Shouldn't be necessary. The pages are supposed to be zero-initialized. Indeed, removed. (find-unmapped.c also has it) > > >> +# Test that dcache behaves correctly when reading a cache line fails. >> + >> +standard_testfile >> + >> +if { [prepare_for_testing "failed to prepare" ${testfile}] } { > > And I always thought you had to supply $srcfile, as well. Can > prepare_for_testing derive the source name from the executable name > and some path magic? Yeah: # Prepares for testing, by calling build_executable, and then clean_restart. # Please refer to build_executable for parameter description. proc prepare_for_testing { testname executable {sources ""} {options {debug}}} { if {[build_executable $testname $executable $sources $options] == -1} { return -1 } {sources ""} is how one specifies optional arguments. "" is the default in this case. And then: # Build executable named EXECUTABLE, from SOURCES. If SOURCES are not # provided, uses $EXECUTABLE.c. The TESTNAME paramer is the name of test # to pass to untested, if something is wrong. OPTIONS are passed # to gdb_compile directly. proc build_executable { testname executable {sources ""} {options {debug}} } { if {[llength $sources]==0} { set sources ${executable}.c } Hmm, for standard_testfile tests, looks like we could even make $testname and $executable be optional arguments. > > >> +proc disassemble { what } { >> + global hex gdb_prompt >> + >> + set cmd "disassemble $what" >> + gdb_test_multiple $cmd $cmd { >> + -re "Cannot access memory.*$gdb_prompt $" { >> + fail $cmd >> + } >> + -re "End of assembler dump\.\r\n$gdb_prompt $" { >> + pass $cmd >> + } > > Wouldn't simply checking for "End of assembler dump\.\r\n" suffice? Yeah. I was just being explicit, but indeed there's no need. I've now switched to just use gdb_test. > Wouldn't it even be more robust as it would catch all kinds of errors? Nope, gdb_test_multiple issues a FAIL if none of the passed in patterns match. And it wouldn't normally timeout internally gdb_test_multiple's issues a FAIL if it sees: -re "\r\n$gdb_prompt $" { ... fail "$message" ... } Below's v2. Let me know how it looks. 8<---------------- >From f433bc8ad604afa15421c10a7387503257eb9754 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 20 May 2014 12:26:01 +0100 Subject: [PATCH] Make the dcache (code/stack cache) handle line reading errors better The dcache (code/stack cache) is supposed to be transparent, but it's actually not in one case. dcache tries to read chunks (cache lines) at a time off of the target. This may end up trying to read unaccessible or unavailable memory. Currently the caller gets an xfer error in this case. But if the specific bits of memory the caller actually wanted are available and accessible, then the caller should get the memory it wanted, not an error. gdb/ 2014-05-21 Pedro Alves * dcache.c (dcache_read_memory_partial): If reading the cache line fails, fallback to reading just the memory the caller wanted. gdb/testsuite/ 2014-05-21 Pedro Alves * gdb.base/dcache-line-read-error.c: New. * gdb.base/dcache-line-read-error.exp: New. --- gdb/dcache.c | 7 +- gdb/testsuite/gdb.base/dcache-line-read-error.c | 107 ++++++++++++++++++++++ gdb/testsuite/gdb.base/dcache-line-read-error.exp | 68 ++++++++++++++ 3 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.c create mode 100644 gdb/testsuite/gdb.base/dcache-line-read-error.exp diff --git a/gdb/dcache.c b/gdb/dcache.c index d3b546b..9780f4d 100644 --- a/gdb/dcache.c +++ b/gdb/dcache.c @@ -497,8 +497,11 @@ dcache_read_memory_partial (struct target_ops *ops, DCACHE *dcache, if (i == 0) { - /* FIXME: We lose the real error status. */ - return TARGET_XFER_E_IO; + /* Even though reading the whole line failed, we may be able to + read a piece starting where the caller wanted. */ + return ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL, + myaddr, NULL, memaddr, len, + xfered_len); } else { diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.c b/gdb/testsuite/gdb.base/dcache-line-read-error.c new file mode 100644 index 0000000..4120593 --- /dev/null +++ b/gdb/testsuite/gdb.base/dcache-line-read-error.c @@ -0,0 +1,107 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2012-2014 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +#include +#include +#include +#include +#include + +size_t pg_size; +void *first_mapped_page; +void *last_mapped_page; + +void +breakpt (void) +{ + /* Nothing. */ +} + +int +main (void) +{ + void *p; + int pg_count; + size_t i; + + /* Map 6 contiguous pages, and then unmap all second first, and + second last. + + From GDB we will disassemble each of the _mapped_ pages, with a + code-cache (dcache) line size bigger than the page size (twice + bigger). This makes GDB try to read one page before the mapped + page once, and the page after another time. GDB should give no + error in either case. + + That is, depending on where the kernel aligns the pages, we get + either: + + .---.---.---.---.---.---. + | U | M | U | U | M | U | + '---'---'---'---'---'---. + | | | | <- line alignment + ^^^^^^^ ^^^^^^^ + | | + + line1 + line2 + + Or: + + .---.---.---.---.---.---. + | U | M | U | U | M | U | + '---'---'---'---'---'---. + | | | <- line alignment + ^^^^^^^ ^^^^^^^ + | | + line1 + + line2 + + Note we really want to test that dcache behaves correctly when + reading a cache line fails. We're just using unmapped memory as + proxy for any kind of error. */ + + pg_size = getpagesize (); + pg_count = 6; + + p = mmap (0, pg_count * pg_size, PROT_READ|PROT_WRITE, + MAP_ANONYMOUS|MAP_PRIVATE, -1, 0); + if (p == MAP_FAILED) + { + perror ("mmap"); + return EXIT_FAILURE; + } + + /* Leave memory zero-initialized. Disassembling 0s should behave on + all targets. */ + + for (i = 0; i < pg_count; i++) + { + if (i == 1 || i == 4) + continue; + + if (munmap (p + (i * pg_size), pg_size) == -1) + { + perror ("munmap"); + return EXIT_FAILURE; + } + } + + first_mapped_page = p + 1 * pg_size;; + last_mapped_page = p + 4 * pg_size; + + breakpt (); + + return EXIT_SUCCESS; +} diff --git a/gdb/testsuite/gdb.base/dcache-line-read-error.exp b/gdb/testsuite/gdb.base/dcache-line-read-error.exp new file mode 100644 index 0000000..7e75460 --- /dev/null +++ b/gdb/testsuite/gdb.base/dcache-line-read-error.exp @@ -0,0 +1,68 @@ +# Copyright 2014 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +# Test that dcache behaves correctly when reading a cache line fails. + +standard_testfile + +if { [prepare_for_testing "failed to prepare" ${testfile}] } { + return -1 +} + +if ![runto breakpt] { + return -1 +} + +# Issue the "delete mem" command. This makes GDB ignore the +# target-provided list, if any. + +proc delete_mem {} { + global gdb_prompt + + set test "delete mem" + gdb_test_multiple $test $test { + -re "Delete all memory regions.*y or n.*$" { + send_gdb "y\n" + exp_continue + } + -re "$gdb_prompt $" { + pass $test + } + } +} + +# Make the dcache line size bigger than the pagesize. +set pagesize [get_integer_valueof "pg_size" -1] +set linesize [expr $pagesize * 2] + +gdb_test_no_output "set dcache line-size $linesize" \ + "set dcache line size to twice the pagesize" + +gdb_test "info dcache" \ + "Dcache 4096 lines of $linesize bytes each.\r\nNo data cache available." + +# Make sure dcache doesn't automatically skip unmapped regions. +delete_mem + +gdb_test "info mem" \ + "Using user-defined memory regions.\r\nThere are no memory regions defined\." + +# Given the line size is bigger than the page size, we have +# alternating mapped and unmapped pages, these make dcache fail to +# fill in the cache line. GDB used to have a bug where that failure +# would end up as user-visible error. The range being disassembled is +# wholly available, so GDB should succeed. +gdb_test "disassemble first_mapped_page, +10" "End of assembler dump\." +gdb_test "disassemble last_mapped_page, +10" "End of assembler dump\."