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

[Eli Friedman]

On Sun, Jul 18, 2010 at 5:08 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> 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().

Ah, I guess that'll cover the issue's I've spotted with class members.

> 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?

Sure.

> 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()?

I don't think so... gcc doesn't warn, and Firefox uses a pattern that
would warn if we checked this.

> 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' ".

OK.

-Eli