Skip to content

[C++20][Modules] Allow using stdarg.h with header units #100739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 1, 2024

Conversation

dmpolukhin
Copy link
Contributor

Summary:
Macro like va_start/va_end marked as builtin functions that makes these identifiers special and it results in redefinition of the identifiers as builtins and it hides macro definitions during preloading C++ modules. In case of modules Clang ignores special identifiers but PP.getCurrentModule() was not set. This diff fixes IsModule detection logic for this particular case.

Test Plan: check-clang

Summary:
Macro like `va_start`/`va_end` marked as builtin functions that makes
these identifiers special and it results in false redifinition for the
identifiers and hides macro definitions.

Test Plan: check-clang
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Jul 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 26, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Dmitry Polukhin (dmpolukhin)

Changes

Summary:
Macro like va_start/va_end marked as builtin functions that makes these identifiers special and it results in redefinition of the identifiers as builtins and it hides macro definitions during preloading C++ modules. In case of modules Clang ignores special identifiers but PP.getCurrentModule() was not set. This diff fixes IsModule detection logic for this particular case.

Test Plan: check-clang


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

2 Files Affected:

  • (modified) clang/lib/Serialization/ASTReader.cpp (+4-5)
  • (added) clang/test/Headers/stdarg-cxx-modules.cpp (+27)
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 3cb96df12e4da..f3e8027526f6a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -1051,10 +1051,9 @@ IdentifierID ASTIdentifierLookupTrait::ReadIdentifierID(const unsigned char *d)
   return Reader.getGlobalIdentifierID(F, RawID >> 1);
 }
 
-static void markIdentifierFromAST(ASTReader &Reader, IdentifierInfo &II) {
+static void markIdentifierFromAST(ASTReader &Reader, IdentifierInfo &II, bool IsModule) {
   if (!II.isFromAST()) {
     II.setIsFromAST();
-    bool IsModule = Reader.getPreprocessor().getCurrentModule() != nullptr;
     if (isInterestingIdentifier(Reader, II, IsModule))
       II.setChangedSinceDeserialization();
   }
@@ -1080,7 +1079,7 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k,
     II = &Reader.getIdentifierTable().getOwn(k);
     KnownII = II;
   }
-  markIdentifierFromAST(Reader, *II);
+  markIdentifierFromAST(Reader, *II, Reader.getPreprocessor().getCurrentModule() != nullptr);
   Reader.markIdentifierUpToDate(II);
 
   IdentifierID ID = Reader.getGlobalIdentifierID(F, RawID);
@@ -4543,7 +4542,7 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName, ModuleKind Type,
 
       // Mark this identifier as being from an AST file so that we can track
       // whether we need to serialize it.
-      markIdentifierFromAST(*this, *II);
+      markIdentifierFromAST(*this, *II, true);
 
       // Associate the ID with the identifier so that the writer can reuse it.
       auto ID = Trait.ReadIdentifierID(Data + KeyDataLen.first);
@@ -8961,7 +8960,7 @@ IdentifierInfo *ASTReader::DecodeIdentifierInfo(IdentifierID ID) {
     auto Key = Trait.ReadKey(Data, KeyDataLen.first);
     auto &II = PP.getIdentifierTable().get(Key);
     IdentifiersLoaded[Index] = ⅈ
-    markIdentifierFromAST(*this,  II);
+    markIdentifierFromAST(*this,  II, getPreprocessor().getCurrentModule() != nullptr);
     if (DeserializationListener)
       DeserializationListener->IdentifierRead(ID, &II);
   }
diff --git a/clang/test/Headers/stdarg-cxx-modules.cpp b/clang/test/Headers/stdarg-cxx-modules.cpp
new file mode 100644
index 0000000000000..b7c4486c89374
--- /dev/null
+++ b/clang/test/Headers/stdarg-cxx-modules.cpp
@@ -0,0 +1,27 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header h1.h
+// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-user-header h2.h -fmodule-file=h1.pcm
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only main.cpp -fmodule-file=h1.pcm -fmodule-file=h2.pcm
+
+//--- h1.h
+#pragma once
+#include <stdarg.h>
+// expected-no-diagnostics
+
+//--- h2.h
+#pragma once
+import "h1.h";
+// expected-no-diagnostics
+
+//--- main.cpp
+import "h1.h";
+import "h2.h";
+
+void foo(int x, ...) {
+  va_list v;
+  va_start(v, x);
+  va_end(v);
+}
+// expected-no-diagnostics

Copy link

github-actions bot commented Jul 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dmpolukhin dmpolukhin merged commit f3761a4 into llvm:main Aug 1, 2024
7 checks passed
@dmpolukhin dmpolukhin deleted the stdarg-cxx-modules branch August 1, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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