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.
> 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.)
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?
> - 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).
... 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.)
Thanks,
Laszlo
------------------------------------------------------------------------------
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
