Skip to content

Conversation

@dyung
Copy link
Collaborator

@dyung dyung commented Aug 2, 2024

Reverts #100837

The test Modules/builtin-vararg.c is failing on AArch64 build bots:

Revert to get the bots back to green.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Aug 2, 2024
@dyung dyung merged commit 48e624d into main Aug 2, 2024
@dyung dyung deleted the revert-100837-builtin-varargs-in-modules branch August 2, 2024 21:11
@llvmbot
Copy link
Member

llvmbot commented Aug 2, 2024

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: None (dyung)

Changes

Reverts llvm/llvm-project#100837

The test Modules/builtin-vararg.c is failing on AArch64 build bots:

Revert to get the bots back to green.


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

2 Files Affected:

  • (modified) clang/lib/Sema/Sema.cpp (-7)
  • (removed) clang/test/Modules/builtin-vararg.c (-83)
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 19d8692ee6484..2e989f0ba6fe4 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -310,13 +310,6 @@ void Sema::addImplicitTypedef(StringRef Name, QualType T) {
 }
 
 void Sema::Initialize() {
-  // Create BuiltinVaListDecl *before* ExternalSemaSource::InitializeSema(this)
-  // because during initialization ASTReader can emit globals that require
-  // name mangling. And the name mangling uses BuiltinVaListDecl.
-  if (Context.getTargetInfo().hasBuiltinMSVaList())
-    (void)Context.getBuiltinMSVaListDecl();
-  (void)Context.getBuiltinVaListDecl();
-
   if (SemaConsumer *SC = dyn_cast<SemaConsumer>(&Consumer))
     SC->InitializeSema(*this);
 
diff --git a/clang/test/Modules/builtin-vararg.c b/clang/test/Modules/builtin-vararg.c
deleted file mode 100644
index ed6e6aeb6e50f..0000000000000
--- a/clang/test/Modules/builtin-vararg.c
+++ /dev/null
@@ -1,83 +0,0 @@
-// Check how builtins using varargs behave with the modules.
-
-// RUN: rm -rf %t
-// RUN: split-file %s %t
-
-// RUN: %clang_cc1 -triple x86_64-apple-darwin \
-// RUN:   -fmodules -fno-implicit-modules -fbuiltin-headers-in-system-modules \
-// RUN:   -emit-module -fmodule-name=DeclareVarargs \
-// RUN:   -x c %t/include/module.modulemap -o %t/DeclareVarargs.pcm \
-// RUN:   -fmodule-map-file=%t/resource_dir/module.modulemap -isystem %t/resource_dir
-// RUN: %clang_cc1 -triple x86_64-apple-darwin \
-// RUN:   -fmodules -fno-implicit-modules -fbuiltin-headers-in-system-modules \
-// RUN:   -emit-pch -fmodule-name=Prefix \
-// RUN:   -x c-header %t/prefix.pch -o %t/prefix.pch.gch \
-// RUN:   -fmodule-map-file=%t/include/module.modulemap -fmodule-file=DeclareVarargs=%t/DeclareVarargs.pcm \
-// RUN:   -I %t/include
-// RUN: %clang_cc1 -triple x86_64-apple-darwin \
-// RUN:   -fmodules -fno-implicit-modules -fbuiltin-headers-in-system-modules \
-// RUN:   -emit-obj -fmodule-name=test \
-// RUN:   -x c %t/test.c -o %t/test.o \
-// RUN:   -Werror=incompatible-pointer-types \
-// RUN:   -fmodule-file=%t/DeclareVarargs.pcm -include-pch %t/prefix.pch.gch \
-// RUN:   -I %t/include
-
-//--- include/declare-varargs.h
-#ifndef DECLARE_VARARGS_H
-#define DECLARE_VARARGS_H
-
-#include <stdarg.h>
-
-int vprintf(const char *format, va_list args);
-
-// 1. initializeBuiltins 'acos' causes its deserialization and deserialization
-//    of 'implementation_of_builtin'. Because this is done before Sema initialization,
-//    'implementation_of_builtin' DeclID is added to PreloadedDeclIDs.
-#undef acos
-#define acos(__x) implementation_of_builtin(__x)
-
-// 2. Because of 'static' the function isn't added to EagerlyDeserializedDecls
-//    and not deserialized in `ASTReader::StartTranslationUnit` before `ASTReader::InitializeSema`.
-// 3. Because of '__overloadable__' attribute the function requires name mangling during deserialization.
-//    And the name mangling requires '__builtin_va_list' decl.
-//    Because the function is added to PreloadedDeclIDs, the deserialization happens in `ASTReader::InitializeSema`.
-static int __attribute__((__overloadable__)) implementation_of_builtin(int x) {
-  return x;
-}
-
-#endif // DECLARE_VARARGS_H
-
-//--- include/module.modulemap
-module DeclareVarargs {
-  header "declare-varargs.h"
-  export *
-}
-
-//--- resource_dir/stdarg.h
-#ifndef STDARG_H
-#define STDARG_H
-
-typedef __builtin_va_list va_list;
-#define va_start(ap, param) __builtin_va_start(ap, param)
-#define va_end(ap) __builtin_va_end(ap)
-
-#endif // STDARG_H
-
-//--- resource_dir/module.modulemap
-module _Builtin_stdarg {
-  header "stdarg.h"
-  export *
-}
-
-//--- prefix.pch
-#include <declare-varargs.h>
-
-//--- test.c
-#include <declare-varargs.h>
-
-void test(const char *format, ...) {
-  va_list argParams;
-  va_start(argParams, format);
-  vprintf(format, argParams);
-  va_end(argParams);
-}

vsapsai added a commit to vsapsai/llvm-project that referenced this pull request Aug 2, 2024
…header. (llvm#101752)"

Fix the false warning
> incompatible pointer types passing 'va_list' (aka '__builtin_va_list') to parameter of type 'struct __va_list_tag *' [-Wincompatible-pointer-types]

The warning is wrong because both in the function declaration and at the
call site we are using `va_list`.

When we call `ASTContext::getBuiltinVaListDecl` at a specific moment, we
end up re-entering this function which causes creating 2 instances of
`BuiltinVaListDecl` and 2 instances of `VaListTagDecl` but the stored
instances are unrelated to each other because of the call sequence like

    getBuiltinVaListDecl
      CreateX86_64ABIBuiltinVaListDecl
        VaListTagDecl = TagA
        indirectly call getBuiltinVaListDecl
          CreateX86_64ABIBuiltinVaListDecl
            VaListTagDecl = TagB
          BuiltinVaListDecl = ListB
      BuiltinVaListDecl = ListA

Now we have `BuiltinVaListDecl == ListA` and `VaListTagDecl == TagB`.

For x86_64 '__builtin_va_list' and 'struct __va_list_tag *' are
compatible because '__builtin_va_list' == '__va_list_tag[1]'. But
because we have unrelated decls for VaListDecl and VaListTagDecl the
types are considered incompatible as we are comparing type pointers.

Fix the error by creating `BuiltinVaListDecl` before
`ASTReader::InitializeSema`, so that during
`ASTContext::getBuiltinVaListDecl` ASTReader doesn't try to de-serialize
'__builtin_va_list' and to call `ASTContext::getBuiltinVaListDecl`
again.

Reland with the requirement to have x86 target to avoid errors like
> error: unable to create target: 'No available targets are compatible with triple "x86_64-apple-darwin"'

rdar://130947515
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:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants