Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

[Daryl McDaniel (EDK2 Lists)]

Marco,

 

Thank you for your submission.

 

Can you please tell me which compiler you are using?

 

Thank you,

Daryl McDaniel

 

 

From: Marco Guerri [mailto:notifications@github.com] 
Sent: Wednesday, January 17, 2018 7:22 AM
To: tianocore/edk2 <edk2@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

 

When compiling in Unix environments, the ABI defaults to SysV.
Callbacks are invoked directly by the system firmware so they
must necessarily use MS ABI.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marco Guerri marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com> 

  _____  


You can view, comment on, or merge this pull request online at:


  https://github.com/tianocore/edk2/pull/123


Commit Summary


*	Stdlib: Add calling convention to completion callbacks


File Changes


*	M StdLib/EfiSocketLib/Ip4.c <https://github.com/tianocore/edk2/pull/123/files#diff-0>  (10) 
*	M StdLib/EfiSocketLib/Socket.h <https://github.com/tianocore/edk2/pull/123/files#diff-1>  (24) 
*	M StdLib/EfiSocketLib/Tcp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-2>  (11) 
*	M StdLib/EfiSocketLib/Tcp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-3>  (3) 
*	M StdLib/EfiSocketLib/Udp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-4>  (30) 
*	M StdLib/EfiSocketLib/Udp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-5>  (20) 


Patch Links:


*	https://github.com/tianocore/edk2/pull/123.patch
*	https://github.com/tianocore/edk2/pull/123.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <https://github.com/tianocore/edk2/pull/123> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AM0XkaIxBOrC6Kacm-Vqq6z4tgQFar04ks5tLhAHgaJpZM4RhgXz> .  <https://github.com/notifications/beacon/AM0XkfL19zucGfgYUKWjci6NI8umTOaaks5tLhAHgaJpZM4RhgXz.gif> 

Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

[Marco Guerri]

Hi Daryl,

First, sorry for not having RTFM and submitting the patch via github. I
was planning to read your workflow during the weekend and re-submit via
the mailing list.
Second, I am using gcc. I tried gcc 4.8 and 5.2 and with both, my
applications using Stdlib badly hang. I dug a bit and discovered that
the signatures of the completion callbacks are missing EFIAPI, so
necessarily under Unix systems they are using the wrong ABI. When the
functions arguments are pointers, the machine ends up in a triple fault
I guess (I can clearly see that the first two integer parameters are
fetched from %rdi and %rsi, which is the SysV ABI). I was a bit
surprised to see this bug, as the buildscripts are pretty comprehensive
in terms of compilers support. Does this make sense to you?
Cheers,
Marco

--
  Marco Guerri
  marco.guerri.dev@fastmail.com



On Thu, Jan 18, 2018, at 11:12 AM, Daryl McDaniel (EDK2 Lists) wrote:
> Marco,


>  


> Thank you for your submission.


>  


> Can you please tell me which compiler you are using?


>  


> Thank you,


> Daryl McDaniel


>  


>  


