I don't think __<span></span>declspec(code_seg) should be part of this, it's complexity is more on the order of dllimport/dllexport as it has complex inheritance rules.<br><br>On Thursday, April 3, 2014, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Would it make sense to also add __declspec(code_seg) as part of this<br>
patch? <a href="http://msdn.microsoft.com/en-us/library/dn636922.aspx" target="_blank">http://msdn.microsoft.com/en-us/library/dn636922.aspx</a><br>
Especially since MSDN recommends it over #pragma code_seg.<br>
<br>
The patch is missing parser test cases.<br>
<br>
More comments below.<br>
<br>
> Index: include/clang/Basic/Attr.td<br>
> ===================================================================<br>
> --- include/clang/Basic/Attr.td<br>
> +++ include/clang/Basic/Attr.td<br>
> @@ -1069,7 +1069,7 @@<br>
> }<br>
><br>
> def Section : InheritableAttr {<br>
> - let Spellings = [GCC<"section">];<br>
> + let Spellings = [GCC<"section">, Declspec<"allocate">];<br>
> let Args = [StringArgument<"Name">];<br>
> let Subjects = SubjectList<[Function, GlobalVar,<br>
> ObjCMethod, ObjCProperty], ErrorDiag,<br>
<br>
I'd love to see:<br>
<br>
+ let Documentation = [Anything other than Undocumented];<br>
<br>
;-)<br>
<br>
> Index: include/clang/Basic/DiagnosticParseKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticParseKinds.td<br>
> +++ include/clang/Basic/DiagnosticParseKinds.td<br>
> @@ -771,6 +771,8 @@<br>
> "missing ')' after '#pragma %0' - ignoring">, InGroup<IgnoredPragmas>;<br>
> def warn_pragma_expected_identifier : Warning<<br>
> "expected identifier in '#pragma %0' - ignored">, InGroup<IgnoredPragmas>;<br>
> +def warn_pragma_expected_string : Warning<<br>
> + "expected string literal in '#pragma %0' - ignored">, InGroup<IgnoredPragmas>;<br>
<br>
This could be combined with warn_pragma_expected_identifier using a %select.<br>
<br>
> def warn_pragma_expected_integer : Warning<<br>
> "expected integer between %0 and %1 inclusive in '#pragma %2' - ignored">,<br>
> InGroup<IgnoredPragmas>;<br>
> Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
> ===================================================================<br>
> --- include/clang/Basic/DiagnosticSemaKinds.td<br>
> +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
> @@ -516,6 +516,7 @@<br>
> Warning<"ms_struct may not produce MSVC-compatible layouts for classes "<br>
> "with base classes or virtual functions">,<br>
> DefaultError, InGroup<IncompatibleMSStruct>;<br>
> +def err_section_conflict : Error<"%0 causes a section type conflict with %1">;<br>
><br>
> def warn_pragma_unused_undeclared_var : Warning<<br>
> "undeclared variable %0 used as an argument for '#pragma unused'">,<br>
> Index: include/clang/Basic/TokenKinds.def<br>
> ===================================================================<br>
> --- include/clang/Basic/TokenKinds.def<br>
> +++ include/clang/Basic/TokenKinds.def<br>
> @@ -684,6 +684,11 @@<br>
> // handles them.<br>
> ANNOTATION(pragma_ms_vtordisp)<br>
><br>
> +// Annotation for all microsoft #pragmas...<br>
> +// The lexer produces these so that they only take effect when the parser<br>
> +// handles them.<br>
> +ANNOTATION(pragma_ms_pragma)<br>
> +<br>
> // Annotation for #pragma OPENCL EXTENSION...<br>
> // The lexer produces these so that they only take effect when the parser<br>
> // handles them.<br>
> Index: include/clang/Parse/Parser.h<br>
> ===================================================================<br>
> --- include/clang/Parse/Parser.h<br>
> +++ include/clang/Parse/Parser.h<br>
> @@ -154,6 +154,11 @@<br>
> std::unique_ptr<PragmaHandler> MSDetectMismatchHandler;<br>
> std::unique_ptr<PragmaHandler> MSPointersToMembers;<br>
> std::unique_ptr<PragmaHandler> MSVtorDisp;<br>
> + std::unique_ptr<PragmaHandler> MSDataSeg;<br>
> + std::unique_ptr<PragmaHandler> MSBSSSeg;<br>
> + std::unique_ptr<PragmaHandler> MSConstSeg;<br>
> + std::unique_ptr<PragmaHandler> MSCodeSeg;<br>
> + std::unique_ptr<PragmaHandler> MSSection;<br>
><br>
> std::unique_ptr<CommentHandler> CommentSemaHandler;<br>
><br>
> @@ -475,6 +480,12 @@<br>
><br>
> void HandlePragmaMSVtorDisp();<br>
><br>
> + void HandlePragmaMSPragma();<br>
> + unsigned HandlePragmaMSSection(llvm::StringRef PragmaName,<br>
> + SourceLocation PragmaLocation);<br>
> + unsigned HandlePragmaMSXXXXSeg(llvm::StringRef PragmaName,<br>
> + SourceLocation PragmaLocation);<br>
<br>
I'm not too keen on XXXXSeg as the name. Perhaps<br>
HandlePragmaMSSegment? Sorry, did I mention I like my bikesheds red?<br>
<br>
> +<br>
> /// \brief Handle the annotation token produced for<br>
> /// #pragma align...<br>
> void HandlePragmaAlign();<br>
> Index: include/clang/Sema/Sema.h<br>
> ===================================================================<br>
> --- include/clang/Sema/Sema.h<br>
> +++ include/clang/Sema/Sema.h<br>
> @@ -274,6 +274,15 @@<br>
> PVDK_Reset ///< #pragma vtordisp()<br>
> };<br>
><br>
> + enum PragmaMsStackAction {<br>
> + PSK_Reset, // #pragma ()<br>
> + PSK_Set, // #pragma ("name")<br>
> + PSK_Push, // #pragma (push[, id])<br>
> + PSK_Push_Set, // #pragma (push[, id], "name")<br>
> + PSK_Pop, // #pragma (pop[, id])<br>
> + PSK_Pop_Set, // #pragma (pop[, id], "name")<br>
> + };<br>
> +<br>
> /// \brief Whether to insert vtordisps prior to virtual bases in the Microsoft<br>
> /// C++ ABI. Possible values are 0, 1, and 2, which mean:<br>
> ///<br>
> @@ -289,6 +298,30 @@<br>
> /// \brief Source location for newly created implicit MSInheritanceAttrs<br>
> SourceLocation ImplicitMSInheritanceAttrLoc;<br>
><br>
> + template<typename ValueType><br>
> + struct PragmaStack {<br>
> + struct Slot {<br>
> + llvm::StringRef StackSlotLabel;<br>
> + ValueType Value;<br>
> + SourceLocation PragmaLocation;<br>
> + };<br>
> + void Act(SourceLocation PragmaLocation,<br>
> + PragmaMsStackAction Action,<br>
> + llvm::StringRef StackSlotLabel,<br>
> + ValueType Value);<br>
> + explicit PragmaStack(const ValueType& Value)<br>
<br>
According to the style guidelines, the reference goes with the<br>
identifier, not the type. This applies across the entire patch (same<br>
with pointers).<br>
<br>
> + : CurrentValue(Value) {}<br>
> + SmallVector<Slot, 2> Stack;<br>
> + ValueType CurrentValue;<br>
> + SourceLocation CurrentPragmaLocation;<br>
> + };<br>
> + // FIXME: We should serialize / deserialize these if they occur in a PCH (but<br>
> + // we shouldn't do so if they're in a module).<br>
> + PragmaStack<StringLiteral *> DataSegStack;<br>
> + PragmaStack<StringLiteral *> BSSSegStack;<br>
> + PragmaStack<StringLiteral *> ConstSegStack;<br>
> + PragmaStack<StringLiteral *> CodeSegStack;<br>
> +<br>
> /// VisContext - Manages the stack for \#pragma GCC visibility.<br>
> void *VisContext; // Really a "PragmaVisStack*"<br>
><br>
> @@ -7030,6 +7063,46 @@<br>
> void ActOnPragmaMSVtorDisp(PragmaVtorDispKind Kind, SourceLocation PragmaLoc,<br>
> MSVtorDispAttr::Mode Value);<br>
><br>
> + enum PragmaMSKind {<br>
> + PSK_DataSeg,<br>
> + PSK_BSSSeg,<br>
> + PSK_ConstSeg,<br>
> + PSK_CodeSeg,<br>
> + };<br>
> +<br>
> + enum PragmaSectionFlag {<br>
> + PSF_None = 0,<br>
> + PSF_Read = 1,<br>
> + PSF_Write = 2,<br>
> + PSF_Execute = 4,<br>
> + PSF_Implicit = 8,<br>
> + };<br>
> +<br>
> + struct SectionInfo {<br>
> + DeclaratorDecl* Decl;<br>
> + SourceLocation PragmaSectionLocation;<br>
> + int SectionFlags;<br>
> + };<br>
> +<br>
> + llvm::StringMap<SectionInfo> SectionInfos;<br>
<br>
This doesn't need to be a public member, does it?<br>
<br>
> + bool UnifySection(const StringRef& SectionName,<br>
> + int SectionFlags,<br>
> + DeclaratorDecl* TheDecl);<br>
> + bool UnifySection(const StringRef& SectionName,<br>
> + int SectionFlags,<br>
> + SourceLocation PragmaSectionLocation);<br>
> +<br>
> + /// \brief Called on well formed \#pragma bss_seg/data_seg/const_seg/code_seg.<br>
> + void ActOnPragmaMSSeg(SourceLocation PragmaLocation,<br>
> + PragmaMsStackAction Action,<br>
> + llvm::StringRef StackSlotLabel,<br>
> + StringLiteral *SegmentName,<br>
> + llvm::StringRef PragmaName);<br>
> +<br>
> + /// \brief Called on well formed \#pragma section().<br>
> + void ActOnPragmaMSSection(SourceLocation PragmaLocation,<br>
> + int SectionFlags, StringLiteral *SegmentName);<br>
> +<br>
> /// ActOnPragmaDetectMismatch - Call on well-formed \#pragma detect_mismatch<br>
> void ActOnPragmaDetectMismatch(StringRef Name, StringRef Value);<br>
><br>
> Index: lib/Parse/ParseDeclCXX.cpp<br>
> ===================================================================<br>
> --- lib/Parse/ParseDeclCXX.cpp<br>
> +++ lib/Parse/ParseDeclCXX.cpp<br>
> @@ -2646,6 +2646,11 @@<br>
> continue;<br>
> }<br>
><br>
> + if (Tok.is(tok::annot_pragma_ms_pragma)) {<br>
> + HandlePragmaMSPragma();<br>
> + continue;<br>
> + }<br>
> +<br>
> // If we see a namespace here, a close brace was missing somewhere.<br>
> if (Tok.is(tok::kw_namespace)) {<br>
> DiagnoseUnexpectedNamespace(cast<NamedDecl>(TagDecl));<br>
> Index: lib/Parse/ParsePragma.cpp<br>
> ===================================================================<br>
> --- lib/Parse/ParsePragma.cpp<br>
> +++ lib/Parse/ParsePragma.cpp<br>
> @@ -11,6 +11,7 @@<br>
> //<br>
> //===----------------------------------------------------------------------===//<br>
><br>
> +#include "RAIIObjectsForParser.h"<br>
> #include "clang/Lex/Preprocessor.h"<br>
> #include "clang/Parse/ParseDiagnostic.h"<br>
> #include "clang/Parse/Parser.h"<br>
> @@ -124,6 +125,12 @@<br>
> Token &FirstToken) override;<br>
> };<br>
><br>
> +struct PragmaMSPragma : public PragmaHandler {<br>
> + explicit PragmaMSPragma(const char *name) : PragmaHandler(name) {}<br>
> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,<br>
> + Token &FirstToken);<br>
<br>
Marked as override and drop the virtual?<br>
<br>
> +};<br>
> +<br>
> } // end namespace<br>
><br>
> void Parser::initializePragmaHandlers() {<br>
> @@ -175,6 +182,16 @@<br>
> PP.AddPragmaHandler(MSPointersToMembers.get());<br>
> MSVtorDisp.reset(new PragmaMSVtorDisp());<br>
> PP.AddPragmaHandler(MSVtorDisp.get());<br>
> + MSDataSeg.reset(new PragmaMSPragma("data_seg"));<br>
> + PP.AddPragmaHandler(MSDataSeg.get());<br>
> + MSBSSSeg.reset(new PragmaMSPragma("bss_seg"));<br>
> + PP.AddPragmaHandler(MSBSSSeg.get());<br>
> + MSConstSeg.reset(new PragmaMSPragma("const_seg"));<br>
> + PP.AddPragmaHandler(MSConstSeg.get());<br>
> + MSCodeSeg.reset(new PragmaMSPragma("code_seg"));<br>
> + PP.AddPragmaHandler(MSCodeSeg.get());<br>
> + MSSection.reset(new PragmaMSPragma("section"));<br>
> + PP.AddPragmaHandler(MSSection.get());<br>
> }<br>
> }<br>
><br>
> @@ -214,6 +231,16 @@<br>
> MSPointersToMembers.reset();<br>
> PP.RemovePragmaHandler(MSVtorDisp.get());<br>
> MSVtorDisp.reset();<br>
> + PP.RemovePragmaHandler(MSDataSeg.get());<br>
> + MSDataSeg.reset();<br>
> + PP.RemovePragmaHandler(MSBSSSeg.get());<br>
> + MSBSSSeg.reset();<br>
> + PP.RemovePragmaHandler(MSConstSeg.get());<br>
> + MSConstSeg.reset();<br>
> + PP.RemovePragmaHandler(MSCodeSeg.get());<br>
> + MSCodeSeg.reset();<br>
> + PP.RemovePragmaHandler(MSSection.get());<br>
> + MSSection.reset();<br>
> }<br>
><br>
> PP.RemovePragmaHandler("STDC", FPContractHandler.get());<br>
> @@ -400,6 +427,120 @@<br>
> Actions.ActOnPragmaMSVtorDisp(Kind, PragmaLoc, Mode);<br>
> }<br>
><br>
> +void Parser::HandlePragmaMSPragma() {<br>
> + assert(Tok.is(tok::annot_pragma_ms_pragma));<br>
> + // Grab the tokens out of the annotation and enter them into the stream.<br>
> + auto TheTokens = (std::pair<Token*, size_t> *)Tok.getAnnotationValue();<br>
> + PP.EnterTokenStream(TheTokens->first, TheTokens->second, true, true);<br>
> + SourceLocation PragmaLocation = ConsumeToken(); // The annotation token.<br>
> + assert(Tok.isAnyIdentifier());<br>
> + llvm::StringRef PragmaName = Tok.getIdentifierInfo()->getName();<br>
> + PP.Lex(Tok); // pragma kind<br>
> + // Figure out which #pragma we're dealing with. The switch has no default<br>
> + // because lex shouldn't emit the annotation token for unrecognized pragmas.<br>
> + typedef unsigned (Parser::*PragmaHandler)(llvm::StringRef, SourceLocation);<br>
> + PragmaHandler Handler = llvm::StringSwitch<PragmaHandler>(PragmaName)<br>
> + .Case("data_seg", &Parser::HandlePragmaMSXXXXSeg)<br>
> + .Case("bss_seg", &Parser::HandlePragmaMSXXXXSeg)<br>
> + .Case("const_seg", &Parser::HandlePragmaMSXXXXSeg)<br>
> + .Case("code_seg", &Parser::HandlePragmaMSXXXXSeg)<br>
> + .Case("section", &Parser::HandlePragmaMSSection);<br>
> + if (auto DiagID = (this->*Handler)(PragmaName, PragmaLocation)) {<br>
> + PP.Diag(PragmaLocation, DiagID) << PragmaName;<br>
> + SkipUntil(tok::eof);<br>
> + PP.Lex(Tok); // tok::eof<br>
> + }<br>
> +}<br>
> +<br>
> +unsigned Parser::HandlePragmaMSSection(llvm::StringRef PragmaName,<br>
> + SourceLocation PragmaLocation) {<br>
<br>
This could use Parser tests.<br>
<br>
> + BalancedDelimiterTracker Parens(*this, tok::l_paren, tok::eof);<br>
> + if (Parens.consumeOpen())<br>
> + return diag::warn_pragma_expected_lparen;<br>
> + // Parsing code for pragma section<br>
> + if (Tok.isNot(tok::string_literal))<br>
<br>
Should you be calling Parser::isTokenStringLiteral() here instead? If<br>
not, there should be a test that shows wide string literals fail to<br>
parse.<br>
<br>
> + return diag::warn_pragma_expected_string;<br>
> + StringLiteral *SegmentName =<br>
> + cast<StringLiteral>(ParseStringLiteralExpression().get());<br>
> + int SectionFlags = 0;<br>
> + while (Tok.is(tok::comma)) {<br>
> + PP.Lex(Tok); // ,<br>
> + if (!Tok.isAnyIdentifier())<br>
> + return diag::warn_pragma_invalid_action;<br>
> + Sema::PragmaSectionFlag Flag =<br>
> + llvm::StringSwitch<Sema::PragmaSectionFlag>(<br>
> + Tok.getIdentifierInfo()->getName())<br>
> + .Case("read", Sema::PSF_Read)<br>
> + .Case("write", Sema::PSF_Write)<br>
> + .Case("execute", Sema::PSF_Execute)<br>
<br>
What about shared, nopage, nocache, discard and remove? I know they're<br>
covered by returning PSF_None, but I would<br>
> + BalancedDelimiterTracker Parens(*this, tok::l_paren, tok::eof);<br>
> + if (Parens.consumeOpen())<br>
> + return diag::warn_pragma_expected_lparen;<br>
> + Sema::PragmaMsStack> </blockquote>