Fix for #pragma warning to work correctly with "1-4:" specifiers

[andreybokhanko]

Summary

Bug fix for PR23577 (https://llvm.org/bugs/show_bug.cgi?id=23577#c0).

"1-4" specifiers are returned as numeric constants, not identifiers, and should be treated as such. Currently pragma handler incorrectly assumes that they are returned as identifiers.

[rnk]

Can you add a -E test that verifies that these pragmas roundtrip through the pre-processor?

lib/Lex/Pragma.cpp

1075 ↗

(On Diff #26067)

Make sure the SmallString has the same lifetime as Specifier.

1091 ↗

(On Diff #26067)

utostr returns a std::string, so this would be a use-after-free. Use getSpelling() to get a StringRef that points into the original source code so we don't need memory allocation. You'll need to supply a SmallString, but 99% of the time it'll be unused.

[andreybokhanko]

Reid, thank you for the review!

I fixed the use-after-free bug you found (good catch, BTW!) I also created a test that checks -E (not sure this is exactly what you wanted; let me know if I misunderstood you).

[rnk]

test/Preprocessor/pragma_microsoft_E.c

1–3 ↗

(On Diff #26147)

Rather than copy-pasting the file, you can actually have multiple RUN lines in a single test. So back in pragma_microsoft.c you can add the same RUN line you have here and the CHECK lines and it'll all work.

83 ↗

(On Diff #26147)

You should check that the warning pragmas roundtrip through the preprocessor, though.

[andreybokhanko]

Test updated according to Reid's comments.

[andreybokhanko]

test/Preprocessor/pragma_microsoft_E.c

83 ↗

(On Diff #26147)

Reid, sorry for a noob question -- what do you mean by checking "roundtrip through the preprocessor"? Is there an existing test I can use as an example?

In the updated patch I added CHECKs for preprocessor's output -- is this what you meant?

[rnk]

Yes, check lines for pragma warning 1.

Sent from phone

[andreybokhanko]

Updated patch has these checks:

CHECK: #pragma warning(pop)
#pragma warning(1: 123)
CHECK: #pragma warning(1: 123)
#pragma warning(2: 234 567)
CHECK: #pragma warning(2: 234 567)
#pragma warning(3: 123; 4: 678)
CHECK: #pragma warning(3: 123)
CHECK: #pragma warning(4: 678)
#pragma warning(5: 123) expected-warning {{expected 'push', 'pop', 'default', 'disable', 'error', 'once', 'suppress', 1, 2, 3, or 4}}

[rnk]

lgtm, thanks!

[andreybokhanko]

Reid, thank you for the quick and thoughtful review! This is my first ever clang fix and it is very nice to get such a good reception.

I don't have commit rights yet; Alexander [Musman], could you, please, commit the fix?

cfe/trunk/lib/Lex/Pragma.cpp

 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/LiteralSupport.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 using namespace clang;
 
 #include "llvm/Support/raw_ostream.h"
 
 // Out-of-line destructor to provide a home for the class.
void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
     PP.Lex(Tok);
     if (Tok.isNot(tok::l_paren)) {
       PP.Diag(Tok, diag::warn_pragma_warning_expected) << "(";
       return;
     }
 
     PP.Lex(Tok);
     IdentifierInfo *II = Tok.getIdentifierInfo();
-    if (!II) {
-      PP.Diag(Tok, diag::warn_pragma_warning_spec_invalid);
-      return;
-    }
 
-    if (II->isStr("push")) {
+    if (II && II->isStr("push")) {
       // #pragma warning( push[ ,n ] )
       int Level = -1;
       PP.Lex(Tok);
       if (Tok.is(tok::comma)) {
         PP.Lex(Tok);
         uint64_t Value;
         if (Tok.is(tok::numeric_constant) &&
             PP.parseSimpleIntegerLiteral(Tok, Value))
           Level = int(Value);
         if (Level < 0 || Level > 4) {
           PP.Diag(Tok, diag::warn_pragma_warning_push_level);
           return;
         }
       }
       if (Callbacks)
         Callbacks->PragmaWarningPush(DiagLoc, Level);
-    } else if (II->isStr("pop")) {
+    } else if (II && II->isStr("pop")) {
       // #pragma warning( pop )
       PP.Lex(Tok);
       if (Callbacks)
         Callbacks->PragmaWarningPop(DiagLoc);
     } else {
       // #pragma warning( warning-specifier : warning-number-list
       //                  [; warning-specifier : warning-number-list...] )
       while (true) {
         II = Tok.getIdentifierInfo();
-        if (!II) {
+        if (!II && !Tok.is(tok::numeric_constant)) {
           PP.Diag(Tok, diag::warn_pragma_warning_spec_invalid);
           return;
         }
 
         // Figure out which warning specifier this is.
-        StringRef Specifier = II->getName();
-        bool SpecifierValid =
-            llvm::StringSwitch<bool>(Specifier)
-                .Cases("1", "2", "3", "4", true)
-                .Cases("default", "disable", "error", "once", "suppress", true)
+        bool SpecifierValid;
+        StringRef Specifier;
+        llvm::SmallString<1> SpecifierBuf;
+        if (II) {
+          Specifier = II->getName();
+          SpecifierValid = llvm::StringSwitch<bool>(Specifier)
+                               .Cases("default", "disable", "error", "once",
+                                      "suppress", true)
                                .Default(false);
+          // If we read a correct specifier, snatch next token (that should be
+          // ":", checked later).
+          if (SpecifierValid)
+            PP.Lex(Tok);
+        } else {
+          // Token is a numeric constant. It should be either 1, 2, 3 or 4.
+          uint64_t Value;
+          Specifier = PP.getSpelling(Tok, SpecifierBuf);
+          if (PP.parseSimpleIntegerLiteral(Tok, Value)) {
+            SpecifierValid = (Value >= 1) && (Value <= 4);
+          } else
+            SpecifierValid = false;
+          // Next token already snatched by parseSimpleIntegerLiteral.
+        }
+
         if (!SpecifierValid) {
           PP.Diag(Tok, diag::warn_pragma_warning_spec_invalid);
           return;
         }
-        PP.Lex(Tok);
         if (Tok.isNot(tok::colon)) {
           PP.Diag(Tok, diag::warn_pragma_warning_expected) << ":";
           return;
         }
 
         // Collect the warning ids.
         SmallVector<int, 4> Ids;
         PP.Lex(Tok);

cfe/trunk/test/Preprocessor/pragma_microsoft.c

 // RUN: %clang_cc1 %s -fsyntax-only -verify -fms-extensions
+// RUN: not %clang_cc1 %s -fms-extensions -E | FileCheck %s
 // REQUIRES: non-ps4-sdk
 
 // rdar://6495941
 
 #define FOO 1
 #define BAR "2"
 
 #pragma comment(linker,"foo=" FOO) // expected-error {{pragma comment requires parenthesized identifier and optional string}}
+// CHECK: #pragma comment(linker,"foo=" 1)
 #pragma comment(linker," bar=" BAR)
+// CHECK: #pragma comment(linker," bar=" "2")
 
 #pragma comment( user, "Compiled on " __DATE__ " at " __TIME__ ) 
+// CHECK: {{#pragma comment\( user, \"Compiled on \".*\" at \".*\" \)}}
 
 #pragma comment(foo)    // expected-error {{unknown kind of pragma comment}}
+// CHECK: #pragma comment(foo)
 #pragma comment(compiler,)     // expected-error {{expected string literal in pragma comment}}
+// CHECK: #pragma comment(compiler,)
 #define foo compiler
 #pragma comment(foo)   // macro expand kind.
+// CHECK: #pragma comment(compiler)
 #pragma comment(foo) x // expected-error {{pragma comment requires}}
+// CHECK: #pragma comment(compiler) x
 
 #pragma comment(user, "foo\abar\nbaz\tsome thing")
+// CHECK: #pragma comment(user, "foo\abar\nbaz\tsome thing")
 
 #pragma detect_mismatch("test", "1")
+// CHECK: #pragma detect_mismatch("test", "1")
 #pragma detect_mismatch()  // expected-error {{expected string literal in pragma detect_mismatch}}
+// CHECK: #pragma detect_mismatch()
 #pragma detect_mismatch("test") // expected-error {{pragma detect_mismatch is malformed; it requires two comma-separated string literals}}
+// CHECK: #pragma detect_mismatch("test")
 #pragma detect_mismatch("test", 1) // expected-error {{expected string literal in pragma detect_mismatch}}
+// CHECK: #pragma detect_mismatch("test", 1)
 #pragma detect_mismatch("test", BAR)
+// CHECK: #pragma detect_mismatch("test", "2")
 
 // __pragma
 
-__pragma(comment(linker," bar=" BAR))                                            
+__pragma(comment(linker," bar=" BAR))
+// CHECK: #pragma comment(linker," bar=" "2")
 
 #define MACRO_WITH__PRAGMA { \
   __pragma(warning(push)); \
   __pragma(warning(disable: 10000)); \
   1 + (2 > 3) ? 4 : 5; \
   __pragma(warning(pop)); \
 }
 
 void f()
 {
   __pragma()
+// CHECK: #pragma
 
   // If we ever actually *support* __pragma(warning(disable: x)),
   // this warning should go away.
   MACRO_WITH__PRAGMA // expected-warning {{lower precedence}} \
                      // expected-note 2 {{place parentheses}}
+// CHECK: #pragma warning(push)
+// CHECK: #pragma warning(disable: 10000)
+// CHECK: ; 1 + (2 > 3) ? 4 : 5;
+// CHECK: #pragma warning(pop)
 }
 
 
 // This should include macro_arg_directive even though the include
 // is looking for test.h  This allows us to assign to "n"
 #pragma include_alias("test.h", "macro_arg_directive.h" )
 #include "test.h"
 void test( void ) {

 #include "quux.h"
 
 // Make sure that empty includes don't work
 #pragma include_alias("", "foo.h")  // expected-error {{empty filename}}
 #pragma include_alias(<foo.h>, <>)  // expected-error {{empty filename}}
 
 // Test that we ignore pragma warning.
 #pragma warning(push)
+// CHECK: #pragma warning(push)
 #pragma warning(push, 1)
+// CHECK: #pragma warning(push, 1)
 #pragma warning(disable : 4705)
+// CHECK: #pragma warning(disable: 4705)
 #pragma warning(disable : 123 456 789 ; error : 321)
+// CHECK: #pragma warning(disable: 123 456 789)
+// CHECK: #pragma warning(error: 321)
 #pragma warning(once : 321)
+// CHECK: #pragma warning(once: 321)
 #pragma warning(suppress : 321)
+// CHECK: #pragma warning(suppress: 321)
 #pragma warning(default : 321)
+// CHECK: #pragma warning(default: 321)
 #pragma warning(pop)
+// CHECK: #pragma warning(pop)
+#pragma warning(1: 123)
+// CHECK: #pragma warning(1: 123)
+#pragma warning(2: 234 567)
+// CHECK: #pragma warning(2: 234 567)
+#pragma warning(3: 123; 4: 678)
+// CHECK: #pragma warning(3: 123)
+// CHECK: #pragma warning(4: 678)
+#pragma warning(5: 123) // expected-warning {{expected 'push', 'pop', 'default', 'disable', 'error', 'once', 'suppress', 1, 2, 3, or 4}}
 
 #pragma warning(push, 0)
+// CHECK: #pragma warning(push, 0)
 // FIXME: We could probably support pushing warning level 0.
 #pragma warning(pop)
+// CHECK: #pragma warning(pop)
 
 #pragma warning  // expected-warning {{expected '('}}
 #pragma warning(   // expected-warning {{expected 'push', 'pop', 'default', 'disable', 'error', 'once', 'suppress', 1, 2, 3, or 4}}
 #pragma warning()   // expected-warning {{expected 'push', 'pop', 'default', 'disable', 'error', 'once', 'suppress', 1, 2, 3, or 4}}
 #pragma warning(push 4)  // expected-warning {{expected ')'}}
+// CHECK: #pragma warning(push)
 #pragma warning(push  // expected-warning {{expected ')'}}
+// CHECK: #pragma warning(push)
 #pragma warning(push, 5)  // expected-warning {{requires a level between 0 and 4}}
 #pragma warning(pop, 1)  // expected-warning {{expected ')'}}
+// CHECK: #pragma warning(pop)
 #pragma warning(push, 1) asdf // expected-warning {{extra tokens at end of #pragma warning directive}}
+// CHECK: #pragma warning(push, 1)
 #pragma warning(disable 4705) // expected-warning {{expected ':'}}
 #pragma warning(disable : 0) // expected-warning {{expected a warning number}}
 #pragma warning(default 321) // expected-warning {{expected ':'}}
 #pragma warning(asdf : 321) // expected-warning {{expected 'push', 'pop'}}
 #pragma warning(push, -1) // expected-warning {{requires a level between 0 and 4}}