From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1031.google.com (mail-pj1-x1031.google.com [IPv6:2607:f8b0:4864:20::1031]) by sourceware.org (Postfix) with ESMTPS id 070F83858424 for ; Wed, 5 Oct 2022 22:37:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 070F83858424 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com Received: by mail-pj1-x1031.google.com with SMTP id gf8so126741pjb.5 for ; Wed, 05 Oct 2022 15:37:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=Owk5QKogZb9XZ8Xrua+H9tBLlVxTpmF06pvkakmjcOQ=; b=Jb43lKI4wQJaFrD3rYQqzDdVrJmlhUp5B8F3EZcYeT9FUV9ZLaVE8FID8IisfIx89o 4+gecPmQNfc42XoZHRt+ij69tMBTS1UrGkuucUnyiqXnVmV0A3vt2lXAvU3hK3qu7nnV Lsvu1jeJIR7QKHb8ft2wk4Nl8TOPp1nyxFk+kbHbrT2sWpu9crAsTgr2XWtcKjAdV684 lyBZuXi1vhSseeeh5KxmKRytRU0jAQ9TPOxh1OHTi+nx9nZ/+9JqX3ySKNEXjyRHPnQI tLeJY68t3lV/hsnvS3AiHSn12iCqKuZ16yQaNaSWbOVfxnpoLX3RVnOvszV5LLdp/Suk rITQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Owk5QKogZb9XZ8Xrua+H9tBLlVxTpmF06pvkakmjcOQ=; b=GIbEL1t0tQwQ+qx0cfCbJaDa1tck21xt/yghyOOQVNzRp9q1KEFtxodWYKtRmeLtI9 p0l7RJ4FDt9f2dobDmyIg2FCVOQucjM5q5HlFQjTkbJFUTKYP4QNf6euhszs8hVyASbA gMroNFXsmFLWHJzjAeGEpwfxUSlfsPAP6lJfSMyVPPcS5Qrk6kF/RpnF2zTEjzxtVUUR axKwM/ELI7YaXWyCbv3fyz9aUCbNIY/85vTsjhcTH9OCi4F+zatkWO9nDgDRSVBQJvdc Ea/3BGUcs+f3ny4l9pEA4l4pU6Z1iN8KNm3kTvuydve6m8lyKjSL+Qrfnhf1aVJLii05 61OQ== X-Gm-Message-State: ACrzQf3tMqsDoLRU/NJ7voBiOEeyytuxrHZzz99mGgSe1D4qPsj94EZD qQiwv4RAHyDUqHdSwP5GGcvRnmU15GvJcR6+ X-Google-Smtp-Source: AMsMyM5ZyVfra4qtmA80tmJv4Cl5Rcs5fpZU/Jx5jc5eb5EWYsaJVtGgQO29L99vyQcK+ahkpf8Eog== X-Received: by 2002:a17:90b:3d8:b0:20a:8e90:8e8c with SMTP id go24-20020a17090b03d800b0020a8e908e8cmr1895175pjb.138.1665009435540; Wed, 05 Oct 2022 15:37:15 -0700 (PDT) Received: from localhost (76-210-143-223.lightspeed.sntcca.sbcglobal.net. [76.210.143.223]) by smtp.gmail.com with ESMTPSA id p30-20020aa79e9e000000b005623f96c24bsm1545528pfq.89.2022.10.05.15.37.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 15:37:14 -0700 (PDT) Date: Wed, 05 Oct 2022 15:37:14 -0700 (PDT) X-Google-Original-Date: Wed, 05 Oct 2022 15:37:12 PDT (-0700) Subject: Re: [PATCH v2 0/6] RISC-V: Fix disassembler types and styles In-Reply-To: <8735c4genq.fsf@redhat.com> CC: binutils@sourceware.org, research_trasio@irq.a4lg.com, nelson@rivosinc.com, kito.cheng@sifive.com, binutils@sourceware.org From: Palmer Dabbelt To: aburgess@redhat.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,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 List-Id: On Tue, 04 Oct 2022 01:46:01 PDT (-0700), aburgess@redhat.com wrote: > Palmer Dabbelt writes: >> On Mon, 03 Oct 2022 02:59:18 PDT (-0700), aburgess@redhat.com wrote: >>> Tsukasa OI via Binutils writes: >>> >>>> Hello, >>>> >>>> This patchset fixes various typing and styling errors on the RISC-V >>>> disassembler. >>> >>> I can't approve binutils patches. >>> >>> I've taken a look through this series, and other than the minor typo I >>> spotted, this all looks good to me. >> >> My worry with these things is that it's sort of a grey area in terms of >> what our stable user interface is: there's definitely folks who try and >> parse the text output of tools like objdump and changing anything risks >> breaking that. IMO that's too strict of a stable interface to keep >> around as it pretty much prevents us from fixing any disassembly >> weirdness, but I'm not sure if that's what other ports do and I don't >> want to make a mess by breaking users' expectations here. > > I guess we'd want to consider these things on a case-by-case basis. I'm > struggling to think of a situation where we wouldn't want to fix > something in the disassembler output if it was wrong over leaving it > incorrect because someone, somewhere, might have a script that depends > on the broken output. > > But unless I've missed something, I don't think this series makes any > changes to the output other than styling, and I don't think a script > that's trying to parse the disassembler output should be running with > styling switched on.... > > ...or if they are, maybe because they want to figure out which bits of > the text are which, then surely they'd welcome these fixes? > >> That said, as far as I can tell the only user-visible change here is to >> print some bits with the correct syntax coloring in GDB. > > And in objdump, which also styles its output by default now when the > output is going to a tty. Any scripts that capture objdump output > should see no changes with this series. Ah, cool, I hadn't seen that when poking around. I don't think it changes the reasoning, though. > The styling is pretty new, and I knew when I added it there were going > to be mistakes with things styled incorrectly. I was basically relying > on getting something in that was not terrible, and then relying on folk > like Tsukasa to come along and fix my mistakes. > > IMHO, any script that relies on a fixed styling is a bad script. Ya, that seems like an easy one -- doubly so if it's relying on styling that's wrong it a handful of cases. Like you said, if anything this would fix scripts rather than break them. > I'd probably go further, and say that any script that can't handle minor > changes in the disassembly output at all is a bad script, but without > seeing every case that might be a little harsh. I've definitely written scripts like that, and it felt pretty bad at the time. It'd still be a small headache if they broke: less actually fixing the script, but more that some test would break on a binutils upgrade and that tends to spook the HW folks into just never upgrading the toolchain again (even if it's just a test checking script that broken). That kind of stuff is someone else's problem these days, though, so I'm fine just pretending none of it ever happened ;) > Anyway, FWIW, I think we should for sure approve this series. Works for me. Sounds like Jeff was saying about the same thing in his reply, so approved from me. Tsukasa: I haven't built/tested these and my box is kind of tied up with Linux right now, but feel free to commit them assuming someone's tested them. Thanks! > > Thanks, > Andrew > > >> Seems pretty >> straight-forward to say anything users can't rely on parsing syntax >> highlighting colors being a stable interface, so maybe we can just punt >> on the grey area for now ;) >> >>> Thanks for doing this. >> >> Yep. These LGTM, but I'm still kind of buried thanks to the >> a few weeks of conference->COVID. They're probably fine, but hopefully >> Nelson has the time to chime in -- if not I'll leave them in the queue. >> >> Thanks! >> >>> >>> Andrew >>> >>> >>> >>>> >>>> Previous Patchsets: >>>> (2022-07-13) >>>> (2022-08-03) >>>> Tracker on GitHub: >>>> >>>> >>>> >>>> [Changes: v1 -> v2] >>>> >>>> - Added PATCH 5-6/6 after T-Head extensions are merged. >>>> - Slightly improved the commit messages? >>>> >>>> >>>> [Styling] >>>> - Print real immediates with the `immediate' style (PATCH 1/6) >>>> - Print comma and tabs with the `text' style (PATCH 4/6) >>>> - Print T-Head literal operand with `immediate' style (PATCH 6/6) >>>> [Typing on printf] >>>> - Fix wrong type for "%x" format specifier (PATCH 2/6) >>>> - Fix minor but various typing issues on T-Head immediates (PATCH 5/6) >>>> [Small Optimization] >>>> - Use smallest portable types possible to print (PATCH 3/6) >>>> >>>> Thanks, >>>> Tsukasa >>>> >>>> >>>> >>>> >>>> Tsukasa OI (6): >>>> RISC-V: Fix immediates to have "immediate" style >>>> RISC-V: Fix printf argument types corresponding %x >>>> RISC-V: Optimize riscv_disassemble_data printf >>>> RISC-V: Print comma and tabs as the "text" style >>>> RISC-V: Fix T-Head immediate types on printing >>>> RISC-V: Print XTheadMemPair literal as "immediate" >>>> >>>> opcodes/riscv-dis.c | 71 +++++++++++++++++++++++++-------------------- >>>> 1 file changed, 39 insertions(+), 32 deletions(-) >>>> >>>> >>>> base-commit: c21736aed1d4877e090df60362413669dbdc391d >>>> -- >>>> 2.34.1