From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from omta040.useast.a.cloudfilter.net (omta040.useast.a.cloudfilter.net [44.202.169.39]) by sourceware.org (Postfix) with ESMTPS id 1A0E03858C50 for ; Tue, 12 Sep 2023 17:47:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1A0E03858C50 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 eig-obgw-6009a.ext.cloudfilter.net ([10.0.30.184]) by cmsmtp with ESMTP id g4HYqDnQFyYOwg7TcqUWWs; Tue, 12 Sep 2023 17:47:04 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id g7TbqGhY64G7Bg7Tcq32zM; Tue, 12 Sep 2023 17:47:04 +0000 X-Authority-Analysis: v=2.4 cv=UMrOoQTy c=1 sm=1 tr=0 ts=6500a418 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=OWjo9vPv0XrRhIrVQ50Ab3nP57M=:19 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=zNV7Rl7Rt7sA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=YH6ZMpt32G5dxNsixRkA:9 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=NcYaTmWGNs5Y1zy8fVDVKXVVrrB06Ih1RWPWr8/2E2Y=; b=hJKcvdPbcwwwHAONRsx5xfZzbY 51q7vMPmzcPlM9XpJUpTtCJITuE5AzKuhOgL0HrihZYWXuFhi5Prme50QjRAnLqXR9hFZHnylVkz4 g/Wz5VwHNig659a4F1fjnmdVg; Received: from 71-211-130-31.hlrn.qwest.net ([71.211.130.31]:46208 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1qg7Tb-003usr-1j; Tue, 12 Sep 2023 11:47:03 -0600 From: Tom Tromey To: Simon Farre via Gdb-patches Cc: Simon Farre , tom@tromey.com Subject: Re: [PATCH v4] gdb/dap - dataBreakpointInfo & setDataBreakpoints References: <20230620093222.210781-1-simon.farre.cx@gmail.com> X-Attribution: Tom Date: Tue, 12 Sep 2023 11:47:02 -0600 In-Reply-To: <20230620093222.210781-1-simon.farre.cx@gmail.com> (Simon Farre via Gdb-patches's message of "Tue, 20 Jun 2023 11:32:22 +0200") Message-ID: <87wmwvi8rt.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: 71.211.130.31 X-Source-L: No X-Exim-ID: 1qg7Tb-003usr-1j X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 71-211-130-31.hlrn.qwest.net (murgatroyd) [71.211.130.31]:46208 X-Source-Auth: tom+tromey.com X-Email-Count: 3 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfJGN9SzmrmMj7VfQd7Fv0Yz0mymMVlw/RCFFwZB5+VIcxvoF1Xd2pLrC5Rvkng+VbAwf5axzFm8zg4IdSR0j500TVv4TmEkht9eY35DhVvCq3/NqmzHd qBTDQaKQqFimDBmVSrjDe47M5+iCSgTPVxi5r1c7Hbhf1mquH0TeMoCPfuMD3fN5VpA5dWHi3cejk3HTTCYdFWjpivn8aacpOuA= X-Spam-Status: No, score=-3015.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,JMQ_SPF_NEUTRAL,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP 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> v4. Simon> Dead imports remained and dead docs on types. Removed Thank you for the patch. Simon> Added tests for data breakpoint, both for a simple struct and vars[1] Simon> which is a std::vector. To be able to test this, I had to throw in Simon> the gcc pretty printers file, as I had no other way (it seemed to me) to Simon> be able to source them from the test. This is not a good approach because it means the gdb tests are dependent on the particular implementation of the C++ library. Instead it's better to write something custom -- like a small C program that uses a linked list or whatever is convenient, and then also write a pretty-printer for that data structure. Simon> I suppose one way to actually make it make Simon> sense, would be to customize a gdb.Breakpoint (i.e. deriving from it), Simon> that behaves both as a watchpoint *and* as a finish breakpoint; as soon Simon> as the finish breakpoint hits, remove the watchpoint. Non-"location" watchpoints kind of work this way, but TBH they are not very good. I think they don't really self-disable, which might be better. And AFAIK they don't really work properly with DWARF location lists. Simon> +def _info_not_available(desc): Simon> + return {"dataId": None, "description": f"{desc}", "accessTypes": None} This could just be a local function in _databreakpoint_info. If it is separate it needs some kind of intro comment. Simon> + Simon> +@in_gdb_thread Simon> +def _databreakpoint_info( Simon> + name: str, variablesReference: Optional[int] = None, frameId: Optional[int] = None frameId isn't used. I wonder if it should be. It affects what sorts of expressions can be watched. Simon> + if variablesReference is not None: Simon> + variable = None Simon> + # Resolve variable expression until find_variable throws Simon> + try: Simon> + variable = find_variable(variablesReference) Simon> + except Exception as vr_not_found: Simon> + return _info_not_available(vr_not_found) Maybe it'd be better to just wrap the whole body in try/except and remove some of these smaller ones and extra calls to _info_not_available. Simon> + elif variablesReference is None: I think this can just be 'else' because the previous test is for "not None". Then the final "return" can be removed. Simon> +def _sanitize_wp_input(breakpoints: Sequence) -> Sequence: Simon> + """Sanitize input so that it plays well with current design.""" Simon> + res = [] Simon> + for wp in breakpoints: Simon> + obj = {} Simon> + obj["condition"] = wp.get("condition") Simon> + obj["hitCondition"] = wp.get("hitCondition") Simon> + obj["accessType"] = wp.get("accessType") Simon> + obj["dataId"] = wp.get("dataId") Simon> + res.append(obj) Simon> + return res I don't understand the need for this. It seems like an identity function, more or less -- extra fields shouldn't really affect anything. Simon> + Simon> +def _wp_type(type: str): Needs a comment. Simon> +@in_gdb_thread Simon> +def _create_watchpoints(dataId, accessType, enabled=True): This should probably be "_create_watchpoint", since it only makes one. I don't think 'enabled' is ever used? Simon> + wp = gdb.Breakpoint( Simon> + spec=f"*{dataId}", type=gdb.BP_WATCHPOINT, wp_class=_wp_type(accessType) I don't think this will fully work. The dataId is computed by: + dataId = hex(int(gdb_value.address)) However, "*0xADDR" in gdb really means something like "*(int *)0xADDR". That is, this watchpoint will only watch a single word. It's tempting to use something like "*ADDR @ N_BYTES" as the expression. I wonder if that's supported by all the language parsers, though. Simon> if isinstance(event, gdb.BreakpointEvent): Simon> # Ignore the expected stop, we hit a breakpoint instead. Simon> - _expected_stop = StopKinds.BREAKPOINT Simon> + if event.breakpoints[0].expression: Simon> + _expected_stop = StopKinds.DATABREAKPOINT Simon> + else: Simon> + _expected_stop = StopKinds.BREAKPOINT This could also just examine Breakpoint.type which may be more clear. Simon> + def try_find_member(self, name: str) -> gdb.Value: Simon> + type = self.value.type Needs some sort of doc string. Simon> + # Non-pretty printed values more common. Search type definition first Simon> + for field in type.fields(): Simon> + if field.name == name: Simon> + return self.value[field] I think it's better to just always use the wrapping pretty-printer and just delete this code. One way this could matter is if we fix the "multiple variables with the same name" problem that I think can arise in some C++ inheritance scenarios -- the printer might rename the fields for presentation purposes. Simon> + for (child_name, value) in self.printer.children(): Simon> + if child_name == name: Simon> + return value It's possible the children are already cached. I sent some patches to change this area a little, but I suppose the way it should work is to examine the cache if it exists. This is an area where we could add random-access-by-name to the printer extension methods. Though that would require docs. Tom