* [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h [not found] <1667068483.2112668.1512898346914.ref@mail.yahoo.com> @ 2017-12-10 9:32 ` Zenith432 2017-12-10 13:53 ` Gao, Liming 2017-12-12 9:16 ` Laszlo Ersek 0 siblings, 2 replies; 12+ messages in thread From: Zenith432 @ 2017-12-10 9:32 UTC (permalink / raw) To: edk2-devel; +Cc: michael.d.kinney, liming.gao This is to resolve bug 457. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> --- MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index 02140a5a..19f36872 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -560,13 +560,14 @@ struct _LIST_ENTRY { // VA_LIST - typedef for argument list. // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. // VA_END (VA_LIST Marker) - Clear Marker -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from -// the ... list. You must know the size and pass it in this macro. +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from +// the ... list. You must know the type and pass it in this macro. // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. // -// example: +// Example: // // UINTN +// EFIAPI // ExampleVarArg ( // IN UINTN NumberOfArgs, // ... @@ -582,7 +583,7 @@ struct _LIST_ENTRY { // VA_START (Marker, NumberOfArgs); // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { // // -// // The ... list is a series of UINTN values, so average them up. +// // The ... list is a series of UINTN values, so sum them up. // // // Result += VA_ARG (Marker, UINTN); // } @@ -591,6 +592,21 @@ struct _LIST_ENTRY { // return Result // } // +// Notes: +// +// This set of macros is intended to support variadic functions that +// use the EFIAPI calling convention. Variadic functions that use a +// native calling convention should use stdarg.h. +// In particular: +// -- VA_START may only be used in a variadic EFIAPI function. +// -- va_start may only be used in a variadic native function. +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. +// -- Both VA_LIST or va_list may be passed as arguments to functions +// of either EFIAPI or native calling conventions. +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used +// in functions of either calling conventions. +// /** Return the size of argument that has been aligned to sizeof (UINTN). -- 2.14.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-10 9:32 ` [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h Zenith432 @ 2017-12-10 13:53 ` Gao, Liming 2017-12-10 15:43 ` Zenith432 2017-12-12 9:16 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Gao, Liming @ 2017-12-10 13:53 UTC (permalink / raw) To: Zenith432, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Could you add bug 457 link in the commit message? > -----Original Message----- > From: Zenith432 [mailto:zenith432@users.sourceforge.net] > Sent: Sunday, December 10, 2017 5:32 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > > This is to resolve bug 457. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > --- > MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index 02140a5a..19f36872 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -560,13 +560,14 @@ struct _LIST_ENTRY { > // VA_LIST - typedef for argument list. > // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. > // VA_END (VA_LIST Marker) - Clear Marker > -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from > -// the ... list. You must know the size and pass it in this macro. > +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from > +// the ... list. You must know the type and pass it in this macro. > // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. > // > -// example: > +// Example: > // > // UINTN > +// EFIAPI > // ExampleVarArg ( > // IN UINTN NumberOfArgs, > // ... > @@ -582,7 +583,7 @@ struct _LIST_ENTRY { > // VA_START (Marker, NumberOfArgs); > // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > // // > -// // The ... list is a series of UINTN values, so average them up. > +// // The ... list is a series of UINTN values, so sum them up. > // // > // Result += VA_ARG (Marker, UINTN); > // } > @@ -591,6 +592,21 @@ struct _LIST_ENTRY { > // return Result > // } > // > +// Notes: > +// > +// This set of macros is intended to support variadic functions that > +// use the EFIAPI calling convention. Variadic functions that use a > +// native calling convention should use stdarg.h. > +// In particular: > +// -- VA_START may only be used in a variadic EFIAPI function. > +// -- va_start may only be used in a variadic native function. > +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. > +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. > +// -- Both VA_LIST or va_list may be passed as arguments to functions > +// of either EFIAPI or native calling conventions. > +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used > +// in functions of either calling conventions. > +// > > /** > Return the size of argument that has been aligned to sizeof (UINTN). > -- > 2.14.3 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-10 13:53 ` Gao, Liming @ 2017-12-10 15:43 ` Zenith432 2017-12-11 14:35 ` Gao, Liming 0 siblings, 1 reply; 12+ messages in thread From: Zenith432 @ 2017-12-10 15:43 UTC (permalink / raw) To: Gao, Liming, edk2-devel@lists.01.org; +Cc: Kinney, Michael D On 10/12/2017 03:53 PM, Gao, Liming wrote: > Could you add bug 457 link in the commit message? --- Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h This is to resolve bug 457. https://bugzilla.tianocore.org/show_bug.cgi?id=457 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> --- MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index 02140a5a..19f36872 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -560,13 +560,14 @@ struct _LIST_ENTRY { // VA_LIST - typedef for argument list. // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. // VA_END (VA_LIST Marker) - Clear Marker -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from -// the ... list. You must know the size and pass it in this macro. +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from +// the ... list. You must know the type and pass it in this macro. // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. // -// example: +// Example: // // UINTN +// EFIAPI // ExampleVarArg ( // IN UINTN NumberOfArgs, // ... @@ -582,7 +583,7 @@ struct _LIST_ENTRY { // VA_START (Marker, NumberOfArgs); // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { // // -// // The ... list is a series of UINTN values, so average them up. +// // The ... list is a series of UINTN values, so sum them up. // // // Result += VA_ARG (Marker, UINTN); // } @@ -591,6 +592,21 @@ struct _LIST_ENTRY { // return Result // } // +// Notes: +// +// This set of macros is intended to support variadic functions that +// use the EFIAPI calling convention. Variadic functions that use a +// native calling convention should use stdarg.h. +// In particular: +// -- VA_START may only be used in a variadic EFIAPI function. +// -- va_start may only be used in a variadic native function. +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. +// -- Both VA_LIST or va_list may be passed as arguments to functions +// of either EFIAPI or native calling conventions. +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used +// in functions of either calling conventions. +// /** Return the size of argument that has been aligned to sizeof (UINTN). -- 2.14.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-10 15:43 ` Zenith432 @ 2017-12-11 14:35 ` Gao, Liming 2017-12-11 14:57 ` Zenith432 0 siblings, 1 reply; 12+ messages in thread From: Gao, Liming @ 2017-12-11 14:35 UTC (permalink / raw) To: Zenith432, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Hi, I see you and Laszlo are still in discussion on GCC behavior. Because there is one bug in GCC compiler, the following functions in edk2 must be EFIAPI. Right? - functions that have a variable argument list and call VA_START / VA_END - functions that call VA_COPY Thanks Liming > -----Original Message----- > From: Zenith432 [mailto:zenith432@users.sourceforge.net] > Sent: Sunday, December 10, 2017 11:43 PM > To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > > On 10/12/2017 03:53 PM, Gao, Liming wrote: > > Could you add bug 457 link in the commit message? > --- > Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > > This is to resolve bug 457. > https://bugzilla.tianocore.org/show_bug.cgi?id=457 > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > --- > MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index 02140a5a..19f36872 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -560,13 +560,14 @@ struct _LIST_ENTRY { > // VA_LIST - typedef for argument list. > // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. > // VA_END (VA_LIST Marker) - Clear Marker > -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from > -// the ... list. You must know the size and pass it in this macro. > +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from > +// the ... list. You must know the type and pass it in this macro. > // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. > // > -// example: > +// Example: > // > // UINTN > +// EFIAPI > // ExampleVarArg ( > // IN UINTN NumberOfArgs, > // ... > @@ -582,7 +583,7 @@ struct _LIST_ENTRY { > // VA_START (Marker, NumberOfArgs); > // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > // // > -// // The ... list is a series of UINTN values, so average them up. > +// // The ... list is a series of UINTN values, so sum them up. > // // > // Result += VA_ARG (Marker, UINTN); > // } > @@ -591,6 +592,21 @@ struct _LIST_ENTRY { > // return Result > // } > // > +// Notes: > +// > +// This set of macros is intended to support variadic functions that > +// use the EFIAPI calling convention. Variadic functions that use a > +// native calling convention should use stdarg.h. > +// In particular: > +// -- VA_START may only be used in a variadic EFIAPI function. > +// -- va_start may only be used in a variadic native function. > +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. > +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. > +// -- Both VA_LIST or va_list may be passed as arguments to functions > +// of either EFIAPI or native calling conventions. > +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used > +// in functions of either calling conventions. > +// > > /** > Return the size of argument that has been aligned to sizeof (UINTN). > -- > 2.14.3 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-11 14:35 ` Gao, Liming @ 2017-12-11 14:57 ` Zenith432 2017-12-12 1:32 ` Gao, Liming 0 siblings, 1 reply; 12+ messages in thread From: Zenith432 @ 2017-12-11 14:57 UTC (permalink / raw) To: Gao, Liming, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Yes, that's right, in mixed ABI on GCC 7.2 (which is what GCC5 toolchain in tools_def.sample creates...), VA_COPY when used inside native (non-EFIAPI) function causes incorrect code generation that leads to crash (it mistreats the __builtin_ms_va_list as a __builtin_sysv_va_list). The same source code works in current versions of LLVM and Apple clang - so it's a compiler-specific bug. The text I wrote >> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used >> +// in functions of either calling conventions. is the hypothetical desired condition, but it depends on compilers generating proper code in mixed ABI. It obviously does not hold for current GCC. However, the part of the documentation to add EFIAPI to ExampleVarArg should definitely be fixed. Using __builtin_ms_va_list (=VA_LIST) in a sysv_abi (native) function probably yields a compile-time diagnostic, and if not - generates code that crashes for sure. On 11/12/2017 04:35 PM, Gao, Liming wrote: > Hi, > I see you and Laszlo are still in discussion on GCC behavior. Because there is one bug in GCC compiler, the following functions in edk2 must be EFIAPI. Right? > > - functions that have a variable argument list and call VA_START / VA_END > - functions that call VA_COPY > > Thanks > Liming >> -----Original Message----- >> From: Zenith432 [mailto:zenith432@users.sourceforge.net] >> Sent: Sunday, December 10, 2017 11:43 PM >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kinney@intel.com> >> Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h >> >> On 10/12/2017 03:53 PM, Gao, Liming wrote: >>> Could you add bug 457 link in the commit message? >> --- >> Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h >> >> This is to resolve bug 457. >> https://bugzilla.tianocore.org/show_bug.cgi?id=457 >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> >> --- >> MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h >> index 02140a5a..19f36872 100644 >> --- a/MdePkg/Include/Base.h >> +++ b/MdePkg/Include/Base.h >> @@ -560,13 +560,14 @@ struct _LIST_ENTRY { >> // VA_LIST - typedef for argument list. >> // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. >> // VA_END (VA_LIST Marker) - Clear Marker >> -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from >> -// the ... list. You must know the size and pass it in this macro. >> +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from >> +// the ... list. You must know the type and pass it in this macro. >> // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. >> // >> -// example: >> +// Example: >> // >> // UINTN >> +// EFIAPI >> // ExampleVarArg ( >> // IN UINTN NumberOfArgs, >> // ... >> @@ -582,7 +583,7 @@ struct _LIST_ENTRY { >> // VA_START (Marker, NumberOfArgs); >> // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { >> // // >> -// // The ... list is a series of UINTN values, so average them up. >> +// // The ... list is a series of UINTN values, so sum them up. >> // // >> // Result += VA_ARG (Marker, UINTN); >> // } >> @@ -591,6 +592,21 @@ struct _LIST_ENTRY { >> // return Result >> // } >> // >> +// Notes: >> +// >> +// This set of macros is intended to support variadic functions that >> +// use the EFIAPI calling convention. Variadic functions that use a >> +// native calling convention should use stdarg.h. >> +// In particular: >> +// -- VA_START may only be used in a variadic EFIAPI function. >> +// -- va_start may only be used in a variadic native function. >> +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. >> +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. >> +// -- Both VA_LIST or va_list may be passed as arguments to functions >> +// of either EFIAPI or native calling conventions. >> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used >> +// in functions of either calling conventions. >> +// >> >> /** >> Return the size of argument that has been aligned to sizeof (UINTN). >> -- >> 2.14.3 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-11 14:57 ` Zenith432 @ 2017-12-12 1:32 ` Gao, Liming 2017-12-12 8:36 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Gao, Liming @ 2017-12-12 1:32 UTC (permalink / raw) To: Zenith432, edk2-devel@lists.01.org, Laszlo Ersek (lersek@redhat.com) Cc: Kinney, Michael D Laszlo: Have you any comments for this patch? Seemly, you discussed this topic in bugzillar. Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zenith432 > Sent: Monday, December 11, 2017 10:58 PM > To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > > Yes, that's right, in mixed ABI on GCC 7.2 (which is what GCC5 toolchain in tools_def.sample creates...), VA_COPY when > used inside native (non-EFIAPI) function causes incorrect code generation that leads to crash (it mistreats the > __builtin_ms_va_list as a __builtin_sysv_va_list). > > The same source code works in current versions of LLVM and Apple clang - so it's a compiler-specific bug. > > The text I wrote > >> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used > >> +// in functions of either calling conventions. > > is the hypothetical desired condition, but it depends on compilers generating proper code in mixed ABI. It obviously > does not hold for current GCC. > > However, the part of the documentation to add EFIAPI to ExampleVarArg should definitely be fixed. Using > __builtin_ms_va_list (=VA_LIST) in a sysv_abi (native) function probably yields a compile-time diagnostic, and if not - > generates code that crashes for sure. > > On 11/12/2017 04:35 PM, Gao, Liming wrote: > > Hi, > > I see you and Laszlo are still in discussion on GCC behavior. Because there is one bug in GCC compiler, the following functions in > edk2 must be EFIAPI. Right? > > > > - functions that have a variable argument list and call VA_START / VA_END > > - functions that call VA_COPY > > > > Thanks > > Liming > >> -----Original Message----- > >> From: Zenith432 [mailto:zenith432@users.sourceforge.net] > >> Sent: Sunday, December 10, 2017 11:43 PM > >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org > >> Cc: Kinney, Michael D <michael.d.kinney@intel.com> > >> Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > >> > >> On 10/12/2017 03:53 PM, Gao, Liming wrote: > >>> Could you add bug 457 link in the commit message? > >> --- > >> Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > >> > >> This is to resolve bug 457. > >> https://bugzilla.tianocore.org/show_bug.cgi?id=457 > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > >> --- > >> MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- > >> 1 file changed, 20 insertions(+), 4 deletions(-) > >> > >> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > >> index 02140a5a..19f36872 100644 > >> --- a/MdePkg/Include/Base.h > >> +++ b/MdePkg/Include/Base.h > >> @@ -560,13 +560,14 @@ struct _LIST_ENTRY { > >> // VA_LIST - typedef for argument list. > >> // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. > >> // VA_END (VA_LIST Marker) - Clear Marker > >> -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from > >> -// the ... list. You must know the size and pass it in this macro. > >> +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from > >> +// the ... list. You must know the type and pass it in this macro. > >> // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. > >> // > >> -// example: > >> +// Example: > >> // > >> // UINTN > >> +// EFIAPI > >> // ExampleVarArg ( > >> // IN UINTN NumberOfArgs, > >> // ... > >> @@ -582,7 +583,7 @@ struct _LIST_ENTRY { > >> // VA_START (Marker, NumberOfArgs); > >> // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > >> // // > >> -// // The ... list is a series of UINTN values, so average them up. > >> +// // The ... list is a series of UINTN values, so sum them up. > >> // // > >> // Result += VA_ARG (Marker, UINTN); > >> // } > >> @@ -591,6 +592,21 @@ struct _LIST_ENTRY { > >> // return Result > >> // } > >> // > >> +// Notes: > >> +// > >> +// This set of macros is intended to support variadic functions that > >> +// use the EFIAPI calling convention. Variadic functions that use a > >> +// native calling convention should use stdarg.h. > >> +// In particular: > >> +// -- VA_START may only be used in a variadic EFIAPI function. > >> +// -- va_start may only be used in a variadic native function. > >> +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. > >> +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. > >> +// -- Both VA_LIST or va_list may be passed as arguments to functions > >> +// of either EFIAPI or native calling conventions. > >> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used > >> +// in functions of either calling conventions. > >> +// > >> > >> /** > >> Return the size of argument that has been aligned to sizeof (UINTN). > >> -- > >> 2.14.3 > > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-12 1:32 ` Gao, Liming @ 2017-12-12 8:36 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2017-12-12 8:36 UTC (permalink / raw) To: Gao, Liming, Zenith432, edk2-devel@lists.01.org; +Cc: Kinney, Michael D On 12/12/17 02:32, Gao, Liming wrote: > Laszlo: > Have you any comments for this patch? Seemly, you discussed this topic in bugzillar. Thanks for the ping, I'll comment under the thread starter message. Thanks! Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Zenith432 >> Sent: Monday, December 11, 2017 10:58 PM >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h >> >> Yes, that's right, in mixed ABI on GCC 7.2 (which is what GCC5 toolchain in tools_def.sample creates...), VA_COPY when >> used inside native (non-EFIAPI) function causes incorrect code generation that leads to crash (it mistreats the >> __builtin_ms_va_list as a __builtin_sysv_va_list). >> >> The same source code works in current versions of LLVM and Apple clang - so it's a compiler-specific bug. >> >> The text I wrote >> >> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used >> >> +// in functions of either calling conventions. >> >> is the hypothetical desired condition, but it depends on compilers generating proper code in mixed ABI. It obviously >> does not hold for current GCC. >> >> However, the part of the documentation to add EFIAPI to ExampleVarArg should definitely be fixed. Using >> __builtin_ms_va_list (=VA_LIST) in a sysv_abi (native) function probably yields a compile-time diagnostic, and if not - >> generates code that crashes for sure. >> >> On 11/12/2017 04:35 PM, Gao, Liming wrote: >>> Hi, >>> I see you and Laszlo are still in discussion on GCC behavior. Because there is one bug in GCC compiler, the following functions in >> edk2 must be EFIAPI. Right? >>> >>> - functions that have a variable argument list and call VA_START / VA_END >>> - functions that call VA_COPY >>> >>> Thanks >>> Liming >>>> -----Original Message----- >>>> From: Zenith432 [mailto:zenith432@users.sourceforge.net] >>>> Sent: Sunday, December 10, 2017 11:43 PM >>>> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org >>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com> >>>> Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h >>>> >>>> On 10/12/2017 03:53 PM, Gao, Liming wrote: >>>>> Could you add bug 457 link in the commit message? >>>> --- >>>> Subject: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h >>>> >>>> This is to resolve bug 457. >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=457 >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> >>>> --- >>>> MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- >>>> 1 file changed, 20 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h >>>> index 02140a5a..19f36872 100644 >>>> --- a/MdePkg/Include/Base.h >>>> +++ b/MdePkg/Include/Base.h >>>> @@ -560,13 +560,14 @@ struct _LIST_ENTRY { >>>> // VA_LIST - typedef for argument list. >>>> // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. >>>> // VA_END (VA_LIST Marker) - Clear Marker >>>> -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from >>>> -// the ... list. You must know the size and pass it in this macro. >>>> +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from >>>> +// the ... list. You must know the type and pass it in this macro. >>>> // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. >>>> // >>>> -// example: >>>> +// Example: >>>> // >>>> // UINTN >>>> +// EFIAPI >>>> // ExampleVarArg ( >>>> // IN UINTN NumberOfArgs, >>>> // ... >>>> @@ -582,7 +583,7 @@ struct _LIST_ENTRY { >>>> // VA_START (Marker, NumberOfArgs); >>>> // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { >>>> // // >>>> -// // The ... list is a series of UINTN values, so average them up. >>>> +// // The ... list is a series of UINTN values, so sum them up. >>>> // // >>>> // Result += VA_ARG (Marker, UINTN); >>>> // } >>>> @@ -591,6 +592,21 @@ struct _LIST_ENTRY { >>>> // return Result >>>> // } >>>> // >>>> +// Notes: >>>> +// >>>> +// This set of macros is intended to support variadic functions that >>>> +// use the EFIAPI calling convention. Variadic functions that use a >>>> +// native calling convention should use stdarg.h. >>>> +// In particular: >>>> +// -- VA_START may only be used in a variadic EFIAPI function. >>>> +// -- va_start may only be used in a variadic native function. >>>> +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. >>>> +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. >>>> +// -- Both VA_LIST or va_list may be passed as arguments to functions >>>> +// of either EFIAPI or native calling conventions. >>>> +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used >>>> +// in functions of either calling conventions. >>>> +// >>>> >>>> /** >>>> Return the size of argument that has been aligned to sizeof (UINTN). >>>> -- >>>> 2.14.3 >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-10 9:32 ` [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h Zenith432 2017-12-10 13:53 ` Gao, Liming @ 2017-12-12 9:16 ` Laszlo Ersek 2017-12-12 10:24 ` Zenith432 1 sibling, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2017-12-12 9:16 UTC (permalink / raw) To: Zenith432, edk2-devel; +Cc: michael.d.kinney, liming.gao Hi, On 12/10/17 10:32, Zenith432 wrote: > This is to resolve bug 457. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > --- > MdePkg/Include/Base.h | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) I'm commenting on this patch because Liming asked me to. I'm stating my opinion below; remember that I'm not an MdePkg maintainer. > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index 02140a5a..19f36872 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -560,13 +560,14 @@ struct _LIST_ENTRY { > // VA_LIST - typedef for argument list. > // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. > // VA_END (VA_LIST Marker) - Clear Marker > -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from > -// the ... list. You must know the size and pass it in this macro. > +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from > +// the ... list. You must know the type and pass it in this macro. > // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. (1) I suggest adding "Type must be compatible with the type of the actual next argument (as promoted according to the default argument promotions)". (This is not the full story either, but it would still remain short enough, and be an improvement.) > // > -// example: > +// Example: > // > // UINTN > +// EFIAPI > // ExampleVarArg ( > // IN UINTN NumberOfArgs, > // ... Correct. > @@ -582,7 +583,7 @@ struct _LIST_ENTRY { > // VA_START (Marker, NumberOfArgs); > // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > // // > -// // The ... list is a series of UINTN values, so average them up. > +// // The ... list is a series of UINTN values, so sum them up. > // // > // Result += VA_ARG (Marker, UINTN); > // } > @@ -591,6 +592,21 @@ struct _LIST_ENTRY { > // return Result (2) Maybe append a semicolon? (Not strictly the topic of this patch, but we're fixing up other typos anyway.) > // } > // > +// Notes: > +// > +// This set of macros is intended to support variadic functions that > +// use the EFIAPI calling convention. Variadic functions that use a > +// native calling convention should use stdarg.h. I disagree with this wording. "MdePkg/Include/Base.h" should be very careful about referencing dependent packages, even in comments. The only "stdarg.h" file in the tree (that is meant to be included by client code directly) is StdLib/Include/stdarg.h I'll comment on that lower down. > +// In particular: > +// -- VA_START may only be used in a variadic EFIAPI function. > +// -- va_start may only be used in a variadic native function. > +// -- VA_START, VA_END, VA_ARG and VA_COPY may only be used on a VA_LIST. > +// -- va_start, va_end, va_arg and va_copy may only be used on a va_list. > +// -- Both VA_LIST or va_list may be passed as arguments to functions > +// of either EFIAPI or native calling conventions. > +// -- VA_END, VA_ARG, VA_COPY, va_end, va_arg, and va_copy may be used > +// in functions of either calling conventions. > +// I would drop the language on the lower-case va_* macros and the native calling convention altogether. In Base.h we should give *practical* instructions for using variable argument lists with *these* macros (i.e. the macros from Base.h), such that they work with any edk2-supported toolchain. (3) So, as I wrote in the BZ, I would replace this Notes section with: Notes: - Functions that have a variable argument list and call VA_START() / VA_END() must be declared EFIAPI. - Functions that call VA_COPY() must be declared EFIAPI. - Functions that only use VA_LIST and VA_ARG() need not be EFIAPI. Now, if we want to consider any interactions with StdLib/Include/stdarg.h plus other standard C functions that take a variable argument list, then any such considerations should be added to "StdLib/Include/stdarg.h". (Unless I'm writing a UEFI_APPLICATION module with standard C functions from StdLib, I don't want to read anything related to va_*.) (4) Thus, I suggest that we additionally *replace* the following leading comment: // // Support for variable length argument lists using the ANSI standard. // // Since we are using the ANSI standard we used the standard naming and // did not follow the coding convention // with: // // Support for variable argument lists in freestanding edk2 modules. // // For modules that use the ISO C library interfaces for variable // argument lists, refer to "StdLib/Include/stdarg.h". // Just my two cents. Thanks Laszlo > > /** > Return the size of argument that has been aligned to sizeof (UINTN). > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-12 9:16 ` Laszlo Ersek @ 2017-12-12 10:24 ` Zenith432 2017-12-12 10:39 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Zenith432 @ 2017-12-12 10:24 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, michael.d.kinney, liming.gao Below is an amended patch. I changed the wording in the notes a little to exclude a non-variadic function using VA_START, and to remind that VA_COPY should also be paired with VA_END. Also, if GCC gets its VA_COPY repaired, documentation may change to allow to using VA_COPY inside non-EFIAPI as well. --- This is to resolve bug 457. https://bugzilla.tianocore.org/show_bug.cgi?id=457 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> --- MdePkg/Include/Base.h | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index 02140a5a..4fd5161f 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -552,21 +552,24 @@ struct _LIST_ENTRY { #define BASE_8EB 0x8000000000000000ULL // -// Support for variable length argument lists using the ANSI standard. +// Support for variable argument lists in freestanding edk2 modules. // -// Since we are using the ANSI standard we used the standard naming and -// did not follow the coding convention +// For modules that use the ISO C library interfaces for variable +// argument lists, refer to "StdLib/Include/stdarg.h". // // VA_LIST - typedef for argument list. // VA_START (VA_LIST Marker, argument before the ...) - Init Marker for use. // VA_END (VA_LIST Marker) - Clear Marker -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an argument from -// the ... list. You must know the size and pass it in this macro. +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an argument from +// the ... list. You must know the type and pass it in this macro. Type +// must be compatible with the type of the actual next argument (as promoted +// according to the default argument promotions.) // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy of Start. // -// example: +// Example: // // UINTN +// EFIAPI // ExampleVarArg ( // IN UINTN NumberOfArgs, // ... @@ -582,15 +585,21 @@ struct _LIST_ENTRY { // VA_START (Marker, NumberOfArgs); // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { // // -// // The ... list is a series of UINTN values, so average them up. +// // The ... list is a series of UINTN values, so sum them up. // // // Result += VA_ARG (Marker, UINTN); // } // // VA_END (Marker); -// return Result +// return Result; // } // +// Notes: +// - Functions that call VA_START() / VA_END() must have a variable +// argument list and must be declared EFIAPI. +// - Functions that call VA_COPY() / VA_END() must be declared EFIAPI. +// - Functions that only use VA_LIST and VA_ARG() need not be EFIAPI. +// /** Return the size of argument that has been aligned to sizeof (UINTN). -- 2.14.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-12 10:24 ` Zenith432 @ 2017-12-12 10:39 ` Laszlo Ersek 2017-12-12 15:01 ` Gao, Liming 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2017-12-12 10:39 UTC (permalink / raw) To: Zenith432, edk2-devel; +Cc: michael.d.kinney, liming.gao On 12/12/17 11:24, Zenith432 wrote: > Below is an amended patch. > > I changed the wording in the notes a little to exclude a non-variadic > function using VA_START, and to remind that VA_COPY should also be > paired with VA_END. > > Also, if GCC gets its VA_COPY repaired, documentation may change to > allow to using VA_COPY > inside non-EFIAPI as well. > --- > > This is to resolve bug 457. > https://bugzilla.tianocore.org/show_bug.cgi?id=457 > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > --- > MdePkg/Include/Base.h | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index 02140a5a..4fd5161f 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -552,21 +552,24 @@ struct _LIST_ENTRY { > #define BASE_8EB 0x8000000000000000ULL > > // > -// Support for variable length argument lists using the ANSI standard. > +// Support for variable argument lists in freestanding edk2 modules. > // > -// Since we are using the ANSI standard we used the standard naming and > -// did not follow the coding convention > +// For modules that use the ISO C library interfaces for variable > +// argument lists, refer to "StdLib/Include/stdarg.h". > // > // VA_LIST - typedef for argument list. > // VA_START (VA_LIST Marker, argument before the ...) - Init Marker > for use. > // VA_END (VA_LIST Marker) - Clear Marker > -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an > argument from > -// the ... list. You must know the size and pass it in this macro. > +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an > argument from > +// the ... list. You must know the type and pass it in this macro. > Type > +// must be compatible with the type of the actual next argument (as > promoted > +// according to the default argument promotions.) > // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy > of Start. > // > -// example: > +// Example: > // > // UINTN > +// EFIAPI > // ExampleVarArg ( > // IN UINTN NumberOfArgs, > // ... > @@ -582,15 +585,21 @@ struct _LIST_ENTRY { > // VA_START (Marker, NumberOfArgs); > // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > // // > -// // The ... list is a series of UINTN values, so average them up. > +// // The ... list is a series of UINTN values, so sum them up. > // // > // Result += VA_ARG (Marker, UINTN); > // } > // > // VA_END (Marker); > -// return Result > +// return Result; > // } > // > +// Notes: > +// - Functions that call VA_START() / VA_END() must have a variable > +// argument list and must be declared EFIAPI. > +// - Functions that call VA_COPY() / VA_END() must be declared EFIAPI. > +// - Functions that only use VA_LIST and VA_ARG() need not be EFIAPI. > +// > > /** > Return the size of argument that has been aligned to sizeof (UINTN). Reviewed-by: Laszlo Ersek <lersek@redhat.com> (Liming, I can help with applying this patch, if you want that.) Thanks! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-12 10:39 ` Laszlo Ersek @ 2017-12-12 15:01 ` Gao, Liming 2017-12-12 17:42 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Gao, Liming @ 2017-12-12 15:01 UTC (permalink / raw) To: Laszlo Ersek, Zenith432, edk2-devel@lists.01.org; +Cc: Kinney, Michael D Laszlo: Thanks! Reviewed-by: Liming Gao <liming.gao@intel.com> > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Tuesday, December 12, 2017 6:39 PM > To: Zenith432 <zenith432@users.sourceforge.net>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h > > On 12/12/17 11:24, Zenith432 wrote: > > Below is an amended patch. > > > > I changed the wording in the notes a little to exclude a non-variadic > > function using VA_START, and to remind that VA_COPY should also be > > paired with VA_END. > > > > Also, if GCC gets its VA_COPY repaired, documentation may change to > > allow to using VA_COPY > > inside non-EFIAPI as well. > > --- > > > > This is to resolve bug 457. > > https://bugzilla.tianocore.org/show_bug.cgi?id=457 > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Zenith432 <zenith432 at users.sourceforge.net> > > --- > > MdePkg/Include/Base.h | 25 +++++++++++++++++-------- > > 1 file changed, 17 insertions(+), 8 deletions(-) > > > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > > index 02140a5a..4fd5161f 100644 > > --- a/MdePkg/Include/Base.h > > +++ b/MdePkg/Include/Base.h > > @@ -552,21 +552,24 @@ struct _LIST_ENTRY { > > #define BASE_8EB 0x8000000000000000ULL > > > > // > > -// Support for variable length argument lists using the ANSI standard. > > +// Support for variable argument lists in freestanding edk2 modules. > > // > > -// Since we are using the ANSI standard we used the standard naming and > > -// did not follow the coding convention > > +// For modules that use the ISO C library interfaces for variable > > +// argument lists, refer to "StdLib/Include/stdarg.h". > > // > > // VA_LIST - typedef for argument list. > > // VA_START (VA_LIST Marker, argument before the ...) - Init Marker > > for use. > > // VA_END (VA_LIST Marker) - Clear Marker > > -// VA_ARG (VA_LIST Marker, var arg size) - Use Marker to get an > > argument from > > -// the ... list. You must know the size and pass it in this macro. > > +// VA_ARG (VA_LIST Marker, var arg type) - Use Marker to get an > > argument from > > +// the ... list. You must know the type and pass it in this macro. > > Type > > +// must be compatible with the type of the actual next argument (as > > promoted > > +// according to the default argument promotions.) > > // VA_COPY (VA_LIST Dest, VA_LIST Start) - Initialize Dest as a copy > > of Start. > > // > > -// example: > > +// Example: > > // > > // UINTN > > +// EFIAPI > > // ExampleVarArg ( > > // IN UINTN NumberOfArgs, > > // ... > > @@ -582,15 +585,21 @@ struct _LIST_ENTRY { > > // VA_START (Marker, NumberOfArgs); > > // for (Index = 0, Result = 0; Index < NumberOfArgs; Index++) { > > // // > > -// // The ... list is a series of UINTN values, so average them up. > > +// // The ... list is a series of UINTN values, so sum them up. > > // // > > // Result += VA_ARG (Marker, UINTN); > > // } > > // > > // VA_END (Marker); > > -// return Result > > +// return Result; > > // } > > // > > +// Notes: > > +// - Functions that call VA_START() / VA_END() must have a variable > > +// argument list and must be declared EFIAPI. > > +// - Functions that call VA_COPY() / VA_END() must be declared EFIAPI. > > +// - Functions that only use VA_LIST and VA_ARG() need not be EFIAPI. > > +// > > > > /** > > Return the size of argument that has been aligned to sizeof (UINTN). > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > (Liming, I can help with applying this patch, if you want that.) > > Thanks! > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h 2017-12-12 15:01 ` Gao, Liming @ 2017-12-12 17:42 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2017-12-12 17:42 UTC (permalink / raw) To: Gao, Liming, Zenith432; +Cc: edk2-devel@lists.01.org, Kinney, Michael D On 12/12/17 16:01, Gao, Liming wrote: > Laszlo: > Thanks! > > Reviewed-by: Liming Gao <liming.gao@intel.com> Thanks! Pushed it as e3e40c83fd2c. Zenith432, thanks for the contribution -- for your next patch; please send the updated versions as separate top-level postings, such as [PATCH v2], [PATCH v3] etc. It seems that your original posting in this thread was sent just fine with git-send-email, but the updated variants were just pasted into emails using Thunderbird. So, in order to apply the final version, I had to reconstruct the commit manually from the email. That's a bit error prone (and slow). "git format-patch" takes an option of the form "-v2", "-v3" etc; it will fill in the subject prefix accordingly. If you want to add extra comments on the patch just about the v1->v2, v2->v3 differences, there are two possibilities: - Simply add those remarks to the commit message. (Some edk2 developers do this all the time, and it is fine IMO, although not the universally recommended practice.) - Use "git notes edit" to edit notes on commits -- they don't become part of the commit message, and don't change the SHA1 commit hashes. Then, use "git format-patch -v2 --notes" to format the updated version, for mailing out. This will include the notes in a separate email "section" such that humans will read it, but "git-am" / "git-push" will not carry it over. You can find more tips if you are interested, in: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers Thanks! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-12-12 17:37 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1667068483.2112668.1512898346914.ref@mail.yahoo.com>
2017-12-10 9:32 ` [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h Zenith432
2017-12-10 13:53 ` Gao, Liming
2017-12-10 15:43 ` Zenith432
2017-12-11 14:35 ` Gao, Liming
2017-12-11 14:57 ` Zenith432
2017-12-12 1:32 ` Gao, Liming
2017-12-12 8:36 ` Laszlo Ersek
2017-12-12 9:16 ` Laszlo Ersek
2017-12-12 10:24 ` Zenith432
2017-12-12 10:39 ` Laszlo Ersek
2017-12-12 15:01 ` Gao, Liming
2017-12-12 17:42 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox