Page MenuHomePhabricator

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

Authored by zahen on Jan 8 2020, 11:13 AM.

Details

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.

Event Timeline

zahen created this revision.Jan 8 2020, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2020, 11:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 added a comment.Jan 9 2020, 8:08 PM

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.

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 updated this revision to Diff 237795.Jan 13 2020, 3:26 PM

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')
zahen marked an inline comment as done.Jan 14 2020, 11:57 AM
rnk updated this revision to Diff 238147.Jan 14 2020, 5:12 PM
  • use %clang_cc1 as possible in tests
rnk accepted this revision.Jan 14 2020, 5:16 PM

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.

This revision is now accepted and ready to land.Jan 14 2020, 5:16 PM
This revision was automatically updated to reflect the committed changes.
ormris added a subscriber: ormris.Jan 14 2020, 6:22 PM

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 added a comment.Jan 14 2020, 7:37 PM

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?