From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com [IPv6:2a00:1450:4864:20::22f]) by sourceware.org (Postfix) with ESMTPS id 448333857340 for ; Wed, 12 Jul 2023 09:17:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 448333857340 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-lj1-x22f.google.com with SMTP id 38308e7fff4ca-2b701e41cd3so110363341fa.3 for ; Wed, 12 Jul 2023 02:17:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689153453; x=1691745453; h=content-transfer-encoding:in-reply-to:cc:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=ztSvrDsF4EOJDJhkRJPfQbcmmZrk4yQhCfa5J9dIUtU=; b=QXpOOs8vV8RMEHLveQcTpAM82hMCAFDpTGU4yfMEwLfY1WiSdHgQC4cCPYCuHNE52d hWHioBqSSZolb1D7whvtT0m3vHXNdhvDVcXBY+U8zzy18t+xe8+yEJMceItSS8QUCc2c j4D5rmEhuMp6dEswhMsZ0/l3AvFFrfXdXBiQtKJIFzpAl4memjnHmzhPrTfljskWPaQt fGYZzh4VDxjMvx4UsnBR8MxsRKqwt0ktibHUykuYfn21Ngnw+gKZsTEH+zxA7Ja526As gkE18Ypu9BrvspP2REAPr3VxBTxHw/Kmkyd6/J58gclK0HoENvOLf57vWWPxev6reKXn 3egg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689153453; x=1691745453; h=content-transfer-encoding:in-reply-to:cc:from: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=ztSvrDsF4EOJDJhkRJPfQbcmmZrk4yQhCfa5J9dIUtU=; b=FlZ/5H9hlnmWMLKsQDVBqTLKwHDnnUrZMIjA2UxqkIMI9+57oxjbqOEEi1TcScOSPg 1GvyCkvcaB0b5i4kOrC/4m1lT5Gh6lkX9I7E3borec/8eMukR8Jvw7j0bhiQ4QJVLETt owA0JgXBSB+D0C5R7n5sMAiD7+xomwuTq4WmhnG5TsQs37R3jm6J3QfdklHl/6N7rFeZ SaRKSJ/gaVqtpFEn68Wmj3aPYzh6UH/H2rDBgpsX8fxS3amXXR98KvwUxkq60IrTUmuj pFItqetXI7Br2VPdWnBDacVIyy1iZFS3IHSP77jmiV4rgUiqp/8GRjaHpOXyTxY++7Iv aj8g== X-Gm-Message-State: ABy/qLYW9uoNv8r+wGk1ViTHrE1KTCxXBYUYgZEE2SUK/b9qhqmuy/TO BkZ4WOucRAlx1TD3F+v0rPhUhlPd0wA= X-Google-Smtp-Source: APBJJlHj5vNEoxM/7UJTKiSY+3SNzWOR1QUXpYvlraTOxeRpWuooj/2LbXnbQqwX9OF1eOZbUiL1Iw== X-Received: by 2002:a2e:9c19:0:b0:2b6:d326:156d with SMTP id s25-20020a2e9c19000000b002b6d326156dmr14377595lji.19.1689153452294; Wed, 12 Jul 2023 02:17:32 -0700 (PDT) Received: from ?IPV6:2001:2044:c7:5500:5637:6b43:7745:198c? ([2001:2044:c7:5500:5637:6b43:7745:198c]) by smtp.gmail.com with ESMTPSA id a9-20020a05651c010900b002b6e3cd82bfsm865891ljb.6.2023.07.12.02.17.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Jul 2023 02:17:31 -0700 (PDT) Message-ID: <4719ab45-639a-d40b-4385-7f1117e00da8@gmail.com> Date: Wed, 12 Jul 2023 11:17:30 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v3] gdb/DAP Fix disassemble bug To: Tom Tromey References: <20230627163836.84269-1-simon.farre.cx@gmail.com> <871qheqkkl.fsf@tromey.com> Content-Language: en-US From: Simon Farre Cc: gdb-patches@sourceware.org In-Reply-To: <871qheqkkl.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.0 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: > A few nits here: > > * Should use @in_gdb_thread > > * I can tell from the line spacing that you didn't run "black". > > * The DAP code has a convention of prefixing internal functions with "_" I'll be sure to fix the nits! > Simon> + while len(instructions) < (offset + 1): > Simon> + block = gdb.current_progspace().block_for_pc(start) > Simon> + # Fail fast; if we can't find a block backwards > Simon> + # fill all with "invalid values" > Simon> + if block is None: > Simon> + tmp = [] > Simon> + for i in range(0, offset - len(instructions)): > Simon> + tmp.append({"addr": 0, "asm": "unknown"}) > Simon> + instructions = tmp + instructions > > I wonder if this block should end with a 'break'. I guess I don't > really follow the logic here. That's probably true, as that would break the while loop's condition, so we might as well break out early. > Simon> + if skip_insns < 0: > Simon> + ins = disasm_backwards(arch, pc, skip_insns, count) > Simon> + skip_insns = 0 > Simon> + count = count - len(ins) > Simon> + result = [ > Simon> + {"address": hex(elt["addr"]), "instruction": elt["asm"]} for elt in ins > Simon> + ] > Simon> + > Simon> + for elt in arch.disassemble(pc, count=count + skip_insns)[skip_insns:]: > > Likewise it seems weird to fall through here. > > I think a test case with a negative skip would be good. > > Tom So, this block explicitly is what solves the problem with the prior patch (where skip_insns could be negative, but wasn't accounted for - particularly if total count was more than that negative offset) Let's take a simple example, where offset is -50 and total count is 100, that means we want instructions in the range [-50 .. 50); If skip_insns is less than 0 (i.e., the instructionOffset is negative), we deal with those first - set skip_insns to 0 and decrement total count of requested instructions Then we proceed and disassemble the remainder in the "postive offset direction". If there is no remainder after the first branch, no more instructions are going to be dissassembled. The reason why I'm setting skip_insns = 0 here, is to not have strange and long if-then-else branches. Because then we must account for a multiplication of things (instead of the simple fall-through approach); The different scenarios would be: Is offset negative? * (is total count less than that offset? || is total count more than that offset?) = 1 + (2) Is offset positive? (total count doesn't matter, we just start at offset .. offset+total count) = 1 Where each branch also will have copy pasted logic to account for these different scenarios. Here we instead just say "process before, then process what remains" - if skip_insns (instructionOffset) isn't negative, that branch is never taken. I think perhaps the current checked in work, confuses things with how it's named in the helper function; skip_insns is actually not skipped instructions it's instructionOffset as you can see in the actual handler function. I would like some pointers though, on how we would test this? Decide on a platform? Because it's not just a matter of testing the instruction count being returned, that's trivial, we must also make sure that the *actual disassembly* is correct. If the test suite was written in a "good" (don't let my frustration at Tcl shine through:P) language, I could write something that calls objdump/readelf, reads the actual instructions and compares it with the output of the request. In Python or Javascript this would be trivial and could be whipped up in 10 minutes, I am not as confident it's that easy in Tcl unfortunately. Perhaps call into Python from tcl? What do you think? Either way, next patch will have tests on the instruction count being correct at least.