Re: [PATCH 1/1] StdLib/EfiSocketLib: Fix ABI mismatch for 2 event functions

["Palmer, Thomas" <thomas.palmer@hpe.com>]

Hi Daryl,

	The gist of the patch is to have these callbacks match the EFI_EVENT_NOTIFY when compiled in GCC/DEBUG.   The EFIAPI token's absence is not noticed in Visual Studio builds b/c VS builds everything with Microsoft's ABI.   However, with GCC/DEBUG the EFIAPI token is critical for instructing compiler which ABI to use.   (I believe GCC/RELEASE uses Microsoft ABI).

	The other piece of the patch is simply changing the callback's second argument type to match EFI_EVENT_NOTIFY. I created a new variable in each function with the callback's old argument type and name so the rest of the function can remain unchanged.

	You should not notice any functional differences for Visual Studio and GCC RELEASE builds.  However, GCC DEBUG builds should now function correctly.

Regards,

Thomas Palmer

"I have only made this letter longer because I have not had the time to make it shorter" - Blaise Pascal


-----Original Message-----
From: Daryl McDaniel (EDK2 Lists) [mailto:edk2-lists@mc2research.org] 
Sent: Monday, September 11, 2017 3:40 PM
To: Palmer, Thomas <thomas.palmer@hpe.com>; 'Carsey, Jaben' <jaben.carsey@intel.com>; edk2-devel@lists.01.org
Cc: Shifflett, Joseph <joseph.shifflett@hpe.com>
Subject: RE: [PATCH 1/1] StdLib/EfiSocketLib: Fix ABI mismatch for 2 event functions

Thomas,

I'll take a look at this.
Have you determined what the impact of changing the signature of the functions (adding/removing parameters) will be?  Can it adversely affect existing code?

Sincerely,
Daryl McDaniel


