[Preprocessor] Do not expand macros if the input is already preprocessed

[jmmartinez]

Preprocessing the preprocessor output again interacts poorly with some flag combinations when we perform a separate preprocessing stage. In our case, -no-integrated-cpp -dD triggered this issue; but I guess that other flags could also trigger problems (-save-temps instead of -no-integrated-cpp).

Full context (which is quite weird I'll admit):

• To cache OpenCL kernel compilation results, we use the -no-integrated-cpp for the driver to generate a separate preprocessing command (clang -E) before the rest of the compilation.
• Some OpenCL C language features are implemented as macro definitions (in opencl-c-base.h). The semantic analysis queries the preprocessor to check if these are defined or not, for example, when we checks if a builtin is available when using -fdeclare-opencl-builtins.
• To preserve these #define directives, on the preprocessor's output, we use -dD. However, other #define directives are also maintained besides OpenCL ones; which triggers the issue shown in this PR.

A better fix for our particular case could have been to move the language features implemented as macros into some sort of a flag to be used together with -fdeclare-opencl-builtins.
But I also thought that not preprocessing preprocessor outputs seemed like something desirable. I hope to work on this on a follow up.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Juan Manuel Martinez Caamaño (jmmartinez)

Changes

This is a draft while I'm trying to figure out what's left to do.

This has issues with the test Modules/initializers.cpp.
The function CompilerInstance::createModuleFromSource creates a FrontendInput that "is preprocessed" from the preprocessed source string that is passed as argument (the module contents). However, this does not seem preprocessed (at least for this test).

llvm-project/clang/lib/Frontend/CompilerInstance.cpp

Line 2243 in ca21508

InputKind::ModuleMap, /*Preprocessed*/true));

I'm not familiar with modules and I'm not sure if it's this patch, or the test that has issues.

Other failures are fixed with #137623

---

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

3 Files Affected:

• (modified) clang/include/clang/Lex/Preprocessor.h (+5)
• (modified) clang/lib/Frontend/InitPreprocessor.cpp (+7)
• (added) clang/test/Preprocessor/preprocess-cpp-output.c (+10)

diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index f2dfd3a349b8b..63774e48a468b 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1831,6 +1831,11 @@ class Preprocessor {
     MacroExpansionInDirectivesOverride = true;
   }
 
+  void SetDisableMacroExpansion() {
+    DisableMacroExpansion = true;
+    MacroExpansionInDirectivesOverride = false;
+  }
+
   /// Peeks ahead N tokens and returns that token without consuming any
   /// tokens.
   ///
diff --git a/clang/lib/Frontend/InitPreprocessor.cpp b/clang/lib/Frontend/InitPreprocessor.cpp
index 1f297f228fc1b..6693cfb469f82 100644
--- a/clang/lib/Frontend/InitPreprocessor.cpp
+++ b/clang/lib/Frontend/InitPreprocessor.cpp
@@ -1556,6 +1556,13 @@ void clang::InitializePreprocessor(Preprocessor &PP,
                                    const PCHContainerReader &PCHContainerRdr,
                                    const FrontendOptions &FEOpts,
                                    const CodeGenOptions &CodeGenOpts) {
+
+  if (all_of(FEOpts.Inputs,
+             [](const FrontendInputFile &FI) { return FI.isPreprocessed(); })) {
+    PP.SetDisableMacroExpansion();
+    return;
+  }
+
   const LangOptions &LangOpts = PP.getLangOpts();
   std::string PredefineBuffer;
   PredefineBuffer.reserve(4080);
diff --git a/clang/test/Preprocessor/preprocess-cpp-output.c b/clang/test/Preprocessor/preprocess-cpp-output.c
new file mode 100644
index 0000000000000..2c180601e30ac
--- /dev/null
+++ b/clang/test/Preprocessor/preprocess-cpp-output.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -E -x c %s | FileCheck %s --check-prefixes=EXPANDED
+// RUN: %clang_cc1 -E -x cpp-output %s | FileCheck %s --check-prefixes=NOT-EXPANDED
+
+// EXPANDED: void __attribute__((__attribute__((always_inline)))) foo()
+// NOT-EXPANDED: void __attribute__((always_inline)) foo()
+
+#define always_inline __attribute__((always_inline))
+void __attribute__((always_inline)) foo() {
+    return 4;
+}

[Bigcheese]

module maps don't get preprocessed, so I don't think it matters what that is set to. For normal module.modulemap files Preprocessed is false. I think it's fine to remove the true and see if anything breaks, the preprocessor just skips over that section anyway.

[lamb-j]

module maps don't get preprocessed, so I don't think it matters what that is set to. For normal module.modulemap files Preprocessed is false. I think it's fine to remove the true and see if anything breaks, the preprocessor just skips over that section anyway.

Thanks for the pointer. @Bigcheese who should we solicit an approving review from? I'm not familiar enough with the implications of the change to feel confident approving

[jmmartinez]

module maps don't get preprocessed, so I don't think it matters what that is set to. For normal module.modulemap files Preprocessed is false. I think it's fine to remove the true and see if anything breaks, the preprocessor just skips over that section anyway.

Currently a single test breaks: Modules/initializers.cpp which has preprocessor directives in the module source.

I guess then that this is an issue with the test. I'll try to propose a fix to the test then.

[jmmartinez]

I fixed the Modules/initializers.cpp test by moving the preprocessor directives outside of the module contents (the module contents should be already preprocessed and not contain preproc directives).

[cor3ntin]

This seems reasonable. Can you update the description to explain the motivation for the change, though?
Is it a bug you are trying to fix, or a specific scenario you think should be optimized? Thanks

[jmmartinez]

This seems reasonable. Can you update the description to explain the motivation for the change, though? Is it a bug you are trying to fix, or a specific scenario you think should be optimized? Thanks

I've updated the description with an explanation of the situation that triggered the issue for us. But in the wider scheme, I think that no preprocessing the preprocessor's output is the actual desired behavior.

Thanks!

[jmmartinez]

Ping !

[cor3ntin]

Did you consider doing that on a per-file basis in FrontendAction::BeginSourceFile?

[jmmartinez]

You're right, it makes more sense there. I've updated it to match.

[cor3ntin]

Ping !

Sorry, I missed that you replied last week

[cor3ntin]

We should (save and) restore the previous state in EndSourceFileAction,
Did you check if any subclass of FrontendAction would need to be modified?