Skip to content

Conversation

@jeremyd2019
Copy link
Contributor

This was in older patches for Cygwin support, but was somehow lost in the latest rounds.

This was in older patches for Cygwin support, but was somehow lost in
the latest rounds.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2025

@llvm/pr-subscribers-clang

Author: None (jeremyd2019)

Changes

This was in older patches for Cygwin support, but was somehow lost in the latest rounds.


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

1 Files Affected:

  • (modified) clang/lib/Basic/Targets/X86.h (+4)
diff --git a/clang/lib/Basic/Targets/X86.h b/clang/lib/Basic/Targets/X86.h
index 3d58be8f898c6..b3fe98c5c742f 100644
--- a/clang/lib/Basic/Targets/X86.h
+++ b/clang/lib/Basic/Targets/X86.h
@@ -984,6 +984,10 @@ class LLVM_LIBRARY_VISIBILITY CygwinX86_64TargetInfo : public X86_64TargetInfo {
     this->WCharType = TargetInfo::UnsignedShort;
   }
 
+  BuiltinVaListKind getBuiltinVaListKind() const override {
+    return TargetInfo::CharPtrBuiltinVaList;
+  }
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
     X86_64TargetInfo::getTargetDefines(Opts, Builder);

@jeremyd2019
Copy link
Contributor Author

@mstorsjo

@mstorsjo
Copy link
Member

mstorsjo commented Jun 6, 2025

The change looks good, but can we have a test for it in some form?

@jeremyd2019
Copy link
Contributor Author

I don't know - it's easy enough to do in vivo

#include <stdio.h>
#include <stdarg.h>

void test(const char * fmt, ...)
{
        va_list args;
        va_start(args, fmt);
        vprintf(fmt, args);
        va_end(args);
}

int main(void)
{
        test("asdf %d %s\n", 1, 1==1 ? "TEST" : "TESTS");
        return 0;
}

Before this change

asdf -13288 (null)

After

asdf 1 TEST

@jeremyd2019
Copy link
Contributor Author

I'm not seeing an obvious place for a target-specific va_list test, can you suggest where this would go?

@mstorsjo
Copy link
Member

mstorsjo commented Jun 6, 2025

I'm not seeing an obvious place for a target-specific va_list test, can you suggest where this would go?

No idea offhand; I tried breaking the existing getBuiltinVaListKind for WindowsX86_64TargetInfo and running check-clang, which caused failures in the following tests:

  Clang :: AST/ast-dump-template-json-win32-mangler-crash.cpp
  Clang :: CodeGen/ms_abi.c
  Clang :: CodeGenCXX/ext-int.cpp

So some of them may contain a good sample for tests that cover this bit.

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Jun 7, 2025

None of those really cry out to me as a place to extend with a cygwin test... but adding a new test in CodeGen that validates that the va_list works the same on windows and cygwin based on the existing tests wouldn't be too hard.

@kikairoya
Copy link
Contributor

I have a test already: #143115

@jeremyd2019 jeremyd2019 closed this Jun 7, 2025
@jeremyd2019 jeremyd2019 deleted the clang-cygwin-valist branch June 7, 2025 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants