From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3BA3D210E4349 for ; Mon, 18 Jun 2018 23:37:08 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id l19-v6so19106613ioj.5 for ; Mon, 18 Jun 2018 23:37:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yhcpzSvI9vmw1fvwYruVmtWGBr1mCePppNLzHLTu1nU=; b=NkWY+9WPKaMnyjQKk/x70LDYdLKBwgWSmSOGuwlX3LBZ3je7gGnJ8IYPQ7C9JDG0jb q8VknMlRoI9uMBV43IuMzUK0Hu+Ev4RQc22Set13lED1W/sprCCxC048DdAsotZV/Idr QVQTkyQ47sLpSamxxUu1NnKDn4L5vHQM9G6uk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yhcpzSvI9vmw1fvwYruVmtWGBr1mCePppNLzHLTu1nU=; b=FRswx3KhS/7VeNm9IfZCcvqtiGMrIGB2TwkrjP/37Xo3myKk82d0UvAJ6rTjeZGxGh FcA2Mkkao3PvxPOb6Q2nIWJxGnshG72O98bvn4qNkKAr4HqF8A4E5n8bfFykeNhtdLQ7 T16dW/XmEVmVsfDMBYQsdUbJhpj2PKWtWj93pccdQ5txT0CS+lAI16S6EYL2NbUf6fhN L1aEzY6i8F28zGSYnh0G5XkZT76LmNMtkCyTjSbxV/cXyUcbUw+A0igrOJJ5NhPs2//q jZWYixFOJbJvnctgsJQG+Nx2FokyuHpef/PVmVY7dY03zTU0U/8xWB6bHjvqY4sk1IWE R7LQ== X-Gm-Message-State: APt69E2+i2LPexjrGLSt4DIMtFy8HvMEdUl/EkkuYLYNm7/HT2XdJdCE 5UYiWhUqn0lHij1YAFXa3tt2ZQdwBJgOAP3z6Y6q7g== X-Google-Smtp-Source: ADUXVKLQKDYnRy70cZdsFqQen/n2UIhLanQLfDqem6MkRpv+wtuXu5ZEP8CDjqgS3dLYZwRC4+wbIbOJlP7OKFdowsw= X-Received: by 2002:a6b:dd0b:: with SMTP id f11-v6mr12331394ioc.173.1529390227408; Mon, 18 Jun 2018 23:37:07 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 18 Jun 2018 23:37:06 -0700 (PDT) In-Reply-To: <96210413-f241-63d1-3e96-a77757d3b0e4@redhat.com> References: <20180618204918.31835-1-ard.biesheuvel@linaro.org> <20180618215758.vsoetpww4ax64v6g@bivouac.eciton.net> <96210413-f241-63d1-3e96-a77757d3b0e4@redhat.com> From: Ard Biesheuvel Date: Tue, 19 Jun 2018 08:37:06 +0200 Message-ID: To: Laszlo Ersek Cc: Leif Lindholm , "edk2-devel@lists.01.org" Subject: Re: [PATCH] EmbeddedPkg/GdbSerialLib: avoid left shift of negative quantity X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jun 2018 06:37:08 -0000 Content-Type: text/plain; charset="UTF-8" On 19 June 2018 at 00:51, Laszlo Ersek wrote: > On 06/18/18 23:57, Leif Lindholm wrote: >> On Mon, Jun 18, 2018 at 10:49:18PM +0200, Ard Biesheuvel wrote: >>> Clang complains about left shifting a negative value being undefined. >> >> As well it should. >> >>> EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c:151:30: >>> error: shifting a negative signed value is undefined [-Werror,-Wshift-negative-value] >>> OutputData = (UINT8)((~DLAB<<7)|((BreakSet<<6)|((Parity<<3)|((StopBits<<2)| Data)))); >>> >>> Redefine all bit pattern constants as unsigned to work around this. >> >> So, I'm totally OK with this and >> Reviewed-by: Leif Lindholm >> but ... >> would it be worth fixing up BIT0-31 in Base.h and use those? > > If we started with the BITxx macros now, I'd agree. Given the current > tree, I don't :) I've made an argument against the suggestion earlier: > > UINT64 Value64; > > Value64 = ~0x1; > Value64 = ~0x1U; > > The two assignments produce different results. > > In the first case, the integer constant 0x1 has type "int". Our C > language implementation uses, for type "int", two's complement > representation, 1 sign bit, 31 value bits, and no padding bits. So after > the bitwise complement, we get 0x1111_1111_1111_1110, also with type > int, and with value (-2). Taking UINT64 for "unsigned long int" or > "unsigned long long int", after the conversion implied by the assignment > we get ((UINT64_MAX + 1) + (-2)), aka (UINT64_MAX - 1). > > In the second case, the constant 0x1U has type "unsigned int". After the > bitwise complement, we get (UINT32_MAX - 1), also of type "unsigned > int". Taking UINT64 for "unsigned long int" or "unsigned long long int", > after the conversion implied by assignment we get the exact same value, > (UINT32_MAX - 1). > > In assembly parlance this is called "sign-extended" vs. "zero-extended". > I dislike those terms when speaking about C; they are not necessary to > explain what happens. (The direction is the opposite -- the compiler > uses sign-extension / zero-extension, if the ISA supports those, for > implementing the C semantics.) > > So, I don't recommend changing BIT0 through BIT30 in Base.h, unless we'd > like to audit all their uses :) > To be honest, I can't say that I had all of this on my radar, but I agree that redefining the BITn macros may create more problems than it solves. Thanks for the elaborate write up. > Note: "through BIT30" is not a typo above. BIT31 already has type > "unsigned int". That's because of how the "type ladder" for integer > constants works in C. It's easy to look up in the standard (or, well, in > the final draft), but here's the mental model I like to use for it: > > - The ladder starts with "int", and works towards integer types with > higher conversion ranks, until the constant fits. > > - Normally only signed types are considered; however, when using the 0x > (hex) or 0 (octal) prefixes, we add unsigned types to the ladder. Each > of those will be considered right after the corresponding signed > integer type, with equal conversion rank. The lesson here is that the > 0x and 0 prefixes *extend* the set of candidate types. > > - The suffix "u" (or equivalently "U") *restricts* the ladder to > unsigned types, however. (Regardless of prefix.) > > - The suffixes "l" and "ll" (or equivalently, "L" and "LL", resp.) don't > affect signedness, instead they affect how high we set our foot on the > ladder at first. And, we climb up from there. > > Given our "signed int" and "unsigned int" representations (see above), > BIT30 (0x40000000) fits in "int", so it gets the type "int". However, > BIT31 (0x80000000) does not fit in "int". Because we use the 0x prefix > with it, it gets the type "unsigned int", because there it fits. Because > BIT31 already gets type "unsigned int", we could append the "u" suffix > to BIT31 (and BIT31 only), without any change in behavior. > > This also means that you already get very different results for the > following two assignments: > > Value64 = ~BIT30; > Value64 = ~BIT31; > > Now, in an ideal world: > - all BIT0..BIT31 macros would carry the U suffix, > - we'd *never* apply bitwise complement to signed integers (even though > the result of that is implementation-defined, not undefined or > unspecified), > - we'd write all expressions similar to the above as > > Value64 = ~(UINT64)BIT30; > Value64 = ~(UINT64)BIT31; > > I don't think we can audit all such uses now, however. > > The present patch differs because it's -- probably -- not hard to review > all uses of the macros being modified here. > > Thanks > Laszlo > >> >> / >> Leif >> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c b/EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c >>> index 069d87ca780d..7931d1ac4e2b 100644 >>> --- a/EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c >>> +++ b/EmbeddedPkg/Library/GdbSerialLib/GdbSerialLib.c >>> @@ -40,11 +40,11 @@ >>> //--------------------------------------------- >>> // UART Register Bit Defines >>> //--------------------------------------------- >>> -#define LSR_TXRDY 0x20 >>> -#define LSR_RXDA 0x01 >>> -#define DLAB 0x01 >>> -#define ENABLE_FIFO 0x01 >>> -#define CLEAR_FIFOS 0x06 >>> +#define LSR_TXRDY 0x20U >>> +#define LSR_RXDA 0x01U >>> +#define DLAB 0x01U >>> +#define ENABLE_FIFO 0x01U >>> +#define CLEAR_FIFOS 0x06U >>> >>> >>> >>> -- >>> 2.17.1 >>> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> >