> -----Original Message-----
> From: Palmer, Thomas [mailto:thomas.palmer@hpe.com]
> Sent: Monday, September 11, 2017 12:35 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; 
> edk2- lists@mc2research.org
> Cc: edk2-lists@mc2research.org; Shifflett, Joseph 
> <joseph.shifflett@hpe.com>
> Subject: RE: [PATCH 1/1] StdLib/EfiSocketLib: Fix ABI mismatch for 2 
> event functions
> 
> Daryl?
> 
> 	I haven't seen a comment on this patch since 8/10
> 
> 
> Regards,
> 
> Thomas Palmer
> 
> "I have only made this letter longer because I have not had the time 
> to make it shorter" - Blaise Pascal
> 
> 
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Thursday, August 10, 2017 5:59 PM
> To: Palmer, Thomas <thomas.palmer@hpe.com>; edk2-devel@lists.01.org
> Cc: edk2-lists@mc2research.org; Shifflett, Joseph 
> <joseph.shifflett@hpe.com>
> Subject: RE: [PATCH 1/1] StdLib/EfiSocketLib: Fix ABI mismatch for 2 
> event functions
> 
> Looks good to me.
> 
> Daryl?
> 
> > -----Original Message-----
> > From: Thomas Palmer [mailto:thomas.palmer@hpe.com]
> > Sent: Thursday, August 10, 2017 3:35 PM
> > To: edk2-devel@lists.01.org
> > Cc: edk2-lists@mc2research.org; Carsey, Jaben 
> > <jaben.carsey@intel.com>; joseph.shifflett@hpe.com; Thomas Palmer 
> > <thomas.palmer@hpe.com>
> > Subject: [PATCH 1/1] StdLib/EfiSocketLib: Fix ABI mismatch for 2 
> > event functions
> > Importance: High
> >
> > The gBS->CreateEvent expects a EFI_EVENT_NOTIFY function as the 
> > third argument. The EFIAPI token is an important component of that 
> > prototype. Its absence can cause unexpected issues on DEBUG systems 
> > built with GCC due to ABI mismatches.
> >
> > Both EslTcp4ConnectComplete and EslTcp6ConnectComplete did not have 
> > the EFIAPI token required of a EFI_EVENT_NOTIFY function. GCC did 
> > not catch this because of the explicit EFI_EVENT_NOTIFY cast.  By 
> > removing the cast, a build error ensues.
> >
> > This patch removes the cast and updates both functions to comply 
> > with EFI_EVENT_NOTIFY.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Thomas Palmer <thomas.palmer@hpe.com>
> > ---
> >  StdLib/EfiSocketLib/Tcp4.c | 8 ++++++--  StdLib/EfiSocketLib/Tcp6.c 
> > |
> > 8 ++++++--
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/StdLib/EfiSocketLib/Tcp4.c b/StdLib/EfiSocketLib/Tcp4.c 
> > index 68477fba6e70..8125a8d4f5ad 100644
> > --- a/StdLib/EfiSocketLib/Tcp4.c
> > +++ b/StdLib/EfiSocketLib/Tcp4.c
> > @@ -2,6 +2,7 @@
> >    Implement the TCP4 driver support for the socket layer.
> >
> >    Copyright (c) 2011 - 2015, Intel Corporation. All rights 
> > reserved.<BR>
> > +  (C) Copyright 2017 Hewlett Packard Enterprise Development LP<BR>
> >    This program and the accompanying materials are licensed and made 
> > available
> >    under the terms and conditions of the BSD License which accompanies this
> >    distribution.  The full text of the license may be found at @@
> > -192,9 +193,10 @@ EslTcp4Accept (
> >
> >  **/
> >  VOID
> > +EFIAPI
> >  EslTcp4ConnectComplete (
> >    IN EFI_EVENT Event,
> > -  IN ESL_PORT * pPort
> > +  IN VOID      *Context
> >    )
> >  {
> >    BOOLEAN bRemoveFirstPort;
> > @@ -203,12 +205,14 @@ EslTcp4ConnectComplete (
> >    ESL_SOCKET * pSocket;
> >    ESL_TCP4_CONTEXT * pTcp4;
> >    EFI_STATUS Status;
> > +  ESL_PORT * pPort;
> >
> >    DBG_ENTER ( );
> >
> >    //
> >    //  Locate the TCP context
> >    //
> > +  pPort = Context;
> >    pSocket = pPort->pSocket;
> >    pTcp4 = &pPort->Context.Tcp4;
> >
> > @@ -1288,7 +1292,7 @@ EslTcp4PortAllocate (
> >      //
> >      Status = gBS->CreateEvent (  EVT_NOTIFY_SIGNAL,
> >                                   TPL_SOCKETS,
> > -                                 (EFI_EVENT_NOTIFY)EslTcp4ConnectComplete,
> > +                                 EslTcp4ConnectComplete,
> >                                   pPort,
> >                                   &pTcp4->ConnectToken.CompletionToken.Event);
> >      if ( EFI_ERROR ( Status )) {
> > diff --git a/StdLib/EfiSocketLib/Tcp6.c b/StdLib/EfiSocketLib/Tcp6.c 
> > index 0f6d2d6ac93c..9f9c00f6dc57 100644
> > --- a/StdLib/EfiSocketLib/Tcp6.c
> > +++ b/StdLib/EfiSocketLib/Tcp6.c
> > @@ -2,6 +2,7 @@
> >    Implement the TCP6 driver support for the socket layer.
> >
> >    Copyright (c) 2011 - 2014, Intel Corporation. All rights 
> > reserved.<BR>
> > +  (C) Copyright 2017 Hewlett Packard Enterprise Development LP<BR>
> >    This program and the accompanying materials are licensed and made 
> > available
> >    under the terms and conditions of the BSD License which accompanies this
> >    distribution.  The full text of the license may be found at @@
> > -186,9 +187,10 @@ EslTcp6Accept (
> >
> >  **/
> >  VOID
> > +EFIAPI
> >  EslTcp6ConnectComplete (
> >    IN EFI_EVENT Event,
> > -  IN ESL_PORT * pPort
> > +  IN VOID      *Context
> >    )
> >  {
> >    BOOLEAN bRemoveFirstPort;
> > @@ -197,12 +199,14 @@ EslTcp6ConnectComplete (
> >    ESL_SOCKET * pSocket;
> >    ESL_TCP6_CONTEXT * pTcp6;
> >    EFI_STATUS Status;
> > +  ESL_PORT * pPort;
> >
> >    DBG_ENTER ( );
> >
> >    //
> >    //  Locate the TCP context
> >    //
> > +  pPort = Context;
> >    pSocket = pPort->pSocket;
> >    pTcp6 = &pPort->Context.Tcp6;
> >
> > @@ -1339,7 +1343,7 @@ EslTcp6PortAllocate (
> >      //
> >      Status = gBS->CreateEvent (  EVT_NOTIFY_SIGNAL,
> >                                   TPL_SOCKETS,
> > -                                 (EFI_EVENT_NOTIFY)EslTcp6ConnectComplete,
> > +                                 EslTcp6ConnectComplete,
> >                                   pPort,
> >                                   &pTcp6->ConnectToken.CompletionToken.Event);
> >      if ( EFI_ERROR ( Status )) {
> > --
> > 2.7.4