On 09/14/14 09:36, Nikolai Saoukh wrote:
> Well,
> subject says all. Review and commit, please
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Nikolai Saoukh <[email protected]>
>
>
> Stall.c.patch
>
>
> Index: MdeModulePkg/Core/Dxe/Misc/Stall.c
> ===================================================================
> --- MdeModulePkg/Core/Dxe/Misc/Stall.c (revision 16103)
> +++ MdeModulePkg/Core/Dxe/Misc/Stall.c (working copy)
> @@ -65,12 +65,11 @@
>
> //
> // Counter = Microseconds * 10 / gMetronome->TickPeriod
> - // 0x1999999999999999 = (2^64 - 1) / 10
> //
> - if (Microseconds > 0x1999999999999999ULL) {
> + if (Microseconds > (MAX_UINTN / 10)) {
> //
> // Microseconds is too large to multiple by 10 first. Perform the
> divide
> - // operation first and loop 10 times to avoid 64-bit math overflow.
> + // operation first and loop 10 times to avoid UINTN math overflow.
> //
> Counter = DivU64x32Remainder (
> Microseconds,
Why should it fit?
It's not a magic number. It's a mathematical limit to see if
MultU64x32() on the other branch of the "if" can succeed without
overflow. If Microseconds is at most 0x1999999999999999ULL, then
MultU64x32 (Microseconds, 10)
will not overflow an U64, and then the outer
DivU64x32Remainder()
will be fine. The quotient, Counter, is UINT64, which is fine, and the
remainder, Remainder, is UINT32, which is also fine, because the
remainder is always strictly less than the divisor, and the divisor in
this case is gMetronome->TickPeriod, an UINT32
[MdePkg/Include/Protocol/Metronome.h].
If Microseconds is greater than 0x1999999999999999ULL, then the
operations are reordered (first divide, then implement the
multiplication by looping ten times).
The MultU64x32() and DivU64x32Remainder() functions are safe, regardless
of the natural word width.
If your patch concerns the comparison itself, rather than the code that
follows the comparison, then it isn't necessary either. The constant
0x1999999999999999ULL has type "unsigned long long". The left hand side,
Microseconds, has type UINTN, which
- either corresponds to UINT64 (hence "unsigned long long" again),
therefore the usual arithmetic conversions won't change the type or
value of either operand of the relational operator,
- or Microseconds corresponds to UINT32 (meaning "unsigned int"), which
has an integer conversion rank under that of "unsigned long long".
In the latter case, the usual arithmetic conversions prescribe
conversion of Microseconds to "unsigned long long":
[...]
If both operands have the same type, then no further conversion is
needed.
Otherwise, if both operands have signed integer types or both have
unsigned integer types, the operand with the type of lesser integer
conversion rank is converted to the type of the operand with
greater rank.
[...]
This conversion doesn't change the value of Microseconds ("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."), and
an "unsigned long long" can always represent Microseconds here. (As the
one of the C Rationales says, the usual arithmetic conversions are
"value preserving", not "unsigned preserving", but I digress.)
Of course a compiler might be capable of deducing that the condition
will never evaluate to TRUE in an IA32 built, but that's perfectly fine.
It exactly matches the intent of the code, so if the compiler can
eliminate the dead branch on IA32, it should.
Your patch would pessimize the code. On Ia32, in some cases it would
select the looping branch, even though the other branch would be fully
valid, and faster.
Is this again about compiler warnings? Like "warning: condition blah
always evaluates to TRUE"? I'm so tired of them. Compilers accommodating
the lowest common denominator in C programming quality make programming
hell for people who actually care about the C standard, and write
careful code.
(NB: I didn't write this code.)
Nacked-by: Laszlo Ersek <[email protected]>
Please excuse my irritation, but compiler warnings look like a probable
incentive for your patch, and those just *freak me out*.
Thanks,
Laszlo
------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
