Add -f[no-]declspec to control recognition of __declspec as a keyword

[wristow]

Summary

In versions of clang prior to r238238, declspec was recognized as a
keyword in all modes. It was then changed to only be enabled when
Microsoft or Borland extensions were enabled (and for CUDA, as a
temporary measure). There is a desire to support declspec in
Playstation code, and possibly other environments. This commit adds a
command-line switch to allow explicit enabling/disabling of the
recognition of __declspec as a keyword. Recognition is enabled by
default in Microsoft, Borland, CUDA and PS4 environments, and disabled
in all other environments.

This proposed change supersedes D11207. In D11207, it was proposed that a new language-dialect be added to Clang, to support extensions used in the Playstation environment. The primary motivation of that language-dialect was the recognition of declspec as a keyword. After consideration, it was suggested that adding a switch to independently control the recognition of declspec was an alternative to consider. See:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20150713/133310.html

As a result of that, this proposed change adds a switch to control the recognition of __declspec.

Can someone review, and if OK then commit this patch for me, please?

-Warren Ristow
SN Systems - Sony Computer Entertainment Group

[compnerd]

include/clang/Basic/LangOptions.def

93

Im not sure I care for Microsoft here. This is an extension that is supported on more than one compiler suite. How about "Enable support for __declspec extension"?

include/clang/Driver/Options.td

520

Shouldn't these be DriverOptions, and CC1Options?

lib/Driver/Tools.cpp

4663

The Args.hasFlag will correctly give you the value (-fdeclspec -fno-declspec will be treated as -fno-declspec). In fact, doesn't your implementation make -fno-declspec take precedence?

Plus, you marked these as cc1options above, not driver options. These aren't technically available here.

[rsmith]

include/clang/Basic/LangOptions.def

93

These strings are used in a diagnostic of the form "support for %0 is not enabled", so "__declspec" or "__declspec keyword" would be appropriate.

include/clang/Driver/Options.td

520

This should be in f_clang_Group, not f_Group.

lib/Driver/Tools.cpp

4663

The options are in the driver's Options.td and marked as CC1Option, so they're available in both the driver and cc1.

[wristow]

include/clang/Basic/LangOptions.def

93

I see. Then I'll change it to "__declspec keyword", to both remove the Microsoft reference and make it appropriate for the diagnostic formed from it.

include/clang/Driver/Options.td

520

I'll upload an updated patch shortly that takes care of this.

lib/Driver/Tools.cpp

4663

OK, then I'll leave them marked as CC1Option.

4663

Regarding the -fno-declspec taking precedence question, I think I've got it right, but I could have misunderstood something. There is a complication with this declspec-keyword recognition, in that in addition to it being enabled by default for PS4, it's implicitly enabled by default with any of -fms-compatibility, -fms-extensions and -fborland-extensions (and temporarily also for CUDA). My goal of -fno-declspec is that it should over-ride any implicit enabling of declspec. The implicit enabling was handled in cc1 itself (by inspecting values that depend on -fms-compatibility, -fms-extensions, -fborland-extensions, and CUDA). I left that being handled in cc1, although I considered collecting that information here in the driver, and passing a more elaborate expression as the 'Default' arg to hasFlag(), in which case I wouldn't have needed to explicitly push_back("-fno-declspec"). Anyway, with implicit enabling of declspec happening in cc1, it was easy to over-ride it by explicitly passing -fno-declspec through, iff that was the last -fdeclspec/-fno-declspec arg passed by the user. So that's what I'm doing on the line with the "// Explicitly disabling __declspec." comment (unless I've misunderstood, if -fno-declspec isn't the last one, we don't get to that line).

[wristow]

Updated patch to change the Group of the diagnostic from f_Group to f_clang_Group, and to change the string associated with the LANGOPT definition of the flag. Also fixed a minor typo in a comment.

[compnerd]

lib/Driver/Tools.cpp

4663

@rsmith, so they are. I misread something and was thinking they were under CC1Options.td.

4663

Sure, but you are still giving -fno-declspec precedence over -fdeclspec. Consider -fno-declspec -fdeclspec. I think what you want is Args.getLastArg, not Args.hasArg.

[wristow]

Added 4 new tests, the verify the last -fdeclspec/-fno-declspec wins, both in the context of -fms-extensions and without -fms-extensions.

[wristow]

lib/Driver/Tools.cpp

4663

