From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from progateway7-pub.mail.pro1.eigbox.com (gproxy5-pub.mail.unifiedlayer.com [67.222.38.55]) by sourceware.org (Postfix) with ESMTPS id 5AD9B3858D35 for ; Fri, 16 Jun 2023 18:14:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5AD9B3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com Received: from cmgw10.mail.unifiedlayer.com (unknown [10.0.90.125]) by progateway7.mail.pro1.eigbox.com (Postfix) with ESMTP id B4A3C10042F97 for ; Fri, 16 Jun 2023 18:14:38 +0000 (UTC) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTP id ADy2qbdzLBlkKADy2qXm39; Fri, 16 Jun 2023 18:14:38 +0000 X-Authority-Reason: nr=8 X-Authority-Analysis: v=2.4 cv=W7r96Tak c=1 sm=1 tr=0 ts=648ca68e a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=of4jigFt-DYA:10:nop_rcvd_month_year a=Qbun_eYptAEA:10:endurance_base64_authed_username_1 a=CCpqsmhAAAAA:8 a=jfWE5ty9Nh8cqv5FW7AA:9 a=p0kisfBXZy0A:10:demote_hacked_domain_1 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date:References :Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=XphHeh8EPNbv/74ZOtyBsWtCyMWtqSCpLYhT7aPp3sE=; b=J0hh5Wbfk3peikVXSr7KbW3mNL dNeXVviwuG1L+3YiY/STWtAXv/W441dtz0099PKnaOyqErMbVbwdmwFD33IRY7HATo5UJnwxmI+9s EBFgDQyhd/19cDS3tzNlZnl1X; Received: from 75-166-136-83.hlrn.qwest.net ([75.166.136.83]:40234 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qADy2-001Ghp-BL; Fri, 16 Jun 2023 12:14:38 -0600 From: Tom Tromey To: Simon Farre via Gdb-patches Cc: Simon Farre Subject: Re: [PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints References: <20230615182845.97700-1-simon.farre.cx@gmail.com> X-Attribution: Tom Date: Fri, 16 Jun 2023 12:14:37 -0600 In-Reply-To: <20230615182845.97700-1-simon.farre.cx@gmail.com> (Simon Farre via Gdb-patches's message of "Thu, 15 Jun 2023 20:28:45 +0200") Message-ID: <87bkhf46lu.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 75.166.136.83 X-Source-L: No X-Exim-ID: 1qADy2-001Ghp-BL X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 75-166-136-83.hlrn.qwest.net (murgatroyd) [75.166.136.83]:40234 X-Source-Auth: tom+tromey.com X-Email-Count: 1 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-Spam-Status: No, score=-3019.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: >>>>> "Simon" == Simon Farre via Gdb-patches writes: Simon> v2. Simon> Added tests. Thanks for the patch. You probably should rebase this one on top of https://sourceware.org/pipermail/gdb-patches/2023-June/200252.html since that changes the breakpoint creation & reuse logic. I suspect it would simplify your patch. Simon> +from .startup import send_gdb_with_response, in_gdb_thread, log Simon> +from .varref import find_variable Simon> +from .frames import frame_for_id, ScopedFrameOperation Simon> +class DataBreakpoint: Make sure to run 'black', I suspect it wants blank lines in here. I just do: murgatroyd. black gdb/python/lib/gdb/ Simon> + def __init__(self, expr, type, condition=None, hit_condition=None, frameId = None): With the aforementioned series, this would probably be replaced by one of the transformer functions, which would then be type-checked. Simon> +watchpoint_map: Mapping[str, DataBreakpoint] = {} This can/should be unified with the existing breakpoint_map somehow. Simon> + # FIXME this does not matter; GDB can translate its understanding Simon> + # of breakpoint locations into individual breakpoints and Simon> + # even though GDB can't delete locs, it can disable them. I think it would be great to discuss this but it seems unrelated to your patch. Maybe we should either discuss on the github issue or open a new gdb 'dap' bug for it? Simon> + # We've resolved the "path" for the variable using varrefs. Construct it Simon> + # for example: baz being a member variable of bar, which is a member of Simon> + # foo: foo.bar.baz Simon> + dataId = ".".join(reversed(path)) Yeah, I'm not totally sure how this part should work. The issue I see is that variable references use python pretty-printers under the hood, but these work in a funny/limited way -- they were intended just for printing -- and so maybe won't interact well with watchpoints. For one thing, nothing requires that the names returned by a printer be meaningful. That is, they don't necessarily correspond to expressions that can actually access the member, and in practice do not (i.e., many C++ printers use readable labels but not usable ones, if that makes sense). I had two ideas for how to handle this situation. One is to keep using pretty printers, and just iterate over the children looking for one that matches the given name. Then, if that printer returns a gdb.Value for the child, watch that value. This would end up working like "watch -location". If 'children' returns a string or whatever, it would just be unwatchable. The other idea is to invert the pretty-printer relationship. Right now DAP will use a no-op printer for printer-less values. Instead, DAP could make its own objects for printer-less values and have a wrapper for pretty-printers. Then watching ordinary values could work by remembering the expression and block, and watching pretty-printed values could simply fail (or could fall back to the gdb.Value idea above). The reason to consider this second plan is that watching a Value might require extra work or will probably just do the wrong thing for local variables. (Though I am not sure watchpoints work well with DWARF location lists anyway.) Simon> + # Verify that symbol is in that frame Simon> + if frameId is not None: Simon> + symbol_name = path[-1] Simon> + with ScopedFrameOperation(frameId): Simon> + (symbol, implicit_this) = gdb.lookup_symbol(symbol_name) Simon> + if symbol is None: Simon> + dataId = None Simon> + description = f"Symbol {name} not found in this frame" My understanding of the spec is that there are two cases: 1. variable reference supplied -> frame ID is ignored 2. no variable reference supplied -> the type of evaluation depends on the presence of the frame ID So there's no need for this verification, I think. However, gdb doesn't currently provide a way to implement #2, as watchpoint expression evaluation doesn't have a way to pass PARSER_LEAVE_BLOCK_ALONE to the parser. This could be added, it's probably a bit of a pain but mostly mechanical work. Tom