> **From:** Marco Guerri [mailto:notifications@github.com] **Sent:**
> Wednesday, January 17, 2018 7:22 AM **To:** tianocore/edk2
> <edk2@noreply.github.com> **Cc:** Subscribed
> <subscribed@noreply.github.com> **Subject:** [tianocore/edk2] Stdlib:
> Add calling convention to completion callbacks (#123)>  


> When compiling in Unix environments, the ABI defaults to SysV.
> Callbacks are invoked directly by the system firmware so they must
> necessarily use MS ABI.> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by:
> Marco Guerri marco.guerri.dev@fastmail.com> 
> *You can view, comment on, or merge this pull request online at:*


>   https://github.com/tianocore/edk2/pull/123


> *Commit Summary*


>  * Stdlib: Add calling convention to completion callbacks> *File Changes*


>  * **M** StdLib/EfiSocketLib/Ip4.c[1] (10)
>  * **M** StdLib/EfiSocketLib/Socket.h[2] (24)
>  * **M** StdLib/EfiSocketLib/Tcp4.c[3] (11)
>  * **M** StdLib/EfiSocketLib/Tcp6.c[4] (3)
>  * **M** StdLib/EfiSocketLib/Udp4.c[5] (30)
>  * **M** StdLib/EfiSocketLib/Udp6.c[6] (20)> *Patch Links:*


>  * https://github.com/tianocore/edk2/pull/123.patch
>  * https://github.com/tianocore/edk2/pull/123.diff> — You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub[7], or mute the
> thread[8].

Links:

  1. https://github.com/tianocore/edk2/pull/123/files#diff-0
  2. https://github.com/tianocore/edk2/pull/123/files#diff-1
  3. https://github.com/tianocore/edk2/pull/123/files#diff-2
  4. https://github.com/tianocore/edk2/pull/123/files#diff-3
  5. https://github.com/tianocore/edk2/pull/123/files#diff-4
  6. https://github.com/tianocore/edk2/pull/123/files#diff-5
  7. https://github.com/tianocore/edk2/pull/123
  8. https://github.com/notifications/unsubscribe-auth/AM0XkaIxBOrC6Kacm-Vqq6z4tgQFar04ks5tLhAHgaJpZM4RhgXz

Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

[Palmer, Thomas]

Daryl,

	Thanks for your patches!  

	Have you compiled your changes with CLANG38? In my sandbox (which, admittedly I may have stale CLANG38 compiler options), the second argument must be a VOID* type or the compiler is upset.  CLANG can be more strict here than GCC


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: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daryl McDaniel (EDK2 Lists)
Sent: Thursday, January 18, 2018 11:12 AM
To: edk2-devel@lists.01.org
Subject: Re: [edk2] [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

Marco,

 

Thank you for your submission.

 

Can you please tell me which compiler you are using?

 

Thank you,

Daryl McDaniel

 

 

From: Marco Guerri [mailto:notifications@github.com]
Sent: Wednesday, January 17, 2018 7:22 AM
To: tianocore/edk2 <edk2@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

 

When compiling in Unix environments, the ABI defaults to SysV.
Callbacks are invoked directly by the system firmware so they must necessarily use MS ABI.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marco Guerri marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com> 

  _____  


You can view, comment on, or merge this pull request online at:


  https://github.com/tianocore/edk2/pull/123


Commit Summary


*	Stdlib: Add calling convention to completion callbacks


File Changes


*	M StdLib/EfiSocketLib/Ip4.c <https://github.com/tianocore/edk2/pull/123/files#diff-0>  (10) 
*	M StdLib/EfiSocketLib/Socket.h <https://github.com/tianocore/edk2/pull/123/files#diff-1>  (24) 
*	M StdLib/EfiSocketLib/Tcp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-2>  (11) 
*	M StdLib/EfiSocketLib/Tcp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-3>  (3) 
*	M StdLib/EfiSocketLib/Udp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-4>  (30) 
*	M StdLib/EfiSocketLib/Udp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-5>  (20) 


Patch Links:


*	https://github.com/tianocore/edk2/pull/123.patch
*	https://github.com/tianocore/edk2/pull/123.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <https://github.com/tianocore/edk2/pull/123> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AM0XkaIxBOrC6Kacm-Vqq6z4tgQFar04ks5tLhAHgaJpZM4RhgXz> .  <https://github.com/notifications/beacon/AM0XkfL19zucGfgYUKWjci6NI8umTOaaks5tLhAHgaJpZM4RhgXz.gif> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

[Daryl McDaniel (EDK2 Lists)]

Marco,

 

The tools definitions for the newer versions of GCC must not be setting the option to force the EFIAPI calling conventions.

We relied upon the compiler option for StdLib in order to enhance portability for POSIX sources.

 

I’ll research what the tools definitions are for the newer GCC versions then readdress your patches.

 

Thank you,

Daryl McDaniel

 

 

From: Marco Guerri [mailto:marco.guerri.dev@fastmail.com] 
Sent: Thursday, January 18, 2018 9:28 AM
To: Daryl McDaniel (EDK2 Lists) <edk2-lists@mc2research.org>; edk2-devel@lists.01.org
Subject: Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

 

Hi Daryl,

 

First, sorry for not having RTFM and submitting the patch via github. I was planning to read your workflow during the weekend and re-submit via the mailing list.

 

Second, I am using gcc. I tried gcc 4.8 and 5.2 and with both, my applications using Stdlib badly hang. I dug a bit and discovered that the signatures of the completion callbacks are missing EFIAPI, so necessarily under Unix systems they are using the wrong ABI. When the functions arguments are pointers, the machine ends up in a triple fault I guess (I can clearly see that the first two integer parameters are fetched from %rdi and %rsi, which is the SysV ABI). I was a bit surprised to see this bug, as the buildscripts are pretty comprehensive in terms of compilers support. Does this make sense to you?

 

Cheers,

Marco

 

--

  Marco Guerri

  marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com> 

 

 

 

On Thu, Jan 18, 2018, at 11:12 AM, Daryl McDaniel (EDK2 Lists) wrote:

Marco,

 

Thank you for your submission.

 

Can you please tell me which compiler you are using?

 

Thank you,

Daryl McDaniel

 

 

From: Marco Guerri [mailto:notifications@github.com] 
Sent: Wednesday, January 17, 2018 7:22 AM
To: tianocore/edk2 <edk2@noreply.github.com <mailto:edk2@noreply.github.com> >
Cc: Subscribed <subscribed@noreply.github.com <mailto:subscribed@noreply.github.com> >
Subject: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

 

When compiling in Unix environments, the ABI defaults to SysV.
Callbacks are invoked directly by the system firmware so they
must necessarily use MS ABI.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marco Guerri marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com> 


  _____  


 


You can view, comment on, or merge this pull request online at:


  https://github.com/tianocore/edk2/pull/123


Commit Summary


*	Stdlib: Add calling convention to completion callbacks


File Changes


*	M StdLib/EfiSocketLib/Ip4.c <https://github.com/tianocore/edk2/pull/123/files#diff-0>  (10)
*	M StdLib/EfiSocketLib/Socket.h <https://github.com/tianocore/edk2/pull/123/files#diff-1>  (24)
*	M StdLib/EfiSocketLib/Tcp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-2>  (11)
*	M StdLib/EfiSocketLib/Tcp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-3>  (3)
*	M StdLib/EfiSocketLib/Udp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-4>  (30)
*	M StdLib/EfiSocketLib/Udp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-5>  (20)


Patch Links:


*	https://github.com/tianocore/edk2/pull/123.patch
*	https://github.com/tianocore/edk2/pull/123.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <https://github.com/tianocore/edk2/pull/123> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AM0XkaIxBOrC6Kacm-Vqq6z4tgQFar04ks5tLhAHgaJpZM4RhgXz> .  <https://www.fastmailusercontent.com/proxy/f37b3dc5910ce7528e4a533ff667a08359273c5576df3e81fc020a90540143e0/8647470737a3f2f2769647865726e236f6d6f2e6f64796669636164796f6e637f226561636f6e6f214d40385b666c41393a757367466769555b475a6369663e4948357d645f41616b6375347c48614847616a407a5d44325867685a7e2769666/AM0XkfL19zucGfgYUKWjci6NI8umTOaaks5tLhAHgaJpZM4RhgXz.gif> 

 

Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)

[Marco Guerri]

Hi Daryl,

Thank you for your feedback. However, I must admit I don't completely
follow your argument. Could you please elaborate on that? I assume we
agree that those callbacks must explicitly use ms_abi calling convention when
compiled with gcc, and the way to do this is by adding __attribute__((ms_abi))
to the signature, which the build system does via EFIAPI macro (and
several other functions of the StdLib do so).

The build system supports up to GCC5, and gcc 5.4 is what it was tested
with (admittedly, I didn't have clang nor a Windows system available, so my
test plan was not very comprehensive).

Thank you,
Marco
On Fri 19/01 17:51, Daryl McDaniel (EDK2 Lists) wrote:
> Marco,
>
>
>
> The tools definitions for the newer versions of GCC must not be setting the option to force the EFIAPI calling conventions.
>
> We relied upon the compiler option for StdLib in order to enhance portability for POSIX sources.
>
>
>
> I’ll research what the tools definitions are for the newer GCC versions then readdress your patches.
>
>
>
> Thank you,
>
> Daryl McDaniel
>
>
>
>
>
> From: Marco Guerri [mailto:marco.guerri.dev@fastmail.com]
> Sent: Thursday, January 18, 2018 9:28 AM
> To: Daryl McDaniel (EDK2 Lists) <edk2-lists@mc2research.org>; edk2-devel@lists.01.org
> Subject: Re: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)
>
>
>
> Hi Daryl,
>
>
>
> First, sorry for not having RTFM and submitting the patch via github. I was planning to read your workflow during the weekend and re-submit via the mailing list.
>
>
>
> Second, I am using gcc. I tried gcc 4.8 and 5.2 and with both, my applications using Stdlib badly hang. I dug a bit and discovered that the signatures of the completion callbacks are missing EFIAPI, so necessarily under Unix systems they are using the wrong ABI. When the functions arguments are pointers, the machine ends up in a triple fault I guess (I can clearly see that the first two integer parameters are fetched from %rdi and %rsi, which is the SysV ABI). I was a bit surprised to see this bug, as the buildscripts are pretty comprehensive in terms of compilers support. Does this make sense to you?
>
>
>
> Cheers,
>
> Marco
>
>
>
> --
>
>   Marco Guerri
>
>   marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com>
>
>
>
>
>
>
>
> On Thu, Jan 18, 2018, at 11:12 AM, Daryl McDaniel (EDK2 Lists) wrote:
>
> Marco,
>
>
>
> Thank you for your submission.
>
>
>
> Can you please tell me which compiler you are using?
>
>
>
> Thank you,
>
> Daryl McDaniel
>
>
>
>
>
> From: Marco Guerri [mailto:notifications@github.com]
> Sent: Wednesday, January 17, 2018 7:22 AM
> To: tianocore/edk2 <edk2@noreply.github.com <mailto:edk2@noreply.github.com> >
> Cc: Subscribed <subscribed@noreply.github.com <mailto:subscribed@noreply.github.com> >
> Subject: [tianocore/edk2] Stdlib: Add calling convention to completion callbacks (#123)
>
>
>
> When compiling in Unix environments, the ABI defaults to SysV.
> Callbacks are invoked directly by the system firmware so they
> must necessarily use MS ABI.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marco Guerri marco.guerri.dev@fastmail.com <mailto:marco.guerri.dev@fastmail.com>
>
>
>   _____
>
>
>
>
>
> You can view, comment on, or merge this pull request online at:
>
>
>   https://github.com/tianocore/edk2/pull/123
>
>
> Commit Summary
>
>
> *	Stdlib: Add calling convention to completion callbacks
>
>
> File Changes
>
>
> *	M StdLib/EfiSocketLib/Ip4.c <https://github.com/tianocore/edk2/pull/123/files#diff-0>  (10)
> *	M StdLib/EfiSocketLib/Socket.h <https://github.com/tianocore/edk2/pull/123/files#diff-1>  (24)
> *	M StdLib/EfiSocketLib/Tcp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-2>  (11)
> *	M StdLib/EfiSocketLib/Tcp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-3>  (3)
> *	M StdLib/EfiSocketLib/Udp4.c <https://github.com/tianocore/edk2/pull/123/files#diff-4>  (30)
> *	M StdLib/EfiSocketLib/Udp6.c <https://github.com/tianocore/edk2/pull/123/files#diff-5>  (20)
>
>
> Patch Links:
>
>
> *	https://github.com/tianocore/edk2/pull/123.patch
> *	https://github.com/tianocore/edk2/pull/123.diff
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub <https://github.com/tianocore/edk2/pull/123> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AM0XkaIxBOrC6Kacm-Vqq6z4tgQFar04ks5tLhAHgaJpZM4RhgXz> .  <https://www.fastmailusercontent.com/proxy/f37b3dc5910ce7528e4a533ff667a08359273c5576df3e81fc020a90540143e0/8647470737a3f2f2769647865726e236f6d6f2e6f64796669636164796f6e637f226561636f6e6f214d40385b666c41393a757367466769555b475a6369663e4948357d645f41616b6375347c48614847616a407a5d44325867685a7e2769666/AM0XkfL19zucGfgYUKWjci6NI8umTOaaks5tLhAHgaJpZM4RhgXz.gif>
>
>
>