Allow /D flags absent during PCH creation under msvc-compat

[zahen]

Summary

Before this patch adding a new /D flag when compiling a source file that consumed a PCH with clang-cl would issue a diagnostic and then fail. With the patch, the diagnostic is still issued but the definition is accepted. This matches the msvc behavior. The fuzzy-pch-msvc.c is a clone of the existing fuzzy-pch.c tests with some msvc specific rework.

msvc diagnostic:

warning C4605: '/DBAR=int' specified on current command line, but was not specified when precompiled header was built

Output of the CHECK-BAR test prior to the code change:

<built-in>(1,9): warning: definition of macro 'BAR' does not match definition in precompiled header [-Wclang-cl-pch]
#define BAR int
        ^
D:\repos\llvm\llvm-project\clang\test\PCH\fuzzy-pch-msvc.c(12,1): error: unknown type name 'BAR'
BAR bar = 17;
^
D:\repos\llvm\llvm-project\clang\test\PCH\fuzzy-pch-msvc.c(23,4): error: BAR was not defined
#  error BAR was not defined
   ^
1 warning and 2 errors generated.

[thakis]

Why do you want this? Isn't it always easy to fix your build instead?

In general, stricter is better when we can get away with it, and since we've had this behavior for a while…

[zahen]

My change keeps the diagnostic so consumers can opt into the same enforcement that exists today. Furthermore, the existing fuzzy-pch.c tests show that new -D flags are allowed under a "clangier" PCH structure. None of the existing tests error on:

BAR bar = 17;

when -DBar=int is only part of the the test file's command line.

[rnk]

Honestly, MSVC's behavior makes more sense to me. Usually warnings tell the user they are doing something silly, and then let them do it anyway. Without this change, there is no way to add extra macros to some compilations, even if they won't affect the already-parsed PCH blob.

+@aganea, since I think he has used /Yc /Yu, and @mikerice, the last person to touch it.

BTW, this raises the issue of how PCH will work with dllexport, which typically works by defining some kind of "FOO_DLL" macro to indicate that some annotations should expand to __declspec(dllexport). I suppose this change won't solve that problem, but it's still interesting.

clang/lib/Lex/PPDirectives.cpp

2728

I think the case of passing -D to compilation but not PCH is likely to be the overwhelming majority of cases that users run into, and MSVC has a nice, specific diagnostic for that case (probably "!OtherMI"). We should do that too, but do not feel obligated to do that in this change.

2731

PCH is in the space of implementation-defined behaviors, not standards-mandated behaviors. I think MicrosoftExt (-fms-extensions) would be a better condition here. Besides, it's consistent with the use just above.

clang/test/PCH/fuzzy-pch-msvc.c

30–31 ↗

(On Diff #236867)

For this stuff, it would be nicer to use %clang_cc1 -verify instead of FileCheck. You can do things like:

// expected-warning@2 {{'FOO' macro redefined}}
// expected-note@1 {{previous definition is here}}
int main() {}

I just checked, and that works.

I see that the fuzzy-pch.c test that you based this on probably predates clang -cc1 -verify.

[zahen]

Incorporate review feedback.

I had no luck converting the CHECK-FOO & CHECK-NOFOO tests to use -verify because the errors were reported against "(frontend)"

error: 'error' diagnostics seen but not expected:
  (frontend): definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')

[rnk]

• use %clang_cc1 as possible in tests

[rnk]

In D72405#1818237, @zahen wrote:

I had no luck converting the CHECK-FOO & CHECK-NOFOO tests to use -verify because the errors were reported against "(frontend)"

I converted what I could over to -verify and switched to testing clang -cc1.

I think this looks good, I'm going to land it, since that's what I've done for you in the past. We have git now, it'll be attributed properly and all that. :)

Let me know if you get commit access in the future to push your own patches.

[ormris]

Hi Zach,

The tests in this patch are failing on this PS4 windows bot. Could you take a look?

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/30273/steps/test-check-all/logs/stdio

Thanks!

[zahen]

Wild guess is that 2> %t.err should be removed from the -verify lines? That change passes in the single env I have access to. @rnk any idea what might be going on?

clang/lib/Lex/PPDirectives.cpp

if (NumTokens != 0) {
     }
   }
 
   // When skipping just warn about macros that do not match.
   if (SkippingUntilPCHThroughHeader) {
     const MacroInfo *OtherMI = getMacroInfo(MacroNameTok.getIdentifierInfo());
     if (!OtherMI || !MI->isIdenticalTo(*OtherMI, *this,
                              /*Syntactic=*/LangOpts.MicrosoftExt))
       Diag(MI->getDefinitionLoc(), diag::warn_pp_macro_def_mismatch_with_pch)
           << MacroNameTok.getIdentifierInfo();
+    // Issue the diagnostic but allow the change if msvc extensions are enabled
+    if (!LangOpts.MicrosoftExt)
       return;
   }
 
   // Finally, if this identifier already had a macro defined for it, verify that
   // the macro bodies are identical, and issue diagnostics if they are not.
   if (const MacroInfo *OtherMI=getMacroInfo(MacroNameTok.getIdentifierInfo())) {
     // In Objective-C, ignore attempts to directly redefine the builtin
     // definitions of the ownership qualifiers.  It's still possible to
     // #undef them.

clang/test/PCH/ms-pch-macro.c

+// Test -D and -U interaction with a PCH when -fms-extensions is enabled.
+
+// RUN: %clang_cc1 -DFOO %S/variables.h -emit-pch -o %t1.pch
+
+// RUN: not %clang_cc1 -DFOO=blah -DBAR=int -include-pch %t1.pch -pch-through-header=%S/variables.h %s 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-FOO %s < %t.err
+
+// RUN: not %clang_cc1 -UFOO -DBAR=int -include-pch %t1.pch %s -pch-through-header=%S/variables.h 2> %t.err
+// RUN: FileCheck -check-prefix=CHECK-NOFOO %s < %t.err
+
+// RUN: %clang_cc1 -include-pch %t1.pch -DBAR=int -pch-through-header=%S/variables.h -verify %s 2> %t.err
+
+// Enabling MS extensions should allow us to add BAR definitions.
+// RUN: %clang_cc1 -fms-extensions -DFOO %S/variables.h -emit-pch -o %t1.pch
+// RUN: %clang_cc1 -fms-extensions -include-pch %t1.pch -DBAR=int -pch-through-header=%S/variables.h -verify %s 2> %t.err
+
+#include "variables.h"
+
+BAR bar = 17;
+#ifndef _MSC_EXTENSIONS
+// expected-error@-2 {{unknown type name 'BAR'}}
+#endif
+
+#ifndef FOO
+#  error FOO was not defined
+#endif
+
+#if FOO != 1
+#  error FOO has the wrong definition
+#endif
+
+#if !defined(_MSC_EXTENSIONS) && !defined(BAR)
+#  error BAR was not defined
+#endif
+
+// CHECK-FOO: definition of macro 'FOO' differs between the precompiled header ('1') and the command line ('blah')
+// CHECK-NOFOO: macro 'FOO' was defined in the precompiled header but undef'd on the command line
+
+// expected-warning@1 {{definition of macro 'BAR' does not match definition in precompiled header}}