[C2y] Add documentation to conform to WG14 N3262; NFC

[AaronBallman]

This paper is about whether a copy of a va_list object which was not produced by calling va_copy is valid to use or not. While this may work on some targets, we explicitly document it as undefined behavior for all targets so there's not confusion as to when it's valid or not. It's not a burden for a user to use va_copy explicitly.

[AaronBallman]

The paper in question can be found here: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3262.pdf

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

This paper is about whether a copy of a va_list object which was not produced by calling va_copy is valid to use or not. While this may work on some targets, we explicitly document it as undefined behavior for all targets so there's not confusion as to when it's valid or not. It's not a burden for a user to use va_copy explicitly.

---

Full diff: https://github.com/llvm/llvm-project/pull/98146.diff

2 Files Affected:

• (modified) clang/docs/LanguageExtensions.rst (+3-1)
• (added) clang/test/C/C2y/n3262.c (+20)

diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
index d9439d49a2e29..90ba6d8e29987 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -3977,7 +3977,9 @@ standard library ``<stdarg.h>`` header:
 
 * ``__builtin_va_list``
 
-A predefined typedef for the target-specific ``va_list`` type.
+A predefined typedef for the target-specific ``va_list`` type. It is undefined
+behavior to use a copy of a value of this type unless the copy was produced by
+calling ``__builtin_va_copy``.
 
 * ``void __builtin_va_start(__builtin_va_list list, <parameter-name>)``
 
diff --git a/clang/test/C/C2y/n3262.c b/clang/test/C/C2y/n3262.c
new file mode 100644
index 0000000000000..3ff2062d88dde
--- /dev/null
+++ b/clang/test/C/C2y/n3262.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -verify -std=c2y -Wall -pedantic %s
+// expected-no-diagnostics
+
+/* WG14 N3262: Yes
+ * Usability of a byte-wise copy of va_list
+ *
+ * NB: Clang explicitly documents this as being undefined behavior. A
+ * diagnostic is produced for some targets but not for others for assignment or
+ * initialization, but no diagnostic is possible to produce for use with memcpy
+ * in the general case, nor with a manual bytewise copy via a for loop.
+ *
+ * Therefore, nothing is tested in this file; it serves as a reminder that we
+ * validated our documentation against the paper. See
+ * clang/docs/LanguageExtensions.rst for more details.
+ *
+ * FIXME: it would be nice to add ubsan support for recognizing when an invalid
+ * copy is made and diagnosing on copy (or on use of the copied va_list).
+ */
+
+int main() {}

[efriedma-quic]

As far as I know, clang doesn't currently support any targets where va_copy is not just a memcpy. The most common implementation is just a void pointer; the more complicated implementations involve multiple memory buffers, but those buffers are not actually part of the va_list, so va_copy is still just a trivial copy.

But if we don't want worry about it, I guess this is fine.

[AaronBallman]

As far as I know, clang doesn't currently support any targets where va_copy is not just a memcpy. The most common implementation is just a void pointer; the more complicated implementations involve multiple memory buffers, but those buffers are not actually part of the va_list, so va_copy is still just a trivial copy.

But if we don't want worry about it, I guess this is fine.

I figured we want the behavior to be well-defined or undefined consistently rather than making users think about it on a per-target basis, and it wasn't clear to me that every target (including future targets we don't yet support) would be able to cope with a byte-wise copy, and since va_copy is trivial to use, so it seemed like a "we don't want to worry about it" kind of situation. That said, I don't have strong opinions; I can certainly document the other way if we want to go that route.

[rjmccall]

I also have trouble imagining why a target would ever want to make va_copy a non-trivial operation, and I suspect that in practice programmers do not reliably call va_end to clean up their iterations. In general, I would say that platforms should be moving towards making varargs simpler by just treating variadic arguments differently from required arguments in the ABI, the way AArch64 originally did and still does on Apple platforms.

I wouldn't want to lock us out of supporting a target that required a non-trivial va_copy for whatever awful reason, but we could be clear that it's only UB on such targets. I don't think we actually gain anything from making it UB; I can't imagine what optimizations that would enable.

[AaronBallman]

