From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) by sourceware.org (Postfix) with ESMTPS id C3BC13858CDA for ; Wed, 26 Apr 2023 09:56:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C3BC13858CDA 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-pl1-x630.google.com with SMTP id d9443c01a7336-1a667067275so54734155ad.1 for ; Wed, 26 Apr 2023 02:56:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1682502983; x=1685094983; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=zh7BIEeWEg6npC95ODJuqv1hrE9W31QcPW/iIzh+qe8=; b=ZywmbTK7ra4+KXfaVCon/iJ/BUbTFUCDJIk71pTzDzVlFClzzycekXgsh31Og8ikqW l4rqjVvGtjQ47ozhbF/lgbJkZzGmfxUgXnN2OA5Bc3I0m8Sf2EawOejaUQfbitUm0eSN Qbt8IhzaukkqJHdGQUaxxbw9/TZ7mFNtpRgnmsKtObWOA0ys35YYUzMq69Wn84VqOzHF e8BubA+bVcKE6efKAiURbQEIXcoUvOBTJengfoCmfMl6KCTyONcUtSuCCgIPn3vPK4zb fA1G046GqbcjNyiDsyzTNlqV19flahW4adgGobqkSuC64JSMgQb31j9CJ8LREeGImtMu k+pA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682502983; x=1685094983; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=zh7BIEeWEg6npC95ODJuqv1hrE9W31QcPW/iIzh+qe8=; b=FfFIoJWvaO/wXaADW98+Rbmomoj8S3e5Z56aH4ccGmixZjdxyLowBz0gHR13GkFEUO y9zsT+WiyttoyyncLhDDF/wgzB7d75ZlAjVckDl6+B24AJp0vlxkj2QGsGuBb7RmD+sU EXIRZ12SscdyWcCtf1qCcAS7BwPONXCfW3CRmEJAZAhtEiXGnsmIHpbk1/qYETfziDL6 /8nNItYZR6sLHdSpamuVtGzHCoO8UzZ7YlBaldGd6qEv2pb0R9fbMPM72jL911UaVBrj YJFn6MWS8X0b0A9Ztcv2PCCryOhHsJygsyzCdBVf6SFn2wZLfHSrLz9GAlAysJDycD+g pPoQ== X-Gm-Message-State: AAQBX9cN0PpiPkX8lv+M56tUjjy1mHd4SWz6fO6DpgTINXYgbFfRA3KK O2mitai1ib7EWq64tVoJJRNI8i/4jsA= X-Google-Smtp-Source: AKy350YwXDQAsPZN3hcUMNQKLb5Vxa0fw35iDLJx4htghO/1p6f0U9lJ7dQzogdIRw3CRI/x2VbLIQ== X-Received: by 2002:a17:903:8c7:b0:1a0:5349:6606 with SMTP id lk7-20020a17090308c700b001a053496606mr20920337plb.56.1682502983178; Wed, 26 Apr 2023 02:56:23 -0700 (PDT) Received: from squeak.grove.modra.org (158.106.96.58.static.exetel.com.au. [58.96.106.158]) by smtp.gmail.com with ESMTPSA id jd15-20020a170903260f00b001a97ffb87b7sm3940832plb.12.2023.04.26.02.56.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Apr 2023 02:56:22 -0700 (PDT) Received: by squeak.grove.modra.org (Postfix, from userid 1000) id 2E80F1142DFC; Wed, 26 Apr 2023 19:26:20 +0930 (ACST) Date: Wed, 26 Apr 2023 19:26:20 +0930 From: Alan Modra To: Jan Beulich Cc: binutils@sourceware.org Subject: Re: i386-dis.c UB shift and other tidies Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3028.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,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: On Wed, Apr 26, 2023 at 10:09:13AM +0200, Jan Beulich wrote: > On 26.04.2023 06:39, Alan Modra via Binutils wrote: > > 1) i386-dis.c:12055:11: runtime error: left shift of negative value -1 > > Bit twiddling is best done unsigned, due to UB on overflow of signed > > expressions. Fix this by using bfd_vma rather than bfd_signed_vma > > everywhere in i386-dis.c except print_displacement. > > > > 2) Return get32s and get16 value in a bfd_vma, reducing the need for > > temp variables. > > While I'm okay with most other changes, I find it pretty odd for any of > ... > > > 3) Introduce get16s and get8s functions to simplify the code. > > ... the gets() functions to return an unsigned quantity. Change it back if you find it ugly, but I've found it better to default to using unsigned types of the largest width you are typically going to manipluate. > This then > leads to ugly casting in at least OP_E_memory() (once explicit, once > implicit). If they want to do some of the calculations (shifts in > particular, as you say) using unsigned intermediate types, that's of > course fine. Which is ugly too, and we often forget to avoid UB by casts or assigning to unsigned types. > As a minor remark - because of you switching some of the get16() to > get16s(), get16() doesn't need a forward declaration anymore (just > like get32() and get64() don't have such [anymore]). (Ultimately I'd > like to get rid of as many forward declarations of static functions > as possible, because this always requires touching yet one more place > when changing their signatures.) I missed that. Maybe you should move things around in the file? > > 4) With some optimisation options gcc-13 legitimately complains about > > a fall-through in OP_I. Fix that. OP_I also doesn't need to use > > "mask" which was wrong for w_mode anyway. > > While making the earlier recent change I was puzzled by that, too, > but I deliberately left it untouched. I'm afraid we don't really have > any test for it, and it looked to me as if that masking was > intentionally done that (odd) way. (This isn't an objection to the > change, but I wouldn't be surprised if something subtly broke, which > we'd then need to take care of later on.) I checked carefully. It really is unneeded when you consider the range of values returned by the get* functions. get16 is limited to [0,0xffff], so masking with 0xfffff does nothing. > > 5) Masking with & 0xffffffff is better than casting to unsigned. We > > don't know for sure that unsigned int is 32-bit. > > > > 6) We also don't know that unsigned char is 8 bits. Mask codep > > accesses everywhere. I don't expect binutils will work on anything > > other than an 8-bit char host, but if we are masking codep accesses in > > some places we might as well be consistent. (Better would be to use > > stdint.h types more in binutils.) > > Would there be anything wrong with switching codep to uint8_t * right > away, in place of all the masking by 0xff that you add? When also done > for its *_codep siblings, at the first glance this looks to not require > much further touching (and hence presumably overall less code churn; > existing masking that then clearly isn't necessary anymore could of > course be purged right away, but this could also be left for later). I think switching would be a good idea. I've changed bfd_byte (not yet committed) and that seems good. -- Alan Modra Australia Development Lab, IBM