[PATCH] PR23923: "redefine_extname" pragma handled incorrectly in case of a local var with the same name

[Aaron Ballman]

On Wed, Jun 24, 2015 at 6:41 AM, Andrey Bokhanko
<andreybokhanko at gmail.com> wrote:
> Patch updated in response to Aaron Ballman's comments (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/131538.html).

Getting there! A few more nits and a question below.

>
>
> http://reviews.llvm.org/D10646
>
> Files:
>   lib/Sema/SemaDecl.cpp
>   test/CodeGen/redefine_extname.c
>
> Index: test/CodeGen/redefine_extname.c
> ===================================================================
> --- test/CodeGen/redefine_extname.c
> +++ test/CodeGen/redefine_extname.c
> @@ -13,3 +13,14 @@
>  // CHECK:   call i32 @real()
>  // Check that this also works with variables names
>  // CHECK:   load i32, i32* @alias
> +
> +// This is a case when redefenition is deferred *and* we have a local of the
> +// same name. PR23923.
> +#pragma redefine_extname foo bar
> +int f() {
> +  int foo = 0;
> +  return foo;
> +}
> +extern int foo() { return 1; }
> +// CHECK: define i32 @bar()

Should this also work with code like:

#pragma redefine_extname foo bar
int f() {
  int foo = 0;
  return foo;
}

extern "C" {
  int foo() { return 1; }
}

If so, I'd like to see a test case for that.

> +
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -5522,6 +5522,18 @@
>    return true;
>  }
>
> +/// \brief Returns true if given declaration is TU-scoped and externally visible

Missing a period at the end of the comment.

> +static bool isDeclTUScopedExternallyVisible(Decl *Decl) {

The Decl passed in can be const. Also, please name the identifier
something other than the type. D would suffice.

> +  if (auto *FD = dyn_cast<FunctionDecl>(Decl))
> +    return (FD->getDeclContext()->isTranslationUnit() || FD->isExternC()) &&
> +           FD->hasExternalFormalLinkage();
> +  else if (auto *VD = dyn_cast<VarDecl>(Decl))
> +    return (VD->getDeclContext()->isTranslationUnit() || VD->isExternC()) &&
> +           VD->hasExternalFormalLinkage();
> +  else
> +    llvm_unreachable("Unknown type of decl!");

I would drop the else and put the llvm_unreachable as the last
statement in the function.

> +}
> +
>  NamedDecl *
>  Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
>                                TypeSourceInfo *TInfo, LookupResult &Previous,
> @@ -5946,7 +5958,8 @@
>
>      NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0),
>                                                  Context, Label, 0));
> -  } else if (!ExtnameUndeclaredIdentifiers.empty()) {
> +  } else if (!ExtnameUndeclaredIdentifiers.empty() &&
> +             isDeclTUScopedExternallyVisible(NewVD)) {
>      llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
>        ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier());
>      if (I != ExtnameUndeclaredIdentifiers.end()) {
> @@ -7468,7 +7481,8 @@
>      StringLiteral *SE = cast<StringLiteral>(E);
>      NewFD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), Context,
>                                                  SE->getString(), 0));
> -  } else if (!ExtnameUndeclaredIdentifiers.empty()) {
> +  } else if (!ExtnameUndeclaredIdentifiers.empty() &&
> +             isDeclTUScopedExternallyVisible(NewFD)) {
>      llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
>        ExtnameUndeclaredIdentifiers.find(NewFD->getIdentifier());
>      if (I != ExtnameUndeclaredIdentifiers.end()) {
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/

~Aaron