Laszlo Ersek [mailto:[email protected]] 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 <[email protected]> ]> ]> This patch contain only type cast. ]> ]> Contributed-under: TianoCore Contribution Agreement 1.0 ]> Signed-off-by: Scott Duplichan <[email protected]> ]> Signed-off-by: Anthony PERARD <[email protected]> ]> --- ]> 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. 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. 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). ]> FreePool (Strings); ]> ]> /* Extract pointers to newly allocated array. */ ]> @@ -660,7 +660,7 @@ XenStoreProcessMessage ( ]> } else { ]> DEBUG ((EFI_D_WARN, "XenStore: Watch handle %a not found\n", ]> Message->u.Watch.Vector[XS_WATCH_TOKEN])); ]> - FreePool(Message->u.Watch.Vector); ]> + FreePool((VOID*)Message->u.Watch.Vector); ]> FreePool(Message); ]> } ]> EfiReleaseLock (&xs.RegisteredWatchesLock); ]> @@ -829,7 +829,7 @@ XenStoreTalkv ( ]> } ]> } ]> ]> - Status = XenStoreReadReply (&Message.type, LenPtr, &Return); ]> + Status = XenStoreReadReply ((enum xsd_sockmsg_type *)&Message.type, LenPtr, &Return); ] ]This seems wrong, generally speaking (again, independently of the patch). ] ]Namely, while enumeration constants are required by the C standard to ]have type "int" (which corresponds to INT32 in edk2), the enumerated ]type itself ] ] shall be compatible with char, a signed integer type, or an ] unsigned integer type. The choice of type is implementation- ] defined, but shall be capable of representing the values of all the ] members of the enumeration. ] ]Looking at "enum xsd_sockmsg_type", a conformant platform might ]perfectly well could choose UINT16 for it. And then the above pointer ]punning is wrong, because the callee will overwrite only part of the ]callee's data (Message.type is UINT32). Good point, I don't see anything to prevent that. ]But, I'll assume that in edk2 / BaseTools, the enum is actually equated ]with UINT32. So in practice this should work. It is just an ordinary enum in xs_wire.h, and a UINT16 would hold every value as you say. Worse case, a dummy value > 16 bits could be added to force the compiler to use 32-bits as a work around. ]> ]> Error: ]> if (Status != XENSTORE_STATUS_SUCCESS) { ]> @@ -843,7 +843,7 @@ Error: ]> } ]> ]> /* Reply is either error or an echo of our request message type. */ ]> - ASSERT (Message.type == RequestType); ]> + ASSERT ((enum xsd_sockmsg_type)Message.type == RequestType); ] ]Hrmpf. Again, the same difference. Given that we decided to cast, I'd ]have cast the RHS to the wider UINT32 type; considering that the RHS is ]"trusted" (comes from the caller) and the LHS comes from xenstored in ]dom0. (Which we shouldn't narrow before investigating.) Yes, that would be a better way. ]> ]> if (ResultPtr) { ]> *ResultPtr = Return; ]> @@ -975,7 +975,7 @@ XenStoreWaitWatch ( ]> if (Message->u.Watch.Handle == Token) { ]> RemoveEntryList (Entry); ]> EfiReleaseLock (&xs.WatchEventsLock); ]> - FreePool(Message->u.Watch.Vector); ]> + FreePool((VOID*)Message->u.Watch.Vector); ]> FreePool(Message); ]> return XENSTORE_STATUS_SUCCESS; ]> } ]> @@ -1057,8 +1057,8 @@ XenStoreInit ( ]> ]> xs.Dev = Dev; ]> ]> - xs.EventChannel = XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_EVTCHN); ]> - XenStoreGpfn = XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN); ]> + xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_EVTCHN); ]> + XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN); ]> xs.XenStore = (VOID *) (XenStoreGpfn << EFI_PAGE_SHIFT); ]> DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n", ]> xs.XenStore, xs.EventChannel)); ]> @@ -1115,7 +1115,7 @@ XenStoreDeinit ( ]> XENSTORE_MESSAGE *Message = XENSTORE_MESSAGE_FROM_LINK (Entry); ]> Entry = GetNextNode (&xs.WatchEvents, Entry); ]> RemoveEntryList (&Message->Link); ]> - FreePool (Message->u.Watch.Vector); ]> + FreePool ((VOID*)Message->u.Watch.Vector); ]> FreePool (Message); ]> } ]> } ]> @@ -1202,7 +1202,7 @@ XenStorePathExists ( ]> if (Status != XENSTORE_STATUS_SUCCESS) { ]> return FALSE; ]> } ]> - FreePool (TempStr); ]> + FreePool ((VOID*)TempStr); ]> return TRUE; ]> } ]> ]> @@ -1283,7 +1283,7 @@ XenStoreTransactionStart ( ]> Status = XenStoreSingle (XST_NIL, XS_TRANSACTION_START, "", NULL, ]> (VOID **) &IdStr); ]> if (Status == XENSTORE_STATUS_SUCCESS) { ]> - Transaction->Id = AsciiStrDecimalToUintn (IdStr); ]> + Transaction->Id = (UINT32)AsciiStrDecimalToUintn (IdStr); ]> FreePool (IdStr); ]> } ]> ]> @@ -1419,7 +1419,7 @@ XenStoreUnregisterWatch ( ]> Entry = GetNextNode (&xs.WatchEvents, Entry); ]> if (Message->u.Watch.Handle == Watch) { ]> RemoveEntryList (&Message->Link); ]> - FreePool (Message->u.Watch.Vector); ]> + FreePool ((VOID*)Message->u.Watch.Vector); ]> FreePool (Message); ]> } ]> } ]> ] ]I realize that this code is very stable (it is based on very stable ]code, anyway); and I won't pretend to know it better than you. I would ]have followed a very different style for this, but that's not really ]important. I can't point out anything that would be a bug in practice, ]so let's not waste our time. ] ]Acked-by: Laszlo Ersek <[email protected]> ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
