[cfe-commits] [PATCH] Implement '#pragma GCC visibility' (WIP)

[Douglas Gregor]

On Jul 17, 2010, at 9:09 PM, Eli Friedman wrote:

> Attached.  Review comments welcome.  This doesn't interact quite
> correctly with classes and namespaces; any guidance on how to
> implement the correct rules there would be appreciated.

Thanks for working on this! A few comments:

Index: lib/Sema/SemaDeclAttr.cpp
===================================================================
--- lib/Sema/SemaDeclAttr.cpp	(revision 108632)
+++ lib/Sema/SemaDeclAttr.cpp	(working copy)
@@ -678,7 +678,7 @@
   else if (TypeStr == "protected")
     type = VisibilityAttr::ProtectedVisibility;
   else {
-    S.Diag(Attr.getLoc(), diag::warn_attribute_unknown_visibility) << TypeStr;
+    S.Diag(Attr.getLoc(), diag::warn_attribute_unknown_visibility) << Str;
     return;
   }
 
@@ -2172,6 +2172,9 @@
   // Finally, apply any attributes on the decl itself.
   if (const AttributeList *Attrs = PD.getAttributes())
     ProcessDeclAttributeList(S, D, Attrs);
+
+  // Tack on a visibility attribute from '#pragma GCC visibility', if necessary.
+  AddPushedVisibilityAttribute(D);
 }

Since this isn't supposed to apply to class members, function-local variables, instance members, etc.,, the AddPushedVisibilityAttribute call should only occur when the declaration has external linkage and !D->getDeclContext()->isRecord(). 

It would be interesting to put a "#pragma GCC visibility push(hidden)" at the top of some non-trivial source file and then look at which declarations get tagged with the visibility attribute;.

Index: lib/Sema/SemaAttr.cpp
===================================================================
--- lib/Sema/SemaAttr.cpp	(revision 108632)
+++ lib/Sema/SemaAttr.cpp	(working copy)
@@ -289,3 +289,62 @@
     VD->addAttr(::new (Context) UnusedAttr());
   }
 }
+
+typedef std::vector<VisibilityAttr::VisibilityTypes> VisStack;

llvm::SmallVector?

Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp	(revision 108632)
+++ lib/Sema/Sema.cpp	(working copy)
@@ -123,7 +123,7 @@
     LangOpts(pp.getLangOptions()), PP(pp), Context(ctxt), Consumer(consumer),
     Diags(PP.getDiagnostics()), SourceMgr(PP.getSourceManager()),
     ExternalSource(0), CodeCompleter(CodeCompleter), CurContext(0), 
-    PackContext(0), TopFunctionScope(0), ParsingDeclDepth(0),
+    PackContext(0), VisContext(0), TopFunctionScope(0), ParsingDeclDepth(0),
     IdResolver(pp.getLangOptions()), StdNamespace(0), StdBadAlloc(0),
     GlobalNewDeleteDeclared(false), 
     CompleteTranslationUnit(CompleteTranslationUnit),
@@ -145,6 +145,7 @@
 
 Sema::~Sema() {
   if (PackContext) FreePackedContext();
+  if (VisContext) FreeVisContext();
   delete TheTargetAttributesSema;
   while (!FunctionScopes.empty())
     PopFunctionOrBlockScope();

Should we warn about  an unpopped visibility push(blah) in Sema::ActOnEndOfTranslationUnit()?

Index: lib/Parse/ParsePragma.cpp
===================================================================
--- lib/Parse/ParsePragma.cpp	(revision 108632)
+++ lib/Parse/ParsePragma.cpp	(working copy)
@@ -18,6 +18,59 @@
 #include "clang/Parse/Parser.h"
 using namespace clang;
 
+
+// #pragma GCC visibility comes in two variants:
+//   'push' '(' [visibility] ')'
+//   'pop'
+void PragmaGCCVisibilityHandler::HandlePragma(Preprocessor &PP, Token &VisTok) {
+  SourceLocation VisLoc = VisTok.getLocation();
+
+  Token Tok;
+  PP.Lex(Tok);
+
+  const IdentifierInfo *PushPop = Tok.getIdentifierInfo();
+
+  bool IsPush;
+  const IdentifierInfo *VisType;
+  if (PushPop && PushPop->isStr("pop")) {
+    IsPush = false;
+    VisType = 0;
+  } else if (PushPop && PushPop->isStr("push")) {
+    IsPush = true;
+    PP.Lex(Tok);
+    if (Tok.isNot(tok::l_paren)) {
+      PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen)
+        << "visibility";
+      return;
+    }
+    PP.Lex(Tok);
+    VisType = Tok.getIdentifierInfo();
+    if (!VisType) {
+      PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_identifier)
+        << "visibility";
+      return;
+    }
+    PP.Lex(Tok);
+    if (Tok.isNot(tok::r_paren)) {
+      PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_rparen)
+        << "visibility";
+      return;
+    }
+  } else {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_identifier)
+      << "visibility";
+    return;
+  }

We should produce a more-helpful diagnostic in this case, e.g., "expected visibility 'push' or 'pop' ".

	- Doug