But in the '-fno-declspec -fdeclspec' case, the 'if' clause "wins", and we never even reach the test of the 'else if' clause. As I said at the end of my previous comment, if -fno-declspec isn't the last one, we don't get to that line. That said, given the interaction with "implicit enabling" of declspec (via Microsoft, Borland, CUDA), it's a bit different than vanilla switch-handling. So I've updated the tests to explicitly have some checks for '-fno-declspec -fdeclspec' case (and the reverse, and for with and without -fms-extensions interacting).

[compnerd]

Committed as SVN r249279 (as per your request in the initial revision).

include/clang/Basic/LangOptions.def

 BENIGN_LANGOPT(ObjCInferRelatedResultType , 1, 1,
                "Objective-C related result type inference")
 LANGOPT(AppExt , 1, 0, "Objective-C App Extension")
 LANGOPT(Trigraphs         , 1, 0,"trigraphs")
 LANGOPT(LineComment       , 1, 0, "'//' comments")
 LANGOPT(Bool              , 1, 0, "bool, true, and false keywords")
 LANGOPT(Half              , 1, 0, "half keyword")
 LANGOPT(WChar             , 1, CPlusPlus, "wchar_t keyword")
+LANGOPT(DeclSpecKeyword   , 1, 0, "Microsoft __declspec keyword support")
 BENIGN_LANGOPT(DollarIdents   , 1, 1, "'$' in identifiers")
 BENIGN_LANGOPT(AsmPreprocessor, 1, 0, "preprocessor in asm mode")
 BENIGN_LANGOPT(GNUMode        , 1, 1, "GNU extensions")
 LANGOPT(GNUKeywords       , 1, 1, "GNU keywords")
 BENIGN_LANGOPT(ImplicitInt, 1, !C99 && !CPlusPlus, "C89 implicit 'int'")
 LANGOPT(Digraphs          , 1, 0, "digraphs")
 BENIGN_LANGOPT(HexFloats  , 1, C99, "C99 hexadecimal float constants")
 LANGOPT(CXXOperatorNames  , 1, 0, "C++ operator name keywords")

include/clang/Basic/TokenKinds.def

 TYPE_TRAIT_2(__is_convertible, IsConvertible, KEYCXX)
 KEYWORD(__array_rank                , KEYCXX)
 KEYWORD(__array_extent              , KEYCXX)
 
 // Apple Extension.
 KEYWORD(__private_extern__          , KEYALL)
 KEYWORD(__module_private__          , KEYALL)
 
+// Extension that will enabled for Microsoft, Borland and PS4, but can be
+// disabled via '-fno-declspec'.
+KEYWORD(__declspec                  , 0)
+
 // Microsoft Extension.
-KEYWORD(__declspec                  , KEYMS|KEYBORLAND)
 KEYWORD(__cdecl                     , KEYALL)
 KEYWORD(__stdcall                   , KEYALL)
 KEYWORD(__fastcall                  , KEYALL)
 KEYWORD(__thiscall                  , KEYALL)
 KEYWORD(__vectorcall                , KEYALL)
 KEYWORD(__forceinline               , KEYMS)
 KEYWORD(__unaligned                 , KEYMS)
 KEYWORD(__super                     , KEYMS)

include/clang/Driver/Options.td

def fdiagnostics_show_option : Flag<["-"], "fdiagnostics-show-option">, Group<f_Group>,
     Flags<[CC1Option]>, HelpText<"Print option name with mappable diagnostics">;
 def fdiagnostics_show_note_include_stack : Flag<["-"], "fdiagnostics-show-note-include-stack">,
     Group<f_Group>,  Flags<[CC1Option]>, HelpText<"Display include stacks for diagnostic notes">;
 def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Group<f_clang_Group>;
 def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Group<f_clang_Group>;
 def fdiagnostics_show_template_tree : Flag<["-"], "fdiagnostics-show-template-tree">,
     Group<f_Group>, Flags<[CC1Option]>,
     HelpText<"Print a template comparison tree for differing templates">;
