Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit

Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit

From: Sergey Isakov <isakov-sl@bk.ru>

To: edk2-devel@lists.sourceforge.net

Date: 2011-12-12 16:06:32

Mike,

Disabling warnings we can loose some other mistakes. I prefer to add to existing procedure a line
------
Counter = (UINT64)Microseconds; //this is good for both 32 and 64 bits
------

Sergey

On 12.12.2011, at 2:48, Kinney, Michael D wrote:

Sergey,
 
The UEFI Specification does not provide details on how the UEFI services should be implemented and it does not mention compiler warnings at all.  The goal in the EDK II is to provide a UEFI conformant implementation for all UEFI supported CPU architectures and we tend to implement as much as we can in portable C sources to reduce maintenance overhead.
 
There are many alternative implementation styles that can eliminate the specific warning you are seeing, but many of those would require us to provide more that one implementation of the Stall() service in the DXE Core.  In this specific case, I would prefer we use a single portable C source implementation of Stall() and disable one warning for 32-bit builds as Andrew suggested earlier in this thread:
 
-Wno-tautological-compare 
 
Thanks,
 
Mike
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Saturday, December 10, 2011 12:43 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit
 
Mike,
 
If for any reason there is a code for Stall(70000years) and it will return too fast then it means a bug in the program. The sources should be more attentive to the users bugs.
And my first question is unresolved. I can't compile the current sources without warnings? UEFI specs said that there must be a warning?
 
Sergey
 
On 09.12.2011, at 1:00, Andrew Fish wrote:


 
On Dec 8, 2011, at 12:45 PM, Kinney, Michael D wrote:


Andrew,
 
ASSERT() may be removed in production builds, so the ASSERT() does not actually fix the issue.  Especially since UEFI Drivers and UEFI Apps may be loadable from media on some platforms and could use Stall(). 
 
Are you concerned about production code failing to stall for more than 74,000 years? I don't understand?


 
Tests of the Stall() service functionality may call with very large values and verify that they do not return within the time the test runs.  If we do not handle the overflow, then a test that calls for a large stall value may return immediately which would be a test failure.
 
 
I would question the purpose of any test that verifies that trying Stall for 74,000 years does not return too quick? 


If you want to limit the size of the stalls, then I think someone would have to propose some changes to the UEFI Specification language.
 
 
That might make sense, since the calling code for IA-32 can not call with these large values, but X64 code can. It could actually be a bug in the caller? But that is not work for this list.


Mike
 
From: Andrew Fish [mailto:afish@apple.com] 
Sent: Thursday, December 08, 2011 10:11 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit
 
Mike,
 
I think Segey's point is overflow is unreachable, based on the laws of physics. No system, built out of current technology, is going to run for 70,000 years, and even if it did I don't think some one is going to pay the electricity bill to sit in EFI stalling for 70,000 years. So the ASSERT() is really saying the spec allows a stall for up to 580,000 years on a 64-bit system, but the code does not handle the 73,000 year Stall overflow properly since it can never happen in the real world. 
 
Andrew Fish
 
0x1FFFFFFFFFFFFFFF microseconds = 73 069.3164 years



0xFFFFFFFFFFFFFFFF microseconds = 584 554.531 years
 
Thanks,
 
Andrew
 
On Dec 8, 2011, at 9:40 AM, Kinney, Michael D wrote:



Sergey,
 
Thanks for all the feedback on this change.
 
I agree it does not make sense to use Stall() for very long stall times.  However, the UEFI Specification does not specify a limit on how large Microseconds can be.  If the UEFI Specification specified a limit, then they approach you suggest would be reasonable, except it would likely be an error code returned instead of an ASSERT().
 
I recommend we keep the current logic that avoids math overflow conditions and follows the current UEFI 2.3.1 Specification.
 
Thanks,
 
Mike
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Wednesday, December 07, 2011 11:56 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit
 
Michael,
 
