Scott Duplichan [mailto:[email protected]] wrote:
]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. Forgot to finish this part... A way to reduce the need to test new code with both gcc and Microsoft compilers is to choose matching warning settings for both. A while back I proposed some changes to make the Microsoft warning settings more like those in effect for gcc. This proposal was not popular. I suppose that means the gcc warning settings must be changed to match Microsoft. This is not a perfect solution because the warning settings for gcc cannot be made to exactly match Microsoft. Here is the closest I can find: -Wconversion -Wno-sign-conversion -pedantic -std=c99 One problem is that existing EDK2 code cannot build with these settings. Changing existing working code to match these warning settings is not a good idea in my opinion. I don't know of a good way to make the extra gcc flags apply to new code only. The extra gcc flags can be added by hand during development. Thanks, Scott ]]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 ------------------------------------------------------------------------------ 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
