On 11/15/14 23:21, Scott Duplichan wrote:
> Laszlo Ersek [mailto:ler...@redhat.com] wrote:
>
> Hello Laszlo,
>
> Comments below from me, but first some ranting...
>
> We must find a better way to handle the different warning settings
> EDK2 uses for Microsoft and EDK2. For Windows users, the problem is
> minimized by the availability of needed Windows hosted gcc builds
> (as soon as the patch is accepted). For Windows, setting up EDK2+gcc
> is easy, even easier than for EDK2+Visual Studio. But for Linux, use
> of Visual Studio is essentially impossible.
>
>
>
> ]comments below
> ]
> ]On 11/13/14 18:27, Anthony PERARD wrote:
> ]> From: Scott Duplichan <sc...@notabs.org>
> ]>
> ]> This patch contain only type cast.
> ]>
> ]> Contributed-under: TianoCore Contribution Agreement 1.0
> ]> Signed-off-by: Scott Duplichan <sc...@notabs.org>
> ]> Signed-off-by: Anthony PERARD <anthony.per...@citrix.com>
> ]> ---
> ]> OvmfPkg/XenBusDxe/EventChannel.c | 6 +++---
> ]> OvmfPkg/XenBusDxe/GrantTable.c | 4 ++--
> ]> OvmfPkg/XenBusDxe/XenBus.c | 6 +++---
> ]> OvmfPkg/XenBusDxe/XenHypercall.c | 6 +++---
> ]> OvmfPkg/XenBusDxe/XenStore.c | 22 +++++++++++-----------
> ]> 5 files changed, 22 insertions(+), 22 deletions(-)
> ]>
> ]> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c
> b/OvmfPkg/XenBusDxe/EventChannel.c
> ]> index 547c111..03efaf9 100644
> ]> --- a/OvmfPkg/XenBusDxe/EventChannel.c
> ]> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
> ]> @@ -29,7 +29,7 @@ XenEventChannelNotify (
> ]>
> ]> Send.port = Port;
> ]> ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
> ]> - return ReturnCode;
> ]> + return (UINT32)ReturnCode;
> ]> }
> ]
> ]Not sure why it is safe to cast (without loss of information) an INTN
> ]retval to UINT32, on X64.
>
> Because the function returns UINT32. In other words, while the original
> code may be broken, the patch doesn't introduce any new truncation.
Right, I missed that for a minute. Thanks!
> Now, is the original code broken? It is hard for me to say for sure.
> Based on looking at other Xen code, it seems like keeping all 64-bits
> would be best. On the other hand, the hypercall return codes are
> apparently small positive and negative integers, so the truncated
> return value might be usable.
>
>
> ]> UINT32
> ]> @@ -48,7 +48,7 @@ XenBusEventChannelAllocate (
> ]>
> ]> Parameter.dom = DOMID_SELF;
> ]> Parameter.remote_dom = DomainId;
> ]> - ReturnCode = XenHypercallEventChannelOp (Private->Dev,
> ]> + ReturnCode = (UINT32)XenHypercallEventChannelOp (Private->Dev,
> ]> EVTCHNOP_alloc_unbound,
> ]> &Parameter);
> ]> if (ReturnCode != 0) {
> ]
> ]ditto
>
> same, no new truncation
>
> ]> @@ -84,5 +84,5 @@ XenBusEventChannelClose (
> ]>
> ]> Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
> ]> Close.port = Port;
> ]> - return XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close,
> &Close);
> ]> + return (UINT32)XenHypercallEventChannelOp (Private->Dev,
> EVTCHNOP_close, &Close);
> ]> }
> ]
> ]ditto
>
> same, no new truncation
>
> ]> diff --git a/OvmfPkg/XenBusDxe/GrantTable.c
> b/OvmfPkg/XenBusDxe/GrantTable.c
> ]> index f68a854..37d3bf7 100644
> ]> --- a/OvmfPkg/XenBusDxe/GrantTable.c
> ]> +++ b/OvmfPkg/XenBusDxe/GrantTable.c
> ]> @@ -101,7 +101,7 @@ XenGrantTableGrantAccess (
> ]>
> ]> ASSERT (GrantTable != NULL);
> ]> Ref = XenGrantTableGetFreeEntry ();
> ]> - GrantTable[Ref].frame = Frame;
> ]> + GrantTable[Ref].frame = (UINT32)Frame;
> ]> GrantTable[Ref].domid = DomainId;
> ]> MemoryFence ();
> ]> Flags = GTF_permit_access;
> ]> @@ -152,7 +152,7 @@ XenGrantTableInit (
> ]> #endif
> ]> EfiInitializeLock (&mGrantListLock, TPL_NOTIFY);
> ]> for (Index = NR_RESERVED_ENTRIES; Index < NR_GRANT_ENTRIES; Index++) {
> ]> - XenGrantTablePutFreeEntry (Index);
> ]> + XenGrantTablePutFreeEntry ((grant_ref_t)Index);
> ]> }
> ]>
> ]> GrantTable = (VOID*)(UINTN) MmioAddr;
> ]> diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c
> ]> index a735058..f69c27d 100644
> ]> --- a/OvmfPkg/XenBusDxe/XenBus.c
> ]> +++ b/OvmfPkg/XenBusDxe/XenBus.c
> ]> @@ -182,7 +182,7 @@ XenBusAddDevice (
> ]> Private->XenBusIo.Type = AsciiStrDup (Type);
> ]> Private->XenBusIo.Node = AsciiStrDup (DevicePath);
> ]> Private->XenBusIo.Backend = BackendPath;
> ]> - Private->XenBusIo.DeviceId = AsciiStrDecimalToUintn (Id);
> ]> + Private->XenBusIo.DeviceId = (UINT16)AsciiStrDecimalToUintn (Id);
> ]> Private->Dev = Dev;
> ]>
> ]> TempXenBusPath = AllocateCopyPool (sizeof (XENBUS_DEVICE_PATH),
> ]> @@ -274,7 +274,7 @@ XenBusEnumerateDeviceType (
> ]> XenBusAddDevice (Dev, Type, Directory[Index]);
> ]> }
> ]>
> ]> - FreePool (Directory);
> ]> + FreePool ((VOID*)Directory);
> ]> }
> ]>
> ]>
> ]> @@ -310,7 +310,7 @@ XenBusEnumerateBus (
> ]> XenBusEnumerateDeviceType (Dev, Types[Index]);
> ]> }
> ]>
> ]> - FreePool (Types);
> ]> + FreePool ((VOID*)Types);
> ]>
> ]> return XENSTORE_STATUS_SUCCESS;
> ]> }
> ]> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c
> b/OvmfPkg/XenBusDxe/XenHypercall.c
> ]> index 15e9e1c..34d92e7 100644
> ]> --- a/OvmfPkg/XenBusDxe/XenHypercall.c
> ]> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
> ]> @@ -53,7 +53,7 @@ XenHypercallHvmGetParam (
> ]>
> ]> Parameter.domid = DOMID_SELF;
> ]> Parameter.index = Index;
> ]> - Error = XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_hvm_op * 32,
> ]> + Error = XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_hvm_op *
> 32,
> ]> HVMOP_get_param, (INTN) &Parameter);
> ]> if (Error != 0) {
> ]> DEBUG ((EFI_D_ERROR,
> ]> @@ -72,7 +72,7 @@ XenHypercallMemoryOp (
> ]> )
> ]> {
> ]> ASSERT (Dev->Hyperpage != NULL);
> ]> - return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_memory_op * 32,
> ]> + return XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_memory_op *
> 32,
> ]> Operation, (INTN) Arguments);
> ]> }
> ]>
> ]> @@ -84,7 +84,7 @@ XenHypercallEventChannelOp (
> ]> )
> ]> {
> ]> ASSERT (Dev->Hyperpage != NULL);
> ]> - return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_event_channel_op *
> 32,
> ]> + return XenHypercall2 ((UINT8*)Dev->Hyperpage +
> __HYPERVISOR_event_channel_op * 32,
> ]> Operation, (INTN) Arguments);
> ]> }
> ]>
> ]> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> ]> index 7c272b3..2df8f53 100644
> ]> --- a/OvmfPkg/XenBusDxe/XenStore.c
> ]> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> ]> @@ -248,7 +248,7 @@ Split (
> ]>
> ]> /* Transfer to one big alloc for easy freeing by the caller. */
> ]> Dst = AllocatePool (*NumPtr * sizeof (CHAR8 *) + Len);
> ]> - CopyMem (&Dst[*NumPtr], Strings, Len);
> ]> + CopyMem ((VOID*)&Dst[*NumPtr], Strings, Len);
> ]
> ]This looks very much like undefined behavior in standard C, but the
> ]patch is independent of that.
>
> I think it is OK. I actually put it into a Windows app and tested it
> using VS2010 and GCC49. The function above (ExtractStrings) hangs due
> to a '+1' dropped from the original code. I will submit a patch for
> that.
My point about "undefined behavior" concerned the copying of Len bytes
into a pointer-to-char, where Len might be smaller than, equal to, or
greater than the number of bytes contained within a pointer-to-char.
I do realize what the code does, and that in this part of the blob we no
longer store pointers but the pointed-to strings, concatenated. I'm just
saying that the way it is expressed in C code is questionable. A C
compiler might infer that we're copying into an element of an array of
pointers-to-char, and assume that Len is == sizeof element. (Because
otherwise it would be undefined behavior.) This is based on the type of
the definition (ie. type) of Dst variable, which is
CONST CHAR8 **Dst;
... Anyway, now I took the time to re-read the effective type rules in
the C99 standard, and I think the above code might be okay after all,
even if we choose to be pedantic.
But, I think your point is about something else.
> The really frustrating part is the different handling of const
> by Microsoft and gcc compilers. I wanted to avoid adding this
> confusing type cast by changing the pointer declarations. I am now
> convinced it cannot be done in a way that satisfies both VS and gcc.
> The type cast silences a Microsoft warning about use of const. This
> Microsoft const warning is also responsible for the (VOID*) that
> the patch adds to FreePool is several places. This code demonstrates
> the problem:
>
> #include <malloc.h>
> void func (void)
> {
> const char **buffer;
> buffer = malloc (100);
> free (buffer);
> }
>
> Gcc accepts this even with the strictest warning settings
> (-Wall -Wextra -pedantic). But VS2010 reports:
> warning C4090: 'function' : different 'const' qualifiers
> If **buffer is replaced with *buffer, then both VS and gcc
> warn about free (buffer).
Yeah, VS is broken.
const char **buffer
means
pointer to (pointer to const char)
You can freely modify the target of the *first* reference. Which
includes freeing 'buffer'.
VS would be right in the following cases:
const char * const * buffer
char * const * buffer
ANYTHING * const * buffer
But we don't have those cases.
In any case, your explicit cast only does what the implicit type
conversion would do anyway (the prototype of CopyMem() is visible), so
I'm okay with it.
[snip]
Thanks!
Laszlo
------------------------------------------------------------------------------
Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
