[clang] [Modules] [HeaderSearch] Don't reenter headers if it is pragma once (PR #76119)

[Chuanqi Xu via cfe-commits]

https://github.com/ChuanqiXu9 updated https://github.com/llvm/llvm-project/pull/76119

>From 65fdddbd501b36f7dcef2db3fd25a7a4a1f80a0b Mon Sep 17 00:00:00 2001
From: Chuanqi Xu <yedeng.yd at linux.alibaba.com>
Date: Thu, 21 Dec 2023 11:24:14 +0800
Subject: [PATCH] [Modules] [HeaderSearch] Don't reenter headers if it is
 pragma once of #import

Close https://github.com/llvm/llvm-project/issues/73023

The direct issue of https://github.com/llvm/llvm-project/issues/73023 is
that we entered a header which is marked as pragma once since the
compiler think it is OK if there is controlling macro.

It doesn't make sense. I feel like it should be sufficient to skip it
after we see the '#pragma once'.

>From the context, it looks like the workaround is primarily for
ObjectiveC. So we might need reviewers from OC.
---
 clang/lib/Lex/HeaderSearch.cpp | 78 +++++++++++++++++-----------------
 clang/test/Modules/pr73023.cpp | 19 +++++++++
 2 files changed, 58 insertions(+), 39 deletions(-)
 create mode 100644 clang/test/Modules/pr73023.cpp

diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0f1090187734ff2..dfa974e9a67ede3 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1408,57 +1408,57 @@ bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor &PP,
   // Get information about this file.
   HeaderFileInfo &FileInfo = getFileInfo(File);
 
-  // FIXME: this is a workaround for the lack of proper modules-aware support
-  // for #import / #pragma once
-  auto TryEnterImported = [&]() -> bool {
-    if (!ModulesEnabled)
-      return false;
-    // Ensure FileInfo bits are up to date.
-    ModMap.resolveHeaderDirectives(File);
-    // Modules with builtins are special; multiple modules use builtins as
-    // modular headers, example:
-    //
-    //    module stddef { header "stddef.h" export * }
-    //
-    // After module map parsing, this expands to:
-    //
-    //    module stddef {
-    //      header "/path_to_builtin_dirs/stddef.h"
-    //      textual "stddef.h"
-    //    }
-    //
-    // It's common that libc++ and system modules will both define such
-    // submodules. Make sure cached results for a builtin header won't
-    // prevent other builtin modules from potentially entering the builtin
-    // header. Note that builtins are header guarded and the decision to
-    // actually enter them is postponed to the controlling macros logic below.
-    bool TryEnterHdr = false;
-    if (FileInfo.isCompilingModuleHeader && FileInfo.isModuleHeader)
-      TryEnterHdr = ModMap.isBuiltinHeader(File);
-
-    // Textual headers can be #imported from different modules. Since ObjC
-    // headers find in the wild might rely only on #import and do not contain
-    // controlling macros, be conservative and only try to enter textual headers
-    // if such macro is present.
-    if (!FileInfo.isModuleHeader &&
-        FileInfo.getControllingMacro(ExternalLookup))
-      TryEnterHdr = true;
-    return TryEnterHdr;
-  };
-
   // If this is a #import directive, check that we have not already imported
   // this header.
   if (isImport) {
     // If this has already been imported, don't import it again.
     FileInfo.isImport = true;
 
+    // FIXME: this is a workaround for the lack of proper modules-aware support
+    // for #import / #pragma once
+    auto TryEnterImported = [&]() -> bool {
+      if (!ModulesEnabled)
+        return false;
+      // Ensure FileInfo bits are up to date.
+      ModMap.resolveHeaderDirectives(File);
+      // Modules with builtins are special; multiple modules use builtins as
+      // modular headers, example:
+      //
+      //    module stddef { header "stddef.h" export * }
+      //
+      // After module map parsing, this expands to:
+      //
+      //    module stddef {
+      //      header "/path_to_builtin_dirs/stddef.h"
+      //      textual "stddef.h"
+      //    }
+      //
+      // It's common that libc++ and system modules will both define such
+      // submodules. Make sure cached results for a builtin header won't
+      // prevent other builtin modules from potentially entering the builtin
+      // header. Note that builtins are header guarded and the decision to
+      // actually enter them is postponed to the controlling macros logic below.
+      bool TryEnterHdr = false;
+      if (FileInfo.isCompilingModuleHeader && FileInfo.isModuleHeader)
+        TryEnterHdr = ModMap.isBuiltinHeader(File);
+
+      // Textual headers can be #imported from different modules. Since ObjC
+      // headers find in the wild might rely only on #import and do not contain
+      // controlling macros, be conservative and only try to enter textual
+      // headers if such macro is present.
+      if (!FileInfo.isModuleHeader &&
+          FileInfo.getControllingMacro(ExternalLookup))
+        TryEnterHdr = true;
+      return TryEnterHdr;
+    };
+
     // Has this already been #import'ed or #include'd?
     if (PP.alreadyIncluded(File) && !TryEnterImported())
       return false;
   } else {
     // Otherwise, if this is a #include of a file that was previously #import'd
     // or if this is the second #include of a #pragma once file, ignore it.
-    if ((FileInfo.isPragmaOnce || FileInfo.isImport) && !TryEnterImported())
+    if (FileInfo.isPragmaOnce || FileInfo.isImport)
       return false;
   }
 
diff --git a/clang/test/Modules/pr73023.cpp b/clang/test/Modules/pr73023.cpp
new file mode 100644
index 000000000000000..19d7df6ce8a4ce9
--- /dev/null
+++ b/clang/test/Modules/pr73023.cpp
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fsyntax-only -verify
+
+//--- i.h
+#ifndef I_H
+#pragma once
+struct S{};
+#endif
+
+//--- test.cpp
+// expected-no-diagnostics
+#include "i.h"
+
+int foo() {
+    return sizeof(S);
+}