I can propose to do something more sensible as Andrew Fish said.
---------------
Counter = (UINT64)Microseconds; //this is good for both 32 and 64 bits
ASSERT(Counter < 100*YEARS);
...
    Counter = DivU64x32Remainder (
                MultU64x32 (Counter, 10),
                gMetronome->TickPeriod,
                &Remainder
                );
---------------
That's all folks.
 
Sergey.
 
On 07.12.2011, at 23:01, Kinney, Michael D wrote:




Sergey,
 
I agree that the missing braces should be fixed.
 
For the Stall.c code, we are sharing the same C code for 32-bit CPUs and 64-bit CPUs.  The logic is required and correct for 64-bit CPUs.  The extra check should be optimized away for 32-bit CPUs.  If that error showed up for bit 32-bit and 64-bit CPUs, then I would agree that it is a logic bug that could be fixed.
 
Mike
 
From: Sergey Isakov [mailto:isakov-sl@bk.ru] 
Sent: Tuesday, December 06, 2011 10:27 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit
 
Hi sirs,
I prefer to correct logical errors instead of close eyes on them.
In my tool_defs.txt I exclude 
-------------
-Wno-tautological-compare -Wno-unused-value -Wno-missing-braces -Wno-unused-variable 
-------------
And I have successfully compilation after some obvious corrections. Look also my letter about Missing Braces. 
Missing Braces may cause runtime error if an array will be wrong initialized.
 
Sergey.
 
On 07.12.2011, at 8:45, Sun, Rui wrote:





Sergey,
 
The purpose of this patch is to follow the UEFI spec which requires that the Stall() function stall execution on the processor for at least the requested number of microseconds.
 
It is expected that the if (Microseconds > 0x1999999999999999ULL) {} block will be optimized away with 32-bit build.
 
Could you tell me which tool chain you are using that generated the warning message?
 
 
From: Andrew Fish [mailto:afish@apple.com] 
Sent: Wednesday, December 07, 2011 9:05 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [patch] Stall.c in rev12822 is wrong for 32bit
 
Sergey,
 
What compiler where you using? I don't see this failure with Xcode 4.2 as the -Wno-tautological-compare compiler flag makes this compare not be regarded as an error.
 
Andrew Fish
 
 
On Dec 6, 2011, at 9:34 AM, Sergey Isakov wrote:






Dear sirs,
Compiling recent update I encounter an error
-------------
edk2/MdeModulePkg/Core/Dxe/Misc/Stall.c:70: warning: comparison is always false due to limited range of data type
-------------
The argument should be UINT64 but not UINTN
----------------
--- DxeMain-old.h       2011-11-23 20:57:38.000000000 +0400
+++ DxeMain.h          2011-12-06 21:25:41.000000000 +0400
@@ -565,7 +565,7 @@ CoreRestoreTpl (
 EFI_STATUS
 EFIAPI
 CoreStall (
-  IN UINTN            Microseconds
+  IN UINT64            Microseconds
   );
----------------
--- Stall-old.c   2011-12-06 20:16:44.000000000 +0400
+++ Stall.c      2011-12-06 21:22:43.000000000 +0400
@@ -52,7 +52,7 @@ CoreInternalWaitForTick (
 EFI_STATUS
 EFIAPI
 CoreStall (
-  IN UINTN            Microseconds
+  IN UINT64            Microseconds
   )
 {
   UINT64  Counter;
------------------------
 
Regards
Sergey
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Cloud Services Checklist: Pricing and Packaging Optimization
This white paper is intended to serve as a reference, checklist and point of 
discussion for anyone considering optimizing the pricing and packaging model 
of a cloud services business. Read Now!
http://www.accelacomm.com/jaw/sfnl/114/51491232/_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
 
------------------------------------------------------------------------------
Learn Windows Azure Live!  Tuesday, Dec 13, 2011
Microsoft is holding a special Learn Windows Azure training event for 
developers. It will provide a great way to learn Windows Azure and what it 
provides. You can attend the event by watching it streamed LIVE online.  
Learn more at http://p.sf.net/sfu/ms-windowsazure_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel