Hello Shumin and others,
Please reconsider the current strategy of getting past the VS2013 warnings
C4701/C4703 about 'potentially uninitialized local
variable'. I recently proposed turning this warning off at the compile level
for all VS2013 builds. That would allow us to reinstate
the warning if/when Microsoft, gcc or some other compiler learns to produce
this warning reliable. The current approach of turning
off the warning in the source code is essentially irreversible. So if a future
compiler learns to issue this warning reliably, then
it can be reinstated and possibly point out some real coding problems. With the
current method of working around invalid cases of
warning C4701, reversing the workaround in the future is essentially impossible.
If the scenario in the above argument for disabling the warning seems
farfetched or unlikely, it is not. Here is an example
<https://github.com/tianocore/edk2/commit/c687b1464bb42a3c2d3eb2935ddd09752c6d2f2a?diff=split#diff-ede56630b6dab5741b2bbcb579d1e657R
729> (svn revision 15794). I believe others can be found too:
DevicePath = NULL;
GopDevicePath = NULL;
Status = gBS->HandleProtocol (
DeviceHandle,
&gEfiDevicePathProtocolGuid,
(VOID*)&DevicePath
);
if (EFI_ERROR (Status)) {
return Status;
}
GetGopDevicePath (DevicePath, &GopDevicePath); //What if this function
returns an error status?
DevicePath = GopDevicePath;
The VS2013 warning C4701 about possible use of uninitialized GopDevicePath is
correct in this case. No workaround is called for. The
compiler found a coding error. The code fails to check the return status after
calling GetGopDevicePath(). The compiler did a good
thing and pointed out a missing status check after the call to function
GetGopDevicePath(). But now look at what the patch does. It
presets GopDevicePath to an unusable value. That disables the compiler warning
but it doesn't fix the coding error. Worse yet, a
future compiler that handles this warning reliably will never be able to warn
about this coding error. The patch has permanently
disabled the ability of any compiler to warn about this problem. If instead, a
non-coding workaround such as turning it off at the
compiler level were used, then there would still be hope that some future
compiler could report this problem.
We know that many of the VS2013 warnings of this type are invalid. Has anyone
tried to find what triggers the invalid warnings?
Doing so might lead to a workaround that would preserve some of the warning's
functionality. If you do many build variations and
then look at these warnings, it becomes apparent more are from the IA32 build.
The X64 build has fewer. This leads to a potential
workaround: Turn off VS2013 warning only for IA32 builds. Most of the EDK2 code
gets built for X64 as well as for IA32, so this
wouldn't hide any valid C4701 warnings. But it would reduce invalid ones.
Why does the IA32 build have more C4701 warnings than the X64 build?
Experiments show the invalid C4701 warning in IA32 builds
results from the RETURN_ERROR() macro in many (or possibly all) cases. The
macro definition is correct and valid for both IA32 and
X64 builds. The VS2013 code generation for the macro is correct for both IA32
and X64 builds. Yet for IA32 builds, the macro affects
the VS2013 static analysis for IA32 mode. This workaround demonstrates:
Index: MdePkg/Include/Base.h
===================================================================
--- MdePkg/Include/Base.h (revision 16101)
+++ MdePkg/Include/Base.h (working copy)
@@ -763,7 +763,12 @@
@retval FALSE The high bit of StatusCode is clear.
**/
+// work around VS2013 IA32 warning C4701/C4703
+#if defined (_M_IX86) && defined (_MSC_VER) && (_MSC_VER == 1800)
+#define RETURN_ERROR(StatusCode) ((RETURN_STATUS)(StatusCode) >= MAX_BIT)
+#else
#define RETURN_ERROR(StatusCode) (((INTN)(RETURN_STATUS)(StatusCode)) < 0)
+#endif
///
/// The operation completed successfully.
When this workaround is used:
1) The FsAccess.c portion of svn revision 15795 (Fix VS2013 build failure) is
no longer needed.
2) Svn rev 16008 (Fix VS2013 build failure) is no longer needed.
3) Warning C4701 for ovmfpkg\library\nvvarsfilelib\fsaccess.c(445) is resolved.
4) Warning C4701 for ovmfpkg\sec\secmain.c(468) is resolved.
5) Warning C4701 for ovmfpkg\sec\secmain.c(356) is resolved.
6) Warning C4701 for duetpkg\pcibusnoenumerationdxe\pcienumeratorsupport.c(243)
is resolved.
In summary, I think these VS2013 warning C4701 handling alternatives should be
considered:
1) Disable Warning C4701/C4703 for all VS2013 builds (original patch does this)
2) Disable Warning C4701/C4703 for all VS2013 AI32builds.
3) Workaround by redefining macro RETURN_ERROR() when VS2013 IA32 build runs.
Benefits:
1) Reduce or eliminate invalid C4701 warnings.
2) Reduce or eliminate changes to valid code.
3) Preserve the ability of compilers to find real coding errors in certain
cases.
Thanks,
Scott
From: Qiu, Shumin [mailto:[email protected]]
Sent: Wednesday, September 17, 2014 09:08 PM
To: Carsey, Jaben
Cc: [email protected]
Subject: [edk2] [Patch] ShellPkg: Refine code style to avoid potential
uninitialized local variable.
Hi Jaben,
Could you help to review the patch? Refine code style to avoid potential
uninitialized local variable.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qiu Shumin <[email protected] <mailto:[email protected]> >
Thanks,
Shumin
------------------------------------------------------------------------------
Slashdot TV. Video for Nerds. Stuff that Matters.
http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