+def fdeclspec : Flag<["-"], "fdeclspec">, Group<f_Group>,
+  HelpText<"Allow __declspec as a keyword">, Flags<[CC1Option]>;
 def fdollars_in_identifiers : Flag<["-"], "fdollars-in-identifiers">, Group<f_Group>,
   HelpText<"Allow '$' in identifiers">, Flags<[CC1Option]>;
 def fdwarf2_cfi_asm : Flag<["-"], "fdwarf2-cfi-asm">, Group<clang_ignored_f_Group>;
 def fno_dwarf2_cfi_asm : Flag<["-"], "fno-dwarf2-cfi-asm">, Group<clang_ignored_f_Group>;
 def fdwarf_directory_asm : Flag<["-"], "fdwarf-directory-asm">, Group<f_Group>;
 def fno_dwarf_directory_asm : Flag<["-"], "fno-dwarf-directory-asm">, Group<f_Group>, Flags<[CC1Option]>;
 def felide_constructors : Flag<["-"], "felide-constructors">, Group<f_Group>;
 def fno_elide_type : Flag<["-"], "fno-elide-type">, Group<f_Group>,

 def fno_cxx_exceptions: Flag<["-"], "fno-cxx-exceptions">, Group<f_Group>;
 def fno_cxx_modules : Flag <["-"], "fno-cxx-modules">, Group<f_Group>,
   Flags<[DriverOption]>;
 def fno_diagnostics_fixit_info : Flag<["-"], "fno-diagnostics-fixit-info">, Group<f_Group>,
   Flags<[CC1Option]>, HelpText<"Do not include fixit information in diagnostics">;
 def fno_diagnostics_show_option : Flag<["-"], "fno-diagnostics-show-option">, Group<f_Group>;
 def fno_diagnostics_show_note_include_stack : Flag<["-"], "fno-diagnostics-show-note-include-stack">,
     Flags<[CC1Option]>, Group<f_Group>;
+def fno_declspec : Flag<["-"], "fno-declspec">, Group<f_Group>,
+  HelpText<"Disallow __declspec as a keyword">, Flags<[CC1Option]>;
 def fno_dollars_in_identifiers : Flag<["-"], "fno-dollars-in-identifiers">, Group<f_Group>,
   HelpText<"Disallow '$' in identifiers">, Flags<[CC1Option]>;
 def fno_elide_constructors : Flag<["-"], "fno-elide-constructors">, Group<f_Group>,
   HelpText<"Disable C++ copy constructor elision">, Flags<[CC1Option]>;
 def fno_eliminate_unused_debug_symbols : Flag<["-"], "fno-eliminate-unused-debug-symbols">, Group<f_Group>;
 def fno_exceptions : Flag<["-"], "fno-exceptions">, Group<f_Group>;
 def fno_gnu_keywords : Flag<["-"], "fno-gnu-keywords">, Group<f_Group>, Flags<[CC1Option]>;
 def fno_inline_functions : Flag<["-"], "fno-inline-functions">, Group<f_clang_Group>, Flags<[CC1Option]>;

include/clang/Parse/Parser.h

void MaybeParseMicrosoftAttributes(ParsedAttributes &attrs,
     if (getLangOpts().MicrosoftExt && Tok.is(tok::l_square))
       ParseMicrosoftAttributes(attrs, endLoc);
   }
   void ParseMicrosoftAttributes(ParsedAttributes &attrs,
                                 SourceLocation *endLoc = nullptr);
   void MaybeParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                     SourceLocation *End = nullptr) {
     const auto &LO = getLangOpts();
-    if ((LO.MicrosoftExt || LO.Borland || LO.CUDA) &&
-        Tok.is(tok::kw___declspec))
+    if (LO.DeclSpecKeyword && Tok.is(tok::kw___declspec))
       ParseMicrosoftDeclSpecs(Attrs, End);
   }
   void ParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                SourceLocation *End = nullptr);
   bool ParseMicrosoftDeclSpecArgs(IdentifierInfo *AttrName,
                                   SourceLocation AttrNameLoc,
                                   ParsedAttributes &Attrs);
   void ParseMicrosoftTypeAttributes(ParsedAttributes &attrs);

lib/Basic/IdentifierTable.cpp

