From patchwork Mon Apr 22 13:51:49 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 88870 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C9360385840F for ; Mon, 22 Apr 2024 15:58:42 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-oo1-xc2f.google.com (mail-oo1-xc2f.google.com [IPv6:2607:f8b0:4864:20::c2f]) by sourceware.org (Postfix) with ESMTPS id 77FA53858D35 for ; Mon, 22 Apr 2024 15:58:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 77FA53858D35 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 77FA53858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::c2f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713801493; cv=none; b=W4JWGRB/fXxSuU4I0wnqGer+3iPBES6rawQ6hAfBrf9pVU0GHfJvM47BAzt3ZAUQIqG8lP53oJDaBUdvUoJEL4s86lBrElSFsqqJ/o1syK423URwK0QWKCnBbAxPqcaQDEY+ED5Mcmc09mnTiCylUAW3WT6kyOvGyPSjULlTXRI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1713801493; c=relaxed/simple; bh=O7UaiwYzHlHyGcX6nUVJeLpSMrQRlXLUbixRFDdDFO8=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=CMRUSNUiE6N1h2qU3xOxTjBp6z3aujgvu3E33pY3Hd1edbZguqnMNWhpERWRILuTjcBy5mY8gys6SPeU3AGBuvkjLNf4E9WgxjzwubgeFjmqKEl+fwxnXWT2ayRB9w7O4bCiZuAKgP73ilFG1vUupRl1xLqFRNLAOyLg4NGuS5M= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-oo1-xc2f.google.com with SMTP id 006d021491bc7-5aa17c29ba0so2700315eaf.3 for ; Mon, 22 Apr 2024 08:58:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1713801489; x=1714406289; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=dfUl3SBqa1uEoFVkOAqnHOplTe3h5D7GQowgB3Vt1MU=; b=HNzkZbLLyOlP6FqpbHAg6zSXbhXKjdMZW7wJhnPucELfOTNb9sNyHp3RKiLVl+Vbiq wl961LLiPmvujjLUqd3kOzbE1w9xmw3Z38uZvOh2T1PthrTyP4yhEbmKQqdB0qRONSqZ f3i0iSk5UwcdVtyx4Q59/G9/p4mHwbIoX8EOBpsrXLcmaoQ+0QT5imrT29U8kZrebill pDdZayCHkLkNOdcSy/UC7qWnkS3bR8NOgduE185S2VpTWD0uFnz0fDP5kfLiLLTp/UtY Xc8/zTzhDoBpshIINSBMOlEhWc0DCMwAQyive1TnOMaJxE0Yj9BGwPr2hbKOwS6Y0gXn eKNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713801489; x=1714406289; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=dfUl3SBqa1uEoFVkOAqnHOplTe3h5D7GQowgB3Vt1MU=; b=enn9ll+VH6lnrsVLx1qZQ/2Hc+voZaDU/iFmbjBMcZi1uvzJIbxrT7e3OFqXBrmnsS ccEy6cRL3K2wP1z8NDHIpYkXKCZFZyjrphM8t+D4zyRKmKi2n7PKh/F1fFX6Nin4lFhJ PEgVnsSAZ/9Iy+L4LxidR4ndqi2ooLloGKNQGoPaZr1wV06mBSsf+OLGJliCAVCTKyQd wtw9djayj/3mEeKyDntdXnrxCGni9ixftj/dkcfNwctLXMY4SGys1MXAh9nCh2CufydS BlZuSpFSLiS8f5ruRjMXsIgti6pxKkMRYQXoc+y1O/ec1/ih8bMGCV9NeWW5llsMi2Lg Je4g== X-Gm-Message-State: AOJu0Yx5eznILQohLaxskzm6JbZWqk3EXG6L1CCCknFlU1o4Jrz2QC/6 /cHohZGlcONCBqsUMO52nZaUBr4LfaK1P8btAFNVjdvthacRkMIhcyuaG4GUwDfeWC9HIWYScHI = X-Google-Smtp-Source: AGHT+IG4BHOMNfysbEDSd86Y8aH4CEh/enQeoLF1zX5gKYjq+vr+3SJdRnP7BobrAVAyjPHaiZoV5g== X-Received: by 2002:a5d:9844:0:b0:7da:a742:e4fb with SMTP id p4-20020a5d9844000000b007daa742e4fbmr7229016ios.17.1713793916963; Mon, 22 Apr 2024 06:51:56 -0700 (PDT) Received: from localhost.localdomain (97-122-86-252.hlrn.qwest.net. [97.122.86.252]) by smtp.gmail.com with ESMTPSA id k19-20020a056638371300b00482eaf98439sm2840773jav.33.2024.04.22.06.51.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Apr 2024 06:51:56 -0700 (PDT) From: Tom Tromey To: gdb-patches@sourceware.org Cc: Tom Tromey Subject: [PATCH] Fix regression on aarch64-linux gdbserver Date: Mon, 22 Apr 2024 07:51:49 -0600 Message-ID: <20240422135149.921896-1-tromey@adacore.com> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Commit 9a03f218 ("Fix gdb.base/watchpoint-unaligned.exp on aarch64") fixed a watchpoint bug in gdb -- but did not touch the corresponding code in gdbserver. This patch moves the gdb code into gdb/nat, so that it can be shared with gdbserver, and then changes gdbserver to use it, fixing the bug. This is yet another case where having a single back end would prevent bugs. I tested this using the AdaCore internal gdb testsuite. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29423 --- gdb/aarch64-nat.c | 115 --------------------------------- gdb/aarch64-nat.h | 8 --- gdb/nat/aarch64-hw-point.c | 115 +++++++++++++++++++++++++++++++++ gdb/nat/aarch64-hw-point.h | 8 +++ gdbserver/linux-aarch64-low.cc | 38 +---------- 5 files changed, 126 insertions(+), 158 deletions(-) diff --git a/gdb/aarch64-nat.c b/gdb/aarch64-nat.c index 894fb73095b..1ba9c4c19a7 100644 --- a/gdb/aarch64-nat.c +++ b/gdb/aarch64-nat.c @@ -224,121 +224,6 @@ aarch64_remove_watchpoint (CORE_ADDR addr, int len, enum target_hw_bp_type type, return ret; } -/* See aarch64-nat.h. */ - -bool -aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state, - CORE_ADDR addr_trap, CORE_ADDR *addr_p) -{ - bool found = false; - for (int phase = 0; phase <= 1; ++phase) - for (int i = aarch64_num_wp_regs - 1; i >= 0; --i) - { - if (!(state->dr_ref_count_wp[i] - && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]))) - { - /* Watchpoint disabled. */ - continue; - } - - const enum target_hw_bp_type type - = aarch64_watchpoint_type (state->dr_ctrl_wp[i]); - if (type == hw_execute) - { - /* Watchpoint disabled. */ - continue; - } - - if (phase == 0) - { - /* Phase 0: No hw_write. */ - if (type == hw_write) - continue; - } - else - { - /* Phase 1: Only hw_write. */ - if (type != hw_write) - continue; - } - - 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_watch = state->dr_addr_wp[i] + offset; - const CORE_ADDR addr_watch_aligned - = align_down (state->dr_addr_wp[i], AARCH64_HWP_MAX_LEN_PER_REG); - const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i]; - - /* ADDR_TRAP reports the first address of the memory range - accessed by the CPU, regardless of what was the memory - range watched. Thus, a large CPU access that straddles - the ADDR_WATCH..ADDR_WATCH+LEN range may result in an - ADDR_TRAP that is lower than the - ADDR_WATCH..ADDR_WATCH+LEN range. E.g.: - - addr: | 4 | 5 | 6 | 7 | 8 | - |---- range watched ----| - |----------- range accessed ------------| - - In this case, ADDR_TRAP will be 4. - - The access size also can be larger than that of the watchpoint - itself. For instance, the access size of an stp instruction is 16. - So, if we use stp to store to address p, and set a watchpoint on - address p + 8, the reported ADDR_TRAP can be p + 8 (observed on - RK3399 SOC). But it also can be p (observed on M1 SOC). Checking - for this situation introduces the possibility of false positives, - so we only do this for hw_write watchpoints. */ - const CORE_ADDR max_access_size = type == hw_write ? 16 : 8; - const CORE_ADDR addr_watch_base = addr_watch_aligned - - (max_access_size - AARCH64_HWP_MAX_LEN_PER_REG); - if (!(addr_trap >= addr_watch_base - && addr_trap < addr_watch + len)) - { - /* Not a match. */ - continue; - } - - /* To match a watchpoint known to GDB core, we must never - report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN - range. ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false - positive on kernels older than 4.10. See PR - external/20207. */ - if (addr_p != nullptr) - *addr_p = addr_orig; - - if (phase == 0) - { - /* Phase 0: Return first match. */ - return true; - } - - /* Phase 1. */ - if (addr_p == nullptr) - { - /* First match, and we don't need to report an address. No need - to look for other matches. */ - return true; - } - - if (!found) - { - /* First match, and we need to report an address. Look for other - matches. */ - found = true; - continue; - } - - /* More than one match, and we need to return an address. No need to - look for further matches. */ - return false; - } - - return found; -} - /* Define AArch64 maintenance commands. */ static void diff --git a/gdb/aarch64-nat.h b/gdb/aarch64-nat.h index 947d2805602..ec85524b2d4 100644 --- a/gdb/aarch64-nat.h +++ b/gdb/aarch64-nat.h @@ -45,14 +45,6 @@ struct aarch64_debug_reg_state *aarch64_get_debug_reg_state (pid_t pid); void aarch64_remove_debug_reg_state (pid_t pid); -/* Helper for the "stopped_data_address" target method. Returns TRUE - if a hardware watchpoint trap at ADDR_TRAP matches a set - watchpoint. The address of the matched watchpoint is returned in - *ADDR_P. */ - -bool aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state, - CORE_ADDR addr_trap, CORE_ADDR *addr_p); - /* Helper functions used by aarch64_nat_target below. See their definitions. */ diff --git a/gdb/nat/aarch64-hw-point.c b/gdb/nat/aarch64-hw-point.c index b62c4627d96..6acee0fb814 100644 --- a/gdb/nat/aarch64-hw-point.c +++ b/gdb/nat/aarch64-hw-point.c @@ -645,3 +645,118 @@ aarch64_region_ok_for_watchpoint (CORE_ADDR addr, int len) the checking is costly. */ return 1; } + +/* See nat/aarch64-hw-point.h. */ + +bool +aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state, + CORE_ADDR addr_trap, CORE_ADDR *addr_p) +{ + bool found = false; + for (int phase = 0; phase <= 1; ++phase) + for (int i = aarch64_num_wp_regs - 1; i >= 0; --i) + { + if (!(state->dr_ref_count_wp[i] + && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]))) + { + /* Watchpoint disabled. */ + continue; + } + + const enum target_hw_bp_type type + = aarch64_watchpoint_type (state->dr_ctrl_wp[i]); + if (type == hw_execute) + { + /* Watchpoint disabled. */ + continue; + } + + if (phase == 0) + { + /* Phase 0: No hw_write. */ + if (type == hw_write) + continue; + } + else + { + /* Phase 1: Only hw_write. */ + if (type != hw_write) + continue; + } + + 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_watch = state->dr_addr_wp[i] + offset; + const CORE_ADDR addr_watch_aligned + = align_down (state->dr_addr_wp[i], AARCH64_HWP_MAX_LEN_PER_REG); + const CORE_ADDR addr_orig = state->dr_addr_orig_wp[i]; + + /* ADDR_TRAP reports the first address of the memory range + accessed by the CPU, regardless of what was the memory + range watched. Thus, a large CPU access that straddles + the ADDR_WATCH..ADDR_WATCH+LEN range may result in an + ADDR_TRAP that is lower than the + ADDR_WATCH..ADDR_WATCH+LEN range. E.g.: + + addr: | 4 | 5 | 6 | 7 | 8 | + |---- range watched ----| + |----------- range accessed ------------| + + In this case, ADDR_TRAP will be 4. + + The access size also can be larger than that of the watchpoint + itself. For instance, the access size of an stp instruction is 16. + So, if we use stp to store to address p, and set a watchpoint on + address p + 8, the reported ADDR_TRAP can be p + 8 (observed on + RK3399 SOC). But it also can be p (observed on M1 SOC). Checking + for this situation introduces the possibility of false positives, + so we only do this for hw_write watchpoints. */ + const CORE_ADDR max_access_size = type == hw_write ? 16 : 8; + const CORE_ADDR addr_watch_base = addr_watch_aligned - + (max_access_size - AARCH64_HWP_MAX_LEN_PER_REG); + if (!(addr_trap >= addr_watch_base + && addr_trap < addr_watch + len)) + { + /* Not a match. */ + continue; + } + + /* To match a watchpoint known to GDB core, we must never + report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN + range. ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false + positive on kernels older than 4.10. See PR + external/20207. */ + if (addr_p != nullptr) + *addr_p = addr_orig; + + if (phase == 0) + { + /* Phase 0: Return first match. */ + return true; + } + + /* Phase 1. */ + if (addr_p == nullptr) + { + /* First match, and we don't need to report an address. No need + to look for other matches. */ + return true; + } + + if (!found) + { + /* First match, and we need to report an address. Look for other + matches. */ + found = true; + continue; + } + + /* More than one match, and we need to return an address. No need to + look for further matches. */ + return false; + } + + return found; +} diff --git a/gdb/nat/aarch64-hw-point.h b/gdb/nat/aarch64-hw-point.h index bdcca932e57..0d50eaab0de 100644 --- a/gdb/nat/aarch64-hw-point.h +++ b/gdb/nat/aarch64-hw-point.h @@ -110,6 +110,14 @@ unsigned int aarch64_watchpoint_offset (unsigned int ctrl); unsigned int aarch64_watchpoint_length (unsigned int ctrl); enum target_hw_bp_type aarch64_watchpoint_type (unsigned int ctrl); +/* Helper for the "stopped_data_address" target method. Returns TRUE + if a hardware watchpoint trap at ADDR_TRAP matches a set + watchpoint. The address of the matched watchpoint is returned in + *ADDR_P. */ + +bool aarch64_stopped_data_address (const struct aarch64_debug_reg_state *state, + CORE_ADDR addr_trap, CORE_ADDR *addr_p); + int aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr, int len, int is_insert, ptid_t ptid, struct aarch64_debug_reg_state *state); diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc index 5df67fccd08..da5c1fd0629 100644 --- a/gdbserver/linux-aarch64-low.cc +++ b/gdbserver/linux-aarch64-low.cc @@ -576,41 +576,9 @@ aarch64_target::low_stopped_data_address () /* 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) - { - 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_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]; - - if (state->dr_ref_count_wp[i] - && DR_CONTROL_ENABLED (state->dr_ctrl_wp[i]) - && addr_trap >= addr_watch_aligned - && addr_trap < addr_watch + len) - { - /* ADDR_TRAP reports the first address of the memory range - accessed by the CPU, regardless of what was the memory - range watched. Thus, a large CPU access that straddles - the ADDR_WATCH..ADDR_WATCH+LEN range may result in an - ADDR_TRAP that is lower than the - ADDR_WATCH..ADDR_WATCH+LEN range. E.g.: - - addr: | 4 | 5 | 6 | 7 | 8 | - |---- range watched ----| - |----------- range accessed ------------| - - In this case, ADDR_TRAP will be 4. - - To match a watchpoint known to GDB core, we must never - report *ADDR_P outside of any ADDR_WATCH..ADDR_WATCH+LEN - range. ADDR_WATCH <= ADDR_TRAP < ADDR_ORIG is a false - positive on kernels older than 4.10. See PR - external/20207. */ - return addr_orig; - } - } + CORE_ADDR result; + if (aarch64_stopped_data_address (state, addr_trap, &result)) + return result; return (CORE_ADDR) 0; }