I can't imagine what optimizations that would enable.

It's more about correctness than optimization opportunities -- do we want to promise users that the behavior will always work in all circumstances on a bytewise copy? What about code like this:

va_list one, two;

va_start(one, something);
memcpy(&one, &two, sizeof(one));
va_end(one);

int i = va_arg(two, int);
va_end(two);

(The situation I'm specifically worried about is when the __builtin_va_list is storing pointers internally. e.g., https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/mpc5xxx/10564/1/Power-Arch-32-bit-ABI-supp-1.0-Embedded.pdf

The type va_list shall be defined as follows:
typedef struct __va_list_tag {
unsigned char gpr;
unsigned char fpr;
/* Two bytes padding. */
char *overflow_arg_area;
char *reg_save_area;
} va_list[1];

it's possible that this works fine, but it's also possible that such an ABI would have different pointer values for different va_list objects within the same function.)

I wouldn't want to lock us out of supporting a target that required a non-trivial va_copy for whatever awful reason, but we could be clear that it's only UB on such targets.

I'm not super happy about "this is well-defined if you happen to be on a target that makes it well-defined" because there's basically no reasonable way for a user to find that information out themselves. I think we need to provide explicit documentation for which targets are doing what if we're not using a blanket rule, but I don't feel comfortable making those assertions for the various ABIs on my own.

[rjmccall]

Every va_list stores pointers; otherwise, it wouldn't be able to support an arbitrary number of arguments. Copying just copies the pointers, and that's fine because the memory they point to is immutable and always outlives the va_list. You can imagine a va_list implementation that doesn't have these properties, but it's hard.

I'm not super happy about "this is well-defined if you happen to be on a target that makes it well-defined" because there's basically no reasonable way for a user to find that information out themselves.

Well, if we're going to document that this is allowed, we should document what targets it's allowed on. I'm not arguing that we shouldn't document it.

[AaronBallman]

Every va_list stores pointers; otherwise, it wouldn't be able to support an arbitrary number of arguments. Copying just copies the pointers, and that's fine because the memory they point to is immutable and always outlives the va_list. You can imagine a va_list implementation that doesn't have these properties, but it's hard.

But do separate va_list objects within the same function always have the same pointer values? I was thinking the "saved state" pointers might actually point to different memory regions to support different restoration points, but I could be way off base there.

I'm not super happy about "this is well-defined if you happen to be on a target that makes it well-defined" because there's basically no reasonable way for a user to find that information out themselves.

Well, if we're going to document that this is allowed, we should document what targets it's allowed on. I'm not arguing that we shouldn't document it.

I had the unpleasant experience of trying to figure out what targets it's allowed on. We have five links to ABI resources regarding va_list in

llvm-project/clang/include/clang/Basic/TargetInfo.h

Line 319 in 746f572

enum BuiltinVaListKind {

; four of the links are dead and one of the targets is unsupported as of 2022 (thus has no documentation whatsoever) but its va_list type continues to be used outside of that target (

llvm-project/clang/lib/Basic/Targets/Le64.h

Line 41 in a1d73ac

return TargetInfo::PNaClABIBuiltinVaList;

).

Given the ease of calling va_copy, the fact that C had implicit UB here for ~40 years, and it's going to be a slog to try to nail down every single target's behavior, I still think documenting it as explicit UB is reasonable. If users or target owners want a different behavior, then they could give a use case for why and we could re-assess at that point. All we're effectively saying with that documentation is "if you do it, we don't promise it will work" and we already effectively say that with our existing documentation when we say It is undefined behavior to call this function with a list that has not been initialized by either __builtin_va_start or __builtin_va_copy.

Alternatively, we could document that it's supported on all targets, test whatever codegen Clang emits, assume all the targets support this, and then document any unsupported targets discovered in the wild as explicitly unsupported. I'm a bit less comfortable with that, but when both the codegen code owners tell me they it's very unlikely this won't work on any targets we care about, I can certainly trust that expertise!

[rjmccall]

Every va_list stores pointers; otherwise, it wouldn't be able to support an arbitrary number of arguments. Copying just copies the pointers, and that's fine because the memory they point to is immutable and always outlives the va_list. You can imagine a va_list implementation that doesn't have these properties, but it's hard.

But do separate va_list objects within the same function always have the same pointer values? I was thinking the "saved state" pointers might actually point to different memory regions to support different restoration points, but I could be way off base there.

The pointer values in a specific va_list can and often do change when you call va_arg, but I am not aware of any targets where they do not always point to the same regions. In every ABI I've ever seen, va_list contains a pointer to the stack argument area, and those pointers must point there because the stack argument area cannot be either copied or relocated. va_list may also contain one or more pointers to separate "spill areas" where the callee is required to spill certain argument registers, which it generally does in its prologue. In theory, a compiler could produce multiple copies of that spill area, or an ABI could require it to be copied by va_copy or even allocated on the heap. However, there's no good reason to do either of those things: the va_list can never validly escape the variadic function because of the limitation on the stack argument area, and so the spill area might as well also go in a unique location on the stack.

I'm not super happy about "this is well-defined if you happen to be on a target that makes it well-defined" because there's basically no reasonable way for a user to find that information out themselves.

Well, if we're going to document that this is allowed, we should document what targets it's allowed on. I'm not arguing that we shouldn't document it.

I had the unpleasant experience of trying to figure out what targets it's allowed on. We have five links to ABI resources regarding va_list in

llvm-project/clang/include/clang/Basic/TargetInfo.h

Line 319 in 746f572

enum BuiltinVaListKind {

; four of the links are dead and one of the targets is unsupported as of 2022 (thus has no documentation whatsoever) but its va_list type continues to be used outside of that target (

llvm-project/clang/lib/Basic/Targets/Le64.h

Line 41 in a1d73ac

return TargetInfo::PNaClABIBuiltinVaList;

).
Given the ease of calling va_copy, the fact that C had implicit UB here for ~40 years, and it's going to be a slog to try to nail down every single target's behavior, I still think documenting it as explicit UB is reasonable. If users or target owners want a different behavior, then they could give a use case for why and we could re-assess at that point. All we're effectively saying with that documentation is "if you do it, we don't promise it will work" and we already effectively say that with our existing documentation when we say It is undefined behavior to call this function with a list that has not been initialized by either __builtin_va_start or __builtin_va_copy.

Alternatively, we could document that it's supported on all targets, test whatever codegen Clang emits, assume all the targets support this, and then document any unsupported targets discovered in the wild as explicitly unsupported. I'm a bit less comfortable with that, but when both the codegen code owners tell me they it's very unlikely this won't work on any targets we care about, I can certainly trust that expertise!

I have no objection to requiring va_copy and documenting primitive copies as undefined behavior. It is the easiest thing to do, and we can clearly revisit it later if we want.

[AaronBallman]

The pointer values in a specific va_list can and often do change when you call va_arg, but I am not aware of any targets where they do not always point to the same regions. In every ABI I've ever seen, va_list contains a pointer to the stack argument area, and those pointers must point there because the stack argument area cannot be either copied or relocated. va_list may also contain one or more pointers to separate "spill areas" where the callee is required to spill certain argument registers, which it generally does in its prologue. In theory, a compiler could produce multiple copies of that spill area, or an ABI could require it to be copied by va_copy or even allocated on the heap. However, there's no good reason to do either of those things: the va_list can never validly escape the variadic function because of the limitation on the stack argument area, and so the spill area might as well also go in a unique location on the stack.

Ah! Thank you for the explanation, that makes a lot of sense.

I have no objection to requiring va_copy and documenting primitive copies as undefined behavior. It is the easiest thing to do, and we can clearly revisit it later if we want.

Thank you for the clarification. So you're happy with the current state of the PR, or are there some changes you'd like to see made?

[rjmccall]

Oh, I completely spaced on this before, but of course there are constraints on va_list in the standard: va_lists are passed by value to functions like vprintf. That, of course, requires the value to be primitively copied. If you call vprintf(format, args), the standard says the value of args is indeterminate after the call. It also specifically says that the v*printf functions do not call va_end.

As a result, the standard is clearly requiring that va_list be correctly "borrowed" when you pass it as an argument. It doesn't directly say that any other forms of relocation are allowed, but I think it would be surprising if argument passing worked but builtin assignment / initialization didn't, and I'm willing to say that we would never implement such a thing in Clang. memcpy is a separate question: you can imagine an ABI that requires these primitive copies to have special behavior, e.g. by storing the pointers with address-sensitive __ptrauth to resist data corruption.

The ownership of the value must be understood in C terms, not in terms of a C++-like object model. A va_list value potentially has ownership of some resource that must be destroyed by va_end. You don't have to destroy the "original" va_list, but you do have to destroy some primitive copy of it. Whether changes to a va_list made by va_arg are observed in primitive copies is indeterminate. va_copy produces an independent va_list that does not observe subsequent changes to the original va_list value(s) and must be separately destroyed.

How precisely we want to document this is an interesting question, but I do think it has to be more subtle than "primitive copies are UB", because we clearly can't make vprintf invalid.

[AaronBallman]

Oh, I completely spaced on this before, but of course there are constraints on va_list in the standard: va_lists are passed by value to functions like vprintf. That, of course, requires the value to be primitively copied. If you call vprintf(format, args), the standard says the value of args is indeterminate after the call. It also specifically says that the v*printf functions do not call va_end.

As a result, the standard is clearly requiring that va_list be correctly "borrowed" when you pass it as an argument. It doesn't directly say that any other forms of relocation are allowed, but I think it would be surprising if argument passing worked but builtin assignment / initialization didn't, and I'm willing to say that we would never implement such a thing in Clang. memcpy is a separate question: you can imagine an ABI that requires these primitive copies to have special behavior, e.g. by storing the pointers with address-sensitive __ptrauth to resist data corruption.

Good catch!

That said, a lot of these definitions of va_list are not available for builtin assignment/initialization because they're an array (which decays to a pointer when passed as a parameter), so memcpy or similar is the only way to get a byte-wise copy. Sometimes. https://godbolt.org/z/Gd5cEYYvz as an example

The ownership of the value must be understood in C terms, not in terms of a C++-like object model. A va_list value potentially has ownership of some resource that must be destroyed by va_end. You don't have to destroy the "original" va_list, but you do have to destroy some primitive copy of it. Whether changes to a va_list made by va_arg are observed in primitive copies is indeterminate. va_copy produces an independent va_list that does not observe subsequent changes to the original va_list value(s) and must be separately destroyed.

How precisely we want to document this is an interesting question, but I do think it has to be more subtle than "primitive copies are UB", because we clearly can't make vprintf invalid.

Hmmm, yeah, we don't want to make vprintf and friends invalid or imply that they are. Perhaps what we really want to make UB are explicit copies via memcpy, memmove, for loop copying the bytes manually, etc? The compiler is free to violate this to its heart's content if it uses these tricks under the hood, but users are not. Maybe something along the lines of:

It is undefined behavior to use a byte-wise copy of a value of this type explicitly produced by calling memcpy, memmove, or similar. Copies produced by callingva_copy or __builtin_va_copy, or copies made implicitly by the compiler such as when passing a parameter or returning a value, do not result in undefined behavior when used.

?

[rjmccall]

Ah right, I'd forgotten that some ABIs use that array trick to get it to pass by reference, and you're right that that makes it ill-formed to simply assign around.

I like your idea of specifically making it UB to copy with memcpy etc and just advising that people use va_copy.

[h-vetinari]

Perhaps also update

llvm-project/clang/www/c_status.html

Lines 1283 to 1285 in 345861b

	<td>Usability of a byte-wise copy of va_list</td>
	<td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3262.pdf">N3262</a></td>
	<td class="unknown" align="center">Unknown</td>

[AaronBallman]

Perhaps also update

llvm-project/clang/www/c_status.html

Lines 1283 to 1285 in 345861b

	<td>Usability of a byte-wise copy of va_list</td>
	<td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3262.pdf">N3262</a></td>
	<td class="unknown" align="center">Unknown</td>

Oh what the heck, I thought I already did that! :-D But yes, I'll update that when landing, thank you!