From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 44EEE21139F63 for ; Tue, 19 Jun 2018 05:51:43 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9CA7EF68A7; Tue, 19 Jun 2018 12:51:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-221.rdu2.redhat.com [10.10.120.221]) by smtp.corp.redhat.com (Postfix) with ESMTP id EAA6E111C4BE; Tue, 19 Jun 2018 12:51:41 +0000 (UTC) To: Leif Lindholm Cc: Ard Biesheuvel , edk2-devel@lists.01.org References: <20180618204918.31835-1-ard.biesheuvel@linaro.org> <20180618215758.vsoetpww4ax64v6g@bivouac.eciton.net> <96210413-f241-63d1-3e96-a77757d3b0e4@redhat.com> <20180619095411.52siakmtbfokc6mu@bivouac.eciton.net> From: Laszlo Ersek Message-ID: <6e536d26-a1bb-52f2-ceca-8b39e35fe5ce@redhat.com> Date: Tue, 19 Jun 2018 14:51:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180619095411.52siakmtbfokc6mu@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 19 Jun 2018 12:51:42 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 19 Jun 2018 12:51:42 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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 12:51:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/19/18 11:54, Leif Lindholm wrote: > On Tue, Jun 19, 2018 at 12:51:49AM +0200, 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). > > Err, if by UINT64_MAX + 1 you mean 0, No, I don't. > as a way of introducing the > second part, I'm with you. Otherwise I think I need to go back to > school. I really meant (UINT64_MAX + 1) as the mathematical integer 18,446,744,073,709,551,616; without the modular wrap-around that occurs in C. The modular wrap-around of C is defined in terms of the mathematical values. This is how the ISO C99 standard defines the behavior: ------ 6.3.1.3 Signed and unsigned integers 1 When a value with integer type is converted to another integer type other than _Bool, if the value can be represented by the new type, it is unchanged. 2 Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type. [footnote 49] 3 Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised. [footnote 49] The rules describe arithmetic on the mathematical value, not the value of a given type of expression. ------ Therefore we really have (18,446,744,073,709,551,616 + (-2)) in the above reasoning. > >> 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). > > But these examples make it look like the differences are an > unobservable internal side effect. That's not the case. Hm, sorry, I don't understand. My point was that the difference was extremely observable, because in the first assignment, we end up with value 18,446,744,073,709,551,614; in the second, we end up with 4,294,967,294. Perhaps my expression "exact same value" was misleading. There I referred to 6.3.1.3p1, which I've now quoted above as well -- I meant that converting 4,294,967,294 from "unsigned int" to either of "unsigned long int" or "unsigned long long int" didn't change the value. Thanks! Laszlo