From: Laszlo Ersek <lersek@redhat.com>
To: Zenith432 <zenith432@users.sourceforge.net>, edk2-devel@lists.01.org
Cc: michael.d.kinney@intel.com, liming.gao@intel.com
Subject: Re: [PATCH] MdePkg: correct and clarify documentation of VA_LIST in Base.h
Date: Tue, 12 Dec 2017 10:16:16 +0100 [thread overview]
Message-ID: <af97af46-0098-1b14-d004-5cddab7b189f@redhat.com> (raw)
In-Reply-To: <1667068483.2112668.1512898346914@mail.yahoo.com>
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).
>
next prev parent reply other threads:[~2017-12-12 9:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=af97af46-0098-1b14-d004-5cddab7b189f@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox