On Mon, Oct 22, 2012 at 5:08 PM, Laszlo Ersek <ler...@redhat.com> wrote: > On 10/22/12 23:42, Jordan Justen wrote: >> Structures should not be directly assigned in EDK II >> code, since this leads to different behaviours on various >> compilers. > > Yes, structure assignment can be implemented by memberwise copy or > blanket memcpy(), as allowed by ISO C99 6.2.6.1p6. Why is that a problem? > > I think I do not understand some coding consideration that permeates the > edk2 tree. For another example, why is it that gcc is not allowed to > generate 64-bit division code on IA32 (by statically linking the > necessary function from libgcc.a)? > > ISO C99 explicitly allows extended integer types (for example one we > choose to designate with the "UINT64" typedef on IA32). The division > operator is not exempt from working with the extended integer types.
I think edk2 has tried to avoid compiler specific behavior to increase portability. For one thing, it is not necessarily safe to link to compiler specific libraries (libgcc.a) because it is possible those libraries may make some assumption about running under an actual OS. They may also not be concerned about code size for basically the same reason. But, it is also difficult to produce compiler specific libraries to satisfy these external references. The interface/API to these functions are often a moving target, or not publicly spec'd. To solve this, EDK II avoids structure assignments, and using structures in function calls. We also avoid 64 bit arithmetic. (By the way, to contradict myself a little, I think the StdLib may have had to provide these routines.) >> Instead, use ZeroMem to zero out the structures. > > That on the other hand casts away the volatile qualification of the > pointer before the access through it (to an object defined as volatile), > which explicitly violates C99 6.7.3p5. (See the CopyMem() comment in the > same file a bit higher up.) What is the concern? ZeroMem/SetMem/CopyMem should have written the memory by the time the call returns. Since you are actually referring to structures on the stack, we're not dealing with memory mapped I/O. > 5 If an attempt is made to modify an object defined with a const- > qualified type through use of an lvalue with non-const-qualified > type, the behavior is undefined. If an attempt is made to refer to an > object defined with a volatile-qualified type through use of an lvalue > with non-volatile-qualified type, the behavior is undefined. > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> >> --- >> OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 15 ++------------- >> 1 file changed, 2 insertions(+), 13 deletions(-) >> >> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c >> b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c >> index 66f6d31..e58dd80 100644 >> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c >> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c >> @@ -413,19 +413,8 @@ VirtioScsiPassThru ( >> volatile VIRTIO_SCSI_RESP Response; >> DESC_INDICES Indices; >> >> - // >> - // Zero-initialization of Request & Response with "= { 0 };" doesn't build >> - // with gcc-4.4: "undefined reference to `memset'". Direct SetMem() is not >> - // allowed as it would cast away the volatile qualifier. Work it around. >> - // > > Actually I didn't have to reach back to the 64-bit division example: I > introduced the workaround exactly because the standard-conformant > initialization didn't work. (Because the build system doesn't allow gcc > to use internal memset here.) May I ask why that is so? See above for the explanation... >> - union { >> - VIRTIO_SCSI_REQ Request; >> - VIRTIO_SCSI_RESP Response; >> - } Zero; >> - >> - SetMem (&Zero, sizeof Zero, 0x00); >> - Request = Zero.Request; >> - Response = Zero.Response; >> + ZeroMem ((VOID*) &Request, sizeof (Request)); >> + ZeroMem ((VOID*) &Response, sizeof (Response)); > > As my comment described, I intended to avoid precisely this. (I may have > referred to SetMem() instead of ZeroMem() due to lacking knowledge of > the edk2 API, but I had SetMem(Buffer, Length, 0x00) in mind when > writing the comment.) > > ... You probably need the (VOID*) because otherwise the compiler warns > about casting away "volatile". > >> >> Dev = VIRTIO_SCSI_FROM_PASS_THRU (This); >> CopyMem (&TargetValue, Target, sizeof TargetValue); > > I cannot ACK this unless we declare for good that we don't care about > standard C. I'm not trying to prevent this patch from going in (that > would be futile anyway), I just don't understand what standard(s) we > adhere to. > > For example, we could say that "ZeroMem()'s side effects are always > complete at the next sequence point", simply positing something that C99 > doesn't require (ie. leaves undefined). Hmm, I had thought MdePkg/Include/Library/BaseMemoryLib.h might have mentioned something about the memory-ops completing by the end of the call, but it doesn't. But I think this is the case for all BaseMemoryLib implementations. > ... I just skimmed the ToC of the Coding Standards (rev 0.53) and > nothing stands out in this regard. > > (OTOH when I first used "volatile", I did try to spell it as VOLATILE -- > cue STATIC, CONST, VOID etc --, and it didn't work. That seems to > conflict with Table 6 of the Coding Standards. > > Hmm yes, the VOLATILE macro is there only in > <EdkCompatibilityPkg/Foundation/Efi/Include/EfiTypes.h>, not in > <MdePkg/Include/Base.h>, where the rest of the all-caps macros resides > for edk2. Confusing.) That does seem a bit odd. -Jordan ------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_sfd2d_oct _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel