[clang] [C2y] Add documentation to conform to WG14 N3262; NFC (PR #98146)

John McCall via cfe-commits cfe-commits at lists.llvm.org
Tue Jul 9 14:58:22 PDT 2024


rjmccall wrote:

> > _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.  Every `va_list` ABI I know has a pointer to the stack argument area, and those pointers *must* point there because the stack argument area cannot be either copied or relocated.  Some ABIs also require certain argument registers to be spilled to the stack in the prologue, and then a pointer to that spill area (or multiple areas, depending on target) is also written into the `va_list`.  In theory, a compiler could produce multiple copies of that spill area, or an ABI could require it to be allocate on the heap or copied by `va_copy`.  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
> 
> https://github.com/llvm/llvm-project/blob/746f5726158d31aeeb1fd12b226dc869834a7ad2/clang/include/clang/Basic/TargetInfo.h#L319
> 
> ; 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 (
> https://github.com/llvm/llvm-project/blob/a1d73ace13a20ed122a66d3d59f0cbae58d0f669/clang/lib/Basic/Targets/Le64.h#L41
> 
> ).
> 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.

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


More information about the cfe-commits mailing list