From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf41.google.com (mail-qv1-xf41.google.com [IPv6:2607:f8b0:4864:20::f41]) by sourceware.org (Postfix) with ESMTPS id 179C3385483E for ; Mon, 14 Dec 2020 13:15:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 179C3385483E Received: by mail-qv1-xf41.google.com with SMTP id p12so7721636qvj.13 for ; Mon, 14 Dec 2020 05:15:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QHutZP/fxsTMMZdIug1SSXVFtbsOFCDLB0SPIDB3hHk=; b=iUa/6oLxY8AvMuaRBiDXdSK0CoAap6aDyf72cNXCSC7U5/nE39KI4yLtVnq5VyRxcA eO/tfhB4LR++Wvhxodgqyr4pUR14EWhp8vQgygXS3KggjO8/uT+patdaj6fsN63Gs+ol EzALCjZcr8zr75rqEQJMQGxb/voiGoJkERDiPuknSs4KqEPWw4sLT6NppS+k2uDwuUTi n7VfJ92opr2oZ7RDP8Pnzht0oU8QWTG1RNgTnN4g+WyyhemgvTd0GL+t1GK3unklpjoX d6C5cgYD3/dR8pYBWUuPA76+NsFbYw/xgdNQ4ShnG8fsLXl32iPrszEx90ZjPV1XUCtd qv8g== X-Gm-Message-State: AOAM531NmLQwNV+Hpa42ZF0uoMWf81Fh4OhQDAC/7OLYuqcHK/v6XgDG QTkg3l0xgN0kI5EWs1ExuD4YBYE2XFwrgw== X-Google-Smtp-Source: ABdhPJxDFw34B0ALnGo6+oj0Dkh92RD9Z1c1kTHGR+4nYd7stvLfNGOoZtbFfvNGUPUzWYhpvLgMkA== X-Received: by 2002:a05:6214:8c9:: with SMTP id da9mr16567380qvb.29.1607951712326; Mon, 14 Dec 2020 05:15:12 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:370e:1581:5197:9856:6728? ([2804:7f0:8284:370e:1581:5197:9856:6728]) by smtp.gmail.com with ESMTPSA id z78sm14125377qkb.0.2020.12.14.05.15.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 14 Dec 2020 05:15:11 -0800 (PST) Subject: Re: [PATCH] [AArch64] Fix TBI handling for watchpoints To: Alan Hayward Cc: "gdb-patches\\@sourceware.org" , nd References: <20201211160754.1641066-1-luis.machado@linaro.org> <60D92422-E8C3-497B-AC25-3E5C65191DFE@arm.com> From: Luis Machado Message-ID: Date: Mon, 14 Dec 2020 10:15:08 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <60D92422-E8C3-497B-AC25-3E5C65191DFE@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Dec 2020 13:15:14 -0000 On 12/14/20 9:52 AM, Alan Hayward wrote: > > >> On 11 Dec 2020, at 16:07, Luis Machado wrote: >> >> When inserting hw watchpoints, we take care of masking off the top byte >> of the address (and sign-extending it if needed). This guarantees we won't >> pass tagged addresses to the kernel via ptrace. >> >> However, from the kernel documentation on tagged pointers... >> >> "Non-zero tags are not preserved when delivering signals. This means that >> signal handlers in applications making use of tags cannot rely on the tag >> information for user virtual addresses being maintained for fields inside >> siginfo_t. >> >> One exception to this rule is for signals raised in response to watchpoint >> debug exceptions, where the tag information will be preserved." >> >> So the stopped data address after a hw watchpoint hit can be potentially >> tagged, and we don't handle this in GDB at the moment. This results in >> GDB missing a hw watchpoint hit and attempting to step over an unsteppable >> hw watchpoint, causing it to spin endlessly. >> >> The following patch fixes this by adjusting the stopped data address and adds >> some tests to expose the problem. >> >> gdb/ChangeLog: >> >> YYYY-MM-DD Luis Machado >> >> * aarch64-linux-nat.c >> (aarch64_linux_nat_target::stopped_data_address): Handle the TBI. >> >> gdbserver/ChangeLog: >> >> YYYY-MM-DD Luis Machado >> >> * linux-aarch64-low.cc (address_significant): New function. >> (aarch64_target::low_stopped_data_address): Handle the TBI. >> >> gdb/testsuite/ChangeLog: >> >> YYYY-MM-DD Luis Machado >> >> * gdb.arch/aarch64-tagged-pointer.c (main): Add a few more >> pointer-based memory accesses. >> * gdb.arch/aarch64-tagged-pointer.exp: Exercise additional >> hw watchpoint cases. >> --- >> gdb/aarch64-linux-nat.c | 8 ++++- >> .../gdb.arch/aarch64-tagged-pointer.c | 8 ++++- >> .../gdb.arch/aarch64-tagged-pointer.exp | 27 ++++++++++------ >> gdbserver/linux-aarch64-low.cc | 31 ++++++++++++++++++- >> 4 files changed, 61 insertions(+), 13 deletions(-) >> >> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c >> index 77d5863a56..b3bbde4b92 100644 >> --- a/gdb/aarch64-linux-nat.c >> +++ b/gdb/aarch64-linux-nat.c >> @@ -877,6 +877,13 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p) >> || (siginfo.si_code & 0xffff) != TRAP_HWBKPT) >> return false; >> >> + /* Make sure to ignore the top byte, otherwise we may not recognize a >> + hardware watchpoint hit. The stopped data addresses coming from the >> + kernel can potentially be tagged addresses. */ >> + struct gdbarch *gdbarch = thread_architecture (inferior_ptid); >> + const CORE_ADDR addr_trap >> + = address_significant (gdbarch, (CORE_ADDR) siginfo.si_addr); >> + >> /* Check if the address matches any watched address. */ >> state = aarch64_get_debug_reg_state (inferior_ptid.pid ()); >> for (i = aarch64_num_wp_regs - 1; i >= 0; --i) >> @@ -884,7 +891,6 @@ aarch64_linux_nat_target::stopped_data_address (CORE_ADDR *addr_p) >> const unsigned int offset >> = aarch64_watchpoint_offset (state->dr_ctrl_wp[i]); >> const unsigned int len = aarch64_watchpoint_length (state->dr_ctrl_wp[i]); >> - const CORE_ADDR addr_trap = (CORE_ADDR) siginfo.si_addr; >> const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset; >> const CORE_ADDR addr_watch_aligned = align_down (state->dr_addr_wp[i], 8); >> const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i]; >> diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c >> index 609d4f220e..658c3093e8 100644 >> --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c >> +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.c >> @@ -53,5 +53,11 @@ main (void) >> } >> >> sp1->i = 8765; >> - i = 1; >> + sp2->i = 4321; >> + sp1->i = 8765; >> + sp2->i = 4321; >> + *p1 = 1; >> + *p2 = 2; >> + *p1 = 1; >> + *p2 = 2; >> } >> diff --git a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp >> index 957571fdf9..01c2b577d5 100644 >> --- a/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp >> +++ b/gdb/testsuite/gdb.arch/aarch64-tagged-pointer.exp >> @@ -92,14 +92,21 @@ foreach_with_prefix bptype {"hbreak" "break"} { >> >> gdb_test "down" >> gdb_test "finish" >> -# Watch on tagged pointer. >> -gdb_test "watch *sp2" >> -gdb_test "continue" \ >> - "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \ >> - "run until watchpoint on s1" >> -delete_breakpoints >> >> -gdb_test "watch *p2" >> -gdb_test "continue" \ >> - "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \ >> - "run until watchpoint on i" >> +# sp1 and p1 are untagged pointers, but sp2 and p2 are tagged pointers. >> +# Cycle through all of them to make sure the following combinations work: >> +# >> +# hw watch on untagged address, hit on untagged address. >> +# hw watch on tagged address, hit on untagged address. >> +# hw watch on untagged address, hit on tagged address. >> +# hw watch on tagged address, hit on tagged address. >> +foreach symbol {"sp1" "sp2" "p1" "p2"} { >> + gdb_test "watch *${symbol}" >> + gdb_test "continue" \ >> + "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \ >> + "run until watchpoint on ${symbol}" >> + gdb_test "continue" \ >> + "Continuing\\..*Hardware watchpoint \[0-9\]+.*" \ >> + "run until watchpoint on ${symbol}, 2nd hit" >> + delete_breakpoints >> +} >> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc >> index 08208ae4f4..da599ef4bb 100644 >> --- a/gdbserver/linux-aarch64-low.cc >> +++ b/gdbserver/linux-aarch64-low.cc >> @@ -458,6 +458,30 @@ aarch64_target::low_remove_point (raw_bkpt_type type, CORE_ADDR addr, >> return ret; >> } >> >> +/* Return the address only having significant bits. This is used to ignore >> + the top byte (TBI). */ >> + >> +static CORE_ADDR >> +address_significant (CORE_ADDR addr) > > Ideally this would be in a common file. But it’s only used the once, so it’s > not worth the hassle of making it arch independent (plus then I’d be wondering > if it could get combined with the gdb version of the function). I considered unifying the implementation, but then it would've required a change to how this is implemented in gdbarch. Currently it is just the number of significant bits plus a generic implementation. We'd need to turn it into an arch-specific implementation instead of just relying on the number of bits, plus a generic default implementation. But that means we'd need to replicate the code for each architecture potentially. Hence the gdbarch-independent implementation in gdbserver. Not too pretty on both strategies. I picked this one. > > >> +{ >> + /* Clear insignificant bits of a target address and sign extend resulting >> + address, avoiding shifts larger or equal than the width of a CORE_ADDR. >> + The local variable ADDR_BIT stops the compiler reporting a shift overflow >> + when it won't occur. Skip updating of target address if current target >> + has not set gdbarch significant_addr_bit. */ > > This comment shouldn’t mention gdbarch or significant_addr_bit. > That's true. In fact, I trimmed most of it, as we have a fixed number of bits. It now reads: /* Clear insignificant bits of a target address and sign extend resulting address. */ >> + int addr_bit = 56; >> + int char_bit = CHAR_BIT; >> + >> + if (addr_bit && (addr_bit < (sizeof (CORE_ADDR) * char_bit))) > > This whole if statement is redundant - all values are fixed. Removed now. > > > Everything else looks good. Thanks. I'll push the updated version. > >> + { >> + CORE_ADDR sign = (CORE_ADDR) 1 << (addr_bit - 1); >> + addr &= ((CORE_ADDR) 1 << addr_bit) - 1; >> + addr = (addr ^ sign) - sign; >> + } >> + >> + return addr; >> +} >> + >> /* Implementation of linux target ops method "low_stopped_data_address". */ >> >> CORE_ADDR >> @@ -478,6 +502,12 @@ aarch64_target::low_stopped_data_address () >> || (siginfo.si_code & 0xffff) != 0x0004 /* TRAP_HWBKPT */) >> return (CORE_ADDR) 0; >> >> + /* Make sure to ignore the top byte, otherwise we may not recognize a >> + hardware watchpoint hit. The stopped data addresses coming from the >> + kernel can potentially be tagged addresses. */ >> + const CORE_ADDR addr_trap >> + = address_significant ((CORE_ADDR) siginfo.si_addr); >> + >> /* Check if the address matches any watched address. */ >> state = aarch64_get_debug_reg_state (pid_of (current_thread)); >> for (i = aarch64_num_wp_regs - 1; i >= 0; --i) >> @@ -485,7 +515,6 @@ aarch64_target::low_stopped_data_address () >> const unsigned int offset >> = aarch64_watchpoint_offset (state->dr_ctrl_wp[i]); >> const unsigned int len = aarch64_watchpoint_length (state->dr_ctrl_wp[i]); >> - const CORE_ADDR addr_trap = (CORE_ADDR) siginfo.si_addr; >> const CORE_ADDR addr_watch = state->dr_addr_wp[i] + offset; >> const CORE_ADDR addr_watch_aligned = align_down (state->dr_addr_wp[i], 8); >> const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i]; >> -- >> 2.25.1 >> >