From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) by sourceware.org (Postfix) with ESMTPS id 25C573858D28 for ; Sat, 17 Jun 2023 22:47:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 25C573858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x134.google.com with SMTP id 2adb3069b0e04-4f866a3d8e4so654600e87.0 for ; Sat, 17 Jun 2023 15:47:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687042040; x=1689634040; h=content-transfer-encoding:in-reply-to:from:cc:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=n2+nxENbVH6C3NT9i2+pAUETPb1npTtLu95lwBx9qP0=; b=aFbOs8B/FOSRx+SHHnzCxf1uIj5MKjWuF9BUTG5vnFTO8QSw3Pw2HbEVUf8+c/eUeO nBZwxaZE6BN3mJSTlMrN4jpyTcFWmsfDsYI1T4bAoGCqfGwEKxb+nF8GyTOrKLXxlGzu cq1NSsqjOJTkWgxOa0zgbgCq/XNhSsA2Sx/d5Lflcd1NvID/Umzkip6R/L+E0v8Nh4bS qI1m5OeeprFPGmtaDMgUV08EtL9DJOGGbuMIlNcxAc12Ti0MherkcHPFAeu2t5kPPA33 1FGY22Ygb7cgu9X0pzaxDXmHVHBed+DkASN+id5cxiqxHmCBlUfiT2jKANpp4OnMGOZT T43g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687042040; x=1689634040; h=content-transfer-encoding:in-reply-to:from:cc:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=n2+nxENbVH6C3NT9i2+pAUETPb1npTtLu95lwBx9qP0=; b=aNH8xGGteUyM7kWMw/z2r9EI1Vd6W4TKGsaPDbkDTljx+Ji7l/7oa6kxNOKh/BtK6W au/JxA4zRNxGoujOLKYpQ6rOVyptAYcJPoM+MdDD3NWYI8SdcZmm1vC9kdJE+myzHMdI rMin5Sg/0ea+uKxld5cF2Dp2jKWsGhqiWHRjvWF+bKPh0p5KjVTwcuXMUYj6KXn9TSMv YwFBdSzLAPsnOd1Y1Lb33UfcN9p/1PTO/6rPOrbBKrUmKn56/ehp5DdPH0Otr2KvGCuP ySX7PDp6mFuB0FG/GnZT1tdpGrr57b18Ba/fzWM3+Fy4NAii0MgeGiUiXXLFDSd41rNg rMNQ== X-Gm-Message-State: AC+VfDxkWifJuQu+dSayYjTFNKvnzqZFqhLbDIGEJAS0jyPlS6udUZGX IYXvNt5ZUCzLLXMXLQvoNwIoLD3kDf0= X-Google-Smtp-Source: ACHHUZ5SocCNVVs2qHV444t1G2u/I1HyPCZqM7QRRoKeTVQYBGL/fWcVTSuycur2XtRogTXQf6oA9Q== X-Received: by 2002:a19:6755:0:b0:4f8:58f4:b96e with SMTP id e21-20020a196755000000b004f858f4b96emr2867181lfj.37.1687042040216; Sat, 17 Jun 2023 15:47:20 -0700 (PDT) Received: from [192.168.1.20] (78-73-77-63-no2450.tbcn.telia.com. [78.73.77.63]) by smtp.gmail.com with ESMTPSA id z3-20020a19f703000000b004f62e33f343sm3621283lfe.109.2023.06.17.15.47.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 17 Jun 2023 15:47:19 -0700 (PDT) Message-ID: <2f655b81-4496-1c07-94ab-e1046a0b2015@gmail.com> Date: Sun, 18 Jun 2023 00:47:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH v2] gdb/dap - dataBreakpointInfo & setDataBreakpoints To: Tom Tromey References: <20230615182845.97700-1-simon.farre.cx@gmail.com> <87bkhf46lu.fsf@tromey.com> Content-Language: en-US Cc: gdb-patches@sourceware.org From: Simon Farre In-Reply-To: <87bkhf46lu.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > 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/ I'll get it done in v3, I saw some minor documentation errors i had left behind too. > 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. I'll kill the comments about breakpoints being translated -  I wanted to be able to enable/disable individual breakpoint locations created by GDB, but that perhaps might not be the goal/scope of the DAP-interpreters initial feature? Besides, speaking strictly from a VSCode perspective here; one could always disable individual breakpoint locations using their "debug console"/evaluateRequest (which would be the equivalent of typing `disable break 1.1`), even if they don't show up in the UI. After having fiddled around with the breakpoint code, it'd require some potential hackery and I wasn't at all pleased with my intial attempt, so I'll remove the comments. I'll re-write the watchpoint creation so that it can fit in the current design. > 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. After having read this the first time I had no idea what you meant, but I (finally) figured out what you meant and where this approach would be greatly useful; for instance, if we had a `std::vector foo` , we can now set `watch -location foo[10]` because we would be able to resolve element 10 and it's address, right?; using my approach it would attempt to watch `foo.[10]` (or maybe foo.10) which is nonsensical. So great idea, I'll try get something that'll work with pretty printers that return `gdb.Value`. I'll also add new tests for this as well. I'll address the remainder of your review after I've addressed these points first.