Skip to content

fix and test va_arg on f128 on x86 - #163037

Open
folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:x86-vaarg-f128
Open

folkertdev wants to merge 1 commit into
rust-lang:mainfrom
folkertdev:x86-vaarg-f128

Conversation

@folkertdev

Copy link
Copy Markdown
Contributor

tracking issue: #116909

Or well, maybe not fix but clarify. There are no actual behavioral changes, but I do think the new code is more correct.

The clang logic for va_arg is kind of (weirdly) complicated. EmitVAArg looks simple, but the complexity is hiding in the "messing with TypeInfo" bit:

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L1080-L1101

RValue X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                QualType Ty, AggValueSlot Slot) const {

  auto TypeInfo = getContext().getTypeInfoInChars(Ty);

  CCState State(*const_cast<CGFunctionInfo *>(CGF.CurFnInfo));
  ABIArgInfo AI = classifyArgumentType(Ty, State, /*ArgIndex*/ 0);
  // Empty records are ignored for parameter passing purposes.
  if (AI.isIgnore())
    return Slot.asRValue();

  // x86-32 changes the alignment of certain arguments on the stack.
  //
  // Just messing with TypeInfo like this works because we never pass
  // anything indirectly.
  TypeInfo.Align = CharUnits::fromQuantity(
                getTypeStackAlignInBytes(Ty, TypeInfo.Align.getQuantity()));

  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
                          CharUnits::fromQuantity(4),
                          /*AllowHigherAlign*/ true, Slot);
}

Relevant for us is that it makes an exception for f128, for which a higher align (16, instead of the slot size of 4) is used.

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L575-L583

unsigned X86_32ABIInfo::getTypeStackAlignInBytes(QualType Ty,
                                                 unsigned Align) const {
  // Otherwise, if the alignment is less than or equal to the minimum ABI
  // alignment, just use the default; the backend will handle this.
  if (Align <= MinABIStackAlignInBytes)
    return 0; // Use default alignment.

  if (Ty->isFloat128Type())
    return 16;

Our code does not have this TypeInfo idea, and it's not correct in general to just use AllowHigherAlign::Yes. But using it only for f128 works. We don't need to consider more complicated cases like structs containing f128 fields.

The code here used is_like_windows before. I'm not exactly sure why, the behavior is the same for all accepted types, except now f128. But windows does not have that type, so its (c-variadic) ABI is just an LLVM fabrication. The x86 code does not seem to make a distinction based on the ABI, so having the same behavior across targets seems best to me.

va_arg to fetch an i128 argument is currently broken in clang. i128 on the target is apparently a clang extension anyway, so kind of low priority. f128 I only fixed somewhat recently in llvm/llvm-project#218017.

r? beetrees or @tgross35

(this may cause a small conflict with #163021)

@folkertdev folkertdev added F-c_variadic `#![feature(c_variadic)]` F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` labels Sep 19, 2026
@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 19, 2026
@tgross35

Copy link
Copy Markdown
Member

Is there a way to poke this into being a problem in the runtime tests?

@folkertdev

Copy link
Copy Markdown
Contributor Author

Hmm, maybe with i686-pc-windows-gnu? I have no way to run that though. Also I think the c-variadic roundtrip.rs test would actually have caught it, it adds a dummy u32 argument to test the alignment handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-c_variadic `#![feature(c_variadic)]` F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants