[clangd] #undef macros inside preamble patch

[kadircet]

Summary

That way we can stop generating false macro redefinition diagnostics.

Depends on D142890

[sammccall]

I can't understand from the description, code, or testcases what problem this is fixing. Can you clarify, ideally by improving the testcases?

It seems to introduce a false negative in the case that the preamble *does* contain a definition of an already-defined macro, which probably needs to be called out.

[kadircet]

In D143093#4099623, @sammccall wrote:

I can't understand from the description, code, or testcases what problem this is fixing. Can you clarify, ideally by improving the testcases?

Yeah should've elaborated on this one. As explained offline the issue is macro token contents are not preserved in the PCH and are re-lexed when needed (for diagnosing macro-redefined errors in our case). Since PCH contents are stale, when we retrieve token contents based on ranges stored in the PCH using the new file contents, we get a false-positive.
This patch tries to avoid such false positive diagnostics by emitting an #undef statement before any macro definition we're going to emit.

---

It seems to introduce a false negative in the case that the preamble *does* contain a definition of an already-defined macro, which probably needs to be called out.

I am failing to follow this one. I can think of two different cases:

#define FOO 1
#define FOO 2
;

changes to

#define BAR
#define FOO 1
#define FOO 2
;

we won't have diagnostics here, not because of the #undef logic, but because we're not moving the diagnostic ranges around.

Other case is we've

#define FOO 1
;
#define FOO 2

and it gets changed to

#define BAR
#define FOO 1
;
#define FOO 2

now the issue is we're emitting re-definition diag to a location inside the preamble patch, but we don't translate the location back to main-file. again it has nothing to do with the #undef logic proposed here (and fixed by D143095).

[kadircet]

clang-tools-extra/clangd/Preamble.cpp

734

i know we've discussed emitting all the #undef directives at the top of the patch together rather than right before #define directives. but i am failing to remember any benefits and looping twice here doesn't feel great. so leaving it as-is for now, LMK if it feels wrong.

[kadircet]

• Update tests

[kadircet]

• Rebase

[sammccall]

clang-tools-extra/clangd/Preamble.cpp

216

initialize to pp_unknown

clang-tools-extra/clangd/unittests/PreambleTests.cpp

701–702

as elsewhere, I think this writing this as a single raw string without reusing BaselinePreamble is easier to follow

707–712

named ranges

[kadircet]

• Use raw strings
• Rebase

clang-tools-extra/clangd/Preamble.cpp

 
 // Represents directives other than includes, where basic textual information is
 // enough.
 struct TextualPPDirective {
   unsigned DirectiveLine;
   // Full text that's representing the directive, including the `#`.
   std::string Text;
   unsigned Offset;
+  tok::PPKeywordKind Directive = tok::PPKeywordKind::pp_not_keyword;
+  // Name of the macro being defined in the case of a #define directive.
+  std::string MacroName;
 
   bool operator==(const TextualPPDirective &RHS) const {
     return std::tie(DirectiveLine, Offset, Text) ==
            std::tie(RHS.DirectiveLine, RHS.Offset, RHS.Text);
   }
 };
 
 // Formats a PP directive consisting of Prefix (e.g. "#define ") and Body ("X
struct DirectiveCollector : public PPCallbacks {
   }
 
   void MacroDefined(const Token &MacroNameTok,
                     const MacroDirective *MD) override {
     if (!InMainFile)
       return;
     TextualDirectives.emplace_back();
     TextualPPDirective &TD = TextualDirectives.back();
+    TD.Directive = tok::pp_define;
+    TD.MacroName = MacroNameTok.getIdentifierInfo()->getName().str();
 
     const auto *MI = MD->getMacroInfo();
     TD.Text =
         spellDirective("#define ",
                        CharSourceRange::getTokenRange(
                            MI->getDefinitionLoc(), MI->getDefinitionEndLoc()),
                        LangOpts, SM, TD.DirectiveLine, TD.Offset);
   }
buildPreamble(PathRef FileName, CompilerInvocation CI,
   if (Stats != nullptr) {
     Stats->TotalBuildTime = PreambleTimer.getTime();
     Stats->FileSystemTime = TimedFS->getTime();
     Stats->SerializedSize = BuiltPreamble ? BuiltPreamble->getSize() : 0;
   }
 
   if (BuiltPreamble) {
     log("Built preamble of size {0} for file {1} version {2} in {3} seconds",
-         BuiltPreamble->getSize(), FileName, Inputs.Version,
-         PreambleTimer.getTime());
     std::vector<Diag> Diags = PreambleDiagnostics.take();
     auto Result = std::make_shared<PreambleData>(std::move(*BuiltPreamble));
     Result->Version = Inputs.Version;
     Result->CompileCommand = Inputs.CompileCommand;
     Result->Diags = std::move(Diags);
     Result->Includes = CapturedInfo.takeIncludes();
     Result->Pragmas = CapturedInfo.takePragmaIncludes();
     Result->Macros = CapturedInfo.takeMacros();
if (DirectivesChanged) {
     // If we've patched only the first directive, the macro definition would've
     // been wrong for the rest of the file, since patch is applied after the
     // baseline preamble.
     //
     // Note that we deliberately ignore conditional directives and undefs to
     // reduce complexity. The former might cause problems because scanning is
     // imprecise and might pick directives from disabled regions.
     for (const auto &TD : ModifiedScan->TextualDirectives) {
+      // Introduce an #undef directive before #defines to suppress any
+      // re-definition warnings.
+      if (TD.Directive == tok::pp_define)
+        Patch << "#undef " << TD.MacroName << '\n';
       Patch << "#line " << TD.DirectiveLine << '\n';
       Patch << TD.Text << '\n';
     }
   }
   dlog("Created preamble patch: {0}", Patch.str());
   Patch.flush();
   return PP;
 }

clang-tools-extra/clangd/unittests/PreambleTests.cpp

struct {
     const char *const Contents;
     const char *const ExpectedPatch;
   } Cases[] = {
       {
           R"cpp(
         #define BAR
         [[BAR]])cpp",
           R"cpp(#line 0 ".*main.cpp"
+#undef BAR
 #line 2
 #define         BAR
 )cpp",
       },
       // multiline macro
       {
           R"cpp(
         #define BAR \
 
         [[BAR]])cpp",
           R"cpp(#line 0 ".*main.cpp"
+#undef BAR
 #line 2
 #define         BAR
 )cpp",
       },
       // multiline macro
       {
           R"cpp(
         #define \
                 BAR
         [[BAR]])cpp",
           R"cpp(#line 0 ".*main.cpp"
+#undef BAR
 #line 3
 #define         BAR
 )cpp",
       },
   };
 
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Contents);
TEST(PreamblePatchTest, OrderingPreserved) {
   llvm::StringLiteral Baseline = "#define BAR(X) X";
   Annotations Modified(R"cpp(
     #define BAR(X, Y) X Y
     #define BAR(X) X
     [[BAR]](int y);
   )cpp");
 
   llvm::StringLiteral ExpectedPatch(R"cpp(#line 0 ".*main.cpp"
+#undef BAR
 #line 2
 #define     BAR\(X, Y\) X Y
+#undef BAR
 #line 3
 #define     BAR\(X\) X
 )cpp");
   EXPECT_THAT(getPreamblePatch(Baseline, Modified.code()),
               MatchesRegex(ExpectedPatch.str()));
 
   auto AST = createPatchedAST(Baseline, Modified.code());
   ASSERT_TRUE(AST);
$foo[[#include "foo.h"]])");
     auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
     EXPECT_THAT(
         *AST->getDiagnostics(),
         UnorderedElementsAre(Diag(NewCode.range("bar"), "unused-includes"),
                              Diag(NewCode.range("foo"), "unused-includes")));
   }
   {
     Annotations Code("#define [[FOO]] 1\n");
     // Check ranges for notes.
-    Annotations NewCode(R"(#define $barxyz[[BARXYZ]] 1
+    Annotations NewCode(R"(#define BARXYZ 1
 #define $foo1[[FOO]] 1
 void foo();
 #define $foo2[[FOO]] 2)");
     auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
     EXPECT_THAT(
         *AST->getDiagnostics(),
-        ElementsAre(
-            // FIXME: This diagnostics shouldn't exist. It's emitted from the
-            // preamble patch to the stale location inside preamble.
-            AllOf(Field(&Diag::Name, Eq("-Wmacro-redefined")),
-                  Field(&Diag::File, HasSubstr("_preamble_patch_")),
-                  withNote(Diag(NewCode.range("barxyz")))),
-            AllOf(
-                Diag(NewCode.range("foo2"), "-Wmacro-redefined"),
-                // FIXME: This should be translated into main file.
-                withNote(Field(&Note::File, HasSubstr("_preamble_patch_"))))));
+        ElementsAre(AllOf(
   }
 }
 
 TEST(PreamblePatch, TranslatesDiagnosticsInPreamble) {
   Config Cfg;
   Cfg.Diagnostics.AllowStalePreamble = true;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));