Laszlo Ersek [mailto:[email protected]] wrote:
]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*. I feel the same about changing working code due to a compiler warning. I thing working code that is already shipping in customer systems should not be changed due to compiler warning unless the compiler warning points out a dependency on undefined behavior, or a definite portability problem. ]Thanks, ]Laszlo Is it just me who finds it humorous to see engineers debating this statement: if (Microseconds > 0x1999999999999999ULL) { Here we have dedicated code to handle delays of duration > 58,000 years. What is a typical use case where the stall needs to exceed 58,000 years? Heck, I would just return an error for any stall request of duration > 100 years. You would probably make the developer happy because it is unlikely that such a long delay was intentional. Thanks, Scott ------------------------------------------------------------------------------ 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