if (LangOpts.ObjC2)          \
     AddObjCKeyword(StringRef(#NAME), tok::objc_##NAME, *this);
 #define TESTING_KEYWORD(NAME, FLAGS)
 #include "clang/Basic/TokenKinds.def"
 
   if (LangOpts.ParseUnknownAnytype)
     AddKeyword("__unknown_anytype", tok::kw___unknown_anytype, KEYALL,
                LangOpts, *this);
 
-  // FIXME: __declspec isn't really a CUDA extension, however it is required for
-  // supporting cuda_builtin_vars.h, which uses __declspec(property). Once that
-  // has been rewritten in terms of something more generic, remove this code.
-  if (LangOpts.CUDA)
+  if (LangOpts.DeclSpecKeyword)
     AddKeyword("__declspec", tok::kw___declspec, KEYALL, LangOpts, *this);
 }
 
 /// \brief Checks if the specified token kind represents a keyword in the
 /// specified language.
 /// \returns Status of the keyword in the language.
 static KeywordStatus getTokenKwStatus(const LangOptions &LangOpts,
                                       tok::TokenKind K) {

lib/Driver/Tools.cpp

else
       CmdArgs.push_back("-std=c++11");
   }
 
   // -fno-borland-extensions is default.
   if (Args.hasFlag(options::OPT_fborland_extensions,
                    options::OPT_fno_borland_extensions, false))
     CmdArgs.push_back("-fborland-extensions");
 
+  // -fno-declspec is default, except for PS4.
+  if (Args.hasFlag(options::OPT_fdeclspec, options::OPT_fno_declspec,
+                   getToolChain().getTriple().isPS4()))
+    CmdArgs.push_back("-fdeclspec");
+  else if (Args.hasArg(options::OPT_fno_declspec))
+    CmdArgs.push_back("-fno-declspec"); // Explicitly disabling __declspec.
+
   // -fthreadsafe-static is default, except for MSVC compatibility versions less
   // than 19.
   if (!Args.hasFlag(options::OPT_fthreadsafe_statics,
                     options::OPT_fno_threadsafe_statics,
                     !IsWindowsMSVC || IsMSVC2015Compatible))
     CmdArgs.push_back("-fno-threadsafe-statics");
 
   // -fno-delayed-template-parsing is default, except for Windows where MSVC STL

lib/Frontend/CompilerInvocation.cpp

#include "clang/Frontend/LangStandards.def"
   Opts.ImplementationOfModule =
       Args.getLastArgValue(OPT_fmodule_implementation_of);
   Opts.ModuleFeatures = Args.getAllArgValues(OPT_fmodule_feature);
   std::sort(Opts.ModuleFeatures.begin(), Opts.ModuleFeatures.end());
   Opts.NativeHalfType |= Args.hasArg(OPT_fnative_half_type);
   Opts.HalfArgsAndReturns = Args.hasArg(OPT_fallow_half_arguments_and_returns);
   Opts.GNUAsm = !Args.hasArg(OPT_fno_gnu_inline_asm);
 
+  // __declspec is enabled by default for the PS4 by the driver, and also
+  // enabled for Microsoft Extensions or Borland Extensions, here.
+  //
+  // FIXME: __declspec is also currently enabled for CUDA, but isn't really a
+  // CUDA extension, however it is required for supporting cuda_builtin_vars.h,
+  // which uses __declspec(property). Once that has been rewritten in terms of
+  // something more generic, remove the Opts.CUDA term here.
+  Opts.DeclSpecKeyword =
+      Args.hasFlag(OPT_fdeclspec, OPT_fno_declspec,
+                   (Opts.MicrosoftExt || Opts.Borland || Opts.CUDA));
+
   if (!Opts.CurrentModule.empty() && !Opts.ImplementationOfModule.empty() &&
       Opts.CurrentModule != Opts.ImplementationOfModule) {
     Diags.Report(diag::err_conflicting_module_names)
         << Opts.CurrentModule << Opts.ImplementationOfModule;
   }
 
   // For now, we only support local submodule visibility in C++ (because we
   // heavily depend on the ODR for merging redefinitions).

lib/Parse/ParseDecl.cpp

 /// [MS] decl-specifier:
 ///             __declspec ( extended-decl-modifier-seq )
 ///
 /// [MS] extended-decl-modifier-seq:
 ///             extended-decl-modifier[opt]
 ///             extended-decl-modifier extended-decl-modifier-seq
 void Parser::ParseMicrosoftDeclSpecs(ParsedAttributes &Attrs,
                                      SourceLocation *End) {
-  assert((getLangOpts().MicrosoftExt || getLangOpts().Borland ||
-          getLangOpts().CUDA) &&
-         "Incorrect language options for parsing __declspec");
+  assert(getLangOpts().DeclSpecKeyword && "__declspec keyword is not enabled");
   assert(Tok.is(tok::kw___declspec) && "Not a declspec!");
 
   while (Tok.is(tok::kw___declspec)) {
     ConsumeToken();
     BalancedDelimiterTracker T(*this, tok::l_paren);
     if (T.expectAndConsume(diag::err_expected_lparen_after, "__declspec",
                            tok::r_paren))
       return;

test/Lexer/keywords_test.c

 // RUN: %clang_cc1 -std=c99 -E %s -o - | FileCheck --check-prefix=CHECK-NONE %s
 
 // RUN: %clang_cc1 -std=gnu89 -E %s -o - \
 // RUN:     | FileCheck --check-prefix=CHECK-GNU-KEYWORDS %s
 // RUN: %clang_cc1 -std=c99 -fgnu-keywords -E %s -o - \
 // RUN:     | FileCheck --check-prefix=CHECK-GNU-KEYWORDS %s
 // RUN: %clang_cc1 -std=gnu89 -fno-gnu-keywords -E %s -o - \
 // RUN:     | FileCheck --check-prefix=CHECK-NONE %s
 
 // RUN: %clang_cc1 -std=c99 -fms-extensions -E %s -o - \
 // RUN:     | FileCheck --check-prefix=CHECK-MS-KEYWORDS %s
+// RUN: %clang_cc1 -std=c99 -fdeclspec -E %s -o - \
+// RUN:     | FileCheck --check-prefix=CHECK-DECLSPEC-KEYWORD %s
+// RUN: %clang_cc1 -std=c99 -fms-extensions -fno-declspec -E %s -o - \
+// RUN:     | FileCheck --check-prefix=CHECK-MS-KEYWORDS-WITHOUT-DECLSPEC %s
 
 void f() {
 // CHECK-NONE: int asm
 // CHECK-GNU-KEYWORDS: asm ("ret" : :)
 #if __is_identifier(asm)
   int asm;
 #else
   asm ("ret" : :);
 #endif
 }
 
 // CHECK-NONE: no_ms_wchar
 // CHECK-MS-KEYWORDS: has_ms_wchar
+// CHECK-MS-KEYWORDS-WITHOUT-DECLSPEC: has_ms_wchar
 #if __is_identifier(__wchar_t)
 void no_ms_wchar();
 #else
 void has_ms_wchar();
 #endif
+
+// CHECK-NONE: no_declspec
+// CHECK-MS-KEYWORDS: has_declspec
+// CHECK-MS-KEYWORDS-WITHOUT-DECLSPEC: no_declspec
+// CHECK-DECLSPEC-KEYWORD: has_declspec
+#if __is_identifier(__declspec)
+void no_declspec();
+#else
+void has_declspec();
+#endif

test/Lexer/keywords_test.cpp

 // RUN: %clang_cc1 -std=c++03 -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++11 -DCXX11 -fsyntax-only %s
 // RUN: %clang_cc1 -std=c++14 -fconcepts-ts -DCXX11 -DCONCEPTS -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++03 -fdeclspec -DDECLSPEC -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++03 -fms-extensions -DDECLSPEC -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++03 -fborland-extensions -DDECLSPEC -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++03 -fms-extensions -fno-declspec -fsyntax-only %s
+// RUN: %clang_cc1 -std=c++03 -fborland-extensions -fno-declspec -fsyntax-only %s
+// RUN: %clang -std=c++03 -target i686-windows-msvc -DDECLSPEC -fsyntax-only %s
+// RUN: %clang -std=c++03 -target x86_64-scei-ps4 -DDECLSPEC -fsyntax-only %s
+// RUN: %clang -std=c++03 -target i686-windows-msvc -fno-declspec -fsyntax-only %s
+// RUN: %clang -std=c++03 -target x86_64-scei-ps4 -fno-declspec -fsyntax-only %s
 
 #define IS_KEYWORD(NAME) _Static_assert(!__is_identifier(NAME), #NAME)
 #define NOT_KEYWORD(NAME) _Static_assert(__is_identifier(NAME), #NAME)
 #define IS_TYPE(NAME) void is_##NAME##_type() { int f(NAME); }
 
 #ifdef CONCEPTS
 #define CONCEPTS_KEYWORD(NAME)  IS_KEYWORD(NAME)
 #else
 #define CONCEPTS_KEYWORD(NAME)  NOT_KEYWORD(NAME)
 #endif
 
+#ifdef DECLSPEC
+#define DECLSPEC_KEYWORD(NAME)  IS_KEYWORD(NAME)
+#else
+#define DECLSPEC_KEYWORD(NAME)  NOT_KEYWORD(NAME)
+#endif
+
 #ifdef CXX11
 #define CXX11_KEYWORD(NAME)  IS_KEYWORD(NAME)
 #define CXX11_TYPE(NAME)     IS_TYPE(NAME)
 #else
 #define CXX11_KEYWORD(NAME)  NOT_KEYWORD(NAME)
 #define CXX11_TYPE(NAME)
 #endif
 

 CXX11_KEYWORD(noexcept);
 CXX11_KEYWORD(static_assert);
 CXX11_KEYWORD(thread_local);
 
 // Concepts TS keywords
 CONCEPTS_KEYWORD(concept);
 CONCEPTS_KEYWORD(requires);
 
+// __declspec extension
+DECLSPEC_KEYWORD(__declspec);
+
 // Clang extension
 IS_KEYWORD(__char16_t);
 IS_TYPE(__char16_t);
 IS_KEYWORD(__char32_t);
 IS_TYPE(__char32_t);