Re: [PATCH 0/1] Refine casting expression result to bigger size

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 23 January 2017 at 11:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/22/17 07:43, Hao Wu wrote:
>> Please note that this patch is maily for feedback collection and the patch
>> only covers MdePkg. We are working on patches for other packages.
>>
>>
>> There are cases that the operands of an expression are all with rank less
>> than UINT64/INT64 and the result of the expression is casted to
>> UINT64/INT64 to fit the target size.
>>
>> An example will be:
>> UINT32 a,b;
>> // a and b can be any unsigned int type with rank less than UINT64, like
>> // UINT8, UINT16, etc.
>> UINT64 c;
>> c = (UINT64) (a + b);
>>
>> Some static code checkers may warn that the expression result might
>> overflow within the rank of int (integer promotions) and the result is
>> then cast to a bigger size.
>>
>> For the consideration of generated binaries size, the commit will keep the
>> size of the operands as the size of int, and explitly add a type cast
>> before converting the result to UINT64/INT64.
>>
>> 1). When there is no operand with type UINTN
>> (UINTN)  (a + b) -> (UINTN)(UINT32)  (a + b) or
>> (UINT64) (a + b) -> (UINT64)(UINT32) (a + b)
>>
>> 2). Otherwise
>> (UINT64) (a + b) -> (UINT64)(UINTN)  (a + b)
>>
>> Hao Wu (1):
>>   MdePkg: Refine casting expression result to bigger size
>>
>>  MdePkg/Library/BaseLib/String.c                              | 4 ++--
>>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c                    | 4 ++--
>>  MdePkg/Library/BaseS3PciLib/S3PciLib.c                       | 4 ++--
>>  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  | 4 ++--
>>  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c | 4 ++--
>>  5 files changed, 10 insertions(+), 10 deletions(-)
>>
>
> - If it is justified to do the addition in 64-bits (or in UINTN), then
> the addition should be modified accordingly. This might incur a code
> size increase, but if that's necessary for correctness, then it should
> be done.
>

Indeed. The problem with

c = (UINT64) (a + b);

is the parens: this should be written as (UINT64)a + b if the sum may
overflow a UINT32

> - However, if the addition is just fine as-is, because we know for sure
> that the sum will never overflow "int" or "unsigned int" (as selected by
> the integer promotions and the usual arithmetic conversions), then we're
> addressing the issue in the wrong direction. Namely, in this case, the
> solution is to simply drop the outermost cast, which is already useless.
> (Because, it would automatically happen as part of the assignment or the
> "return" statement.)
>

Agreed.

> I mean... Those (useless) outermost casts were probably introduced to
> appease the Visual Studio compiler. Apparently, they cause various
> static code checkers to complain, so now we introduce yet more casts to
> keep them quiet as well. When will it end?
>
> For example, the 2nd return statement of the InternalHexCharToUintn()
> function is proposed as
>
>   return (UINTN)(UINT32) (10 + InternalCharToUpper (Char) - L'A');
>
> in reality it should be just
>
>   return 10 + InternalCharToUpper (Char) - L'A';
>

Indeed. IMO this is related to our warnings-as-errors policy, while in
reality, some warnings are simply warnings, and should be ignored if
it can be confirmed by visual inspection that the expression can never
overflow.