[PATCH] Disabling of "redefine_extname" pragma for C++ code

[Aaron Ballman]

On Tue, Jul 7, 2015 at 10:42 AM, Andrey Bokhanko
<andreybokhanko at gmail.com> wrote:
> Aaron, thank you for the review!
>
> I added a warning (for non applying pragma redefine_extname) and
> removed these nasty braces. :-)
>
> Please re-review: http://reviews.llvm.org/D10805.

> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -5561,15 +5561,12 @@
>    return true;
>  }
>
> -/// \brief Returns true if given declaration is TU-scoped and externally
> -/// visible.
> -static bool isDeclTUScopedExternallyVisible(const Decl *D) {
> +/// \brief Returns true if given declaration has external C language linkage.
> +static bool isDeclExternC(const Decl *D) {
>    if (auto *FD = dyn_cast<FunctionDecl>(D))
> -    return (FD->getDeclContext()->isTranslationUnit() || FD->isExternC()) &&
> -           FD->hasExternalFormalLinkage();
> +    return FD->isExternC();
>    else if (auto *VD = dyn_cast<VarDecl>(D))
> -    return (VD->getDeclContext()->isTranslationUnit() || VD->isExternC()) &&
> -           VD->hasExternalFormalLinkage();
> +    return VD->isExternC();
>
>    llvm_unreachable("Unknown type of decl!");
>  }
> @@ -5998,13 +5995,16 @@
>
>      NewVD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0),
>                                                  Context, Label, 0));
> -  } else if (!ExtnameUndeclaredIdentifiers.empty() &&
> -             isDeclTUScopedExternallyVisible(NewVD)) {
> +  } else if (!ExtnameUndeclaredIdentifiers.empty()) {
>      llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
>        ExtnameUndeclaredIdentifiers.find(NewVD->getIdentifier());
>      if (I != ExtnameUndeclaredIdentifiers.end()) {
> -      NewVD->addAttr(I->second);
> -      ExtnameUndeclaredIdentifiers.erase(I);
> +      if (isDeclExternC(NewVD)) {
> +        NewVD->addAttr(I->second);
> +        ExtnameUndeclaredIdentifiers.erase(I);
> +      } else
> +        Diag(NewVD->getLocation(), diag::warn_redefine_extname_not_applied)
> +            << 1 << NewVD->getName();

You can leave off the ->getName() part and just pass in the Decl *.
Also, can you change 1 into /*Variable*/1?

>      }
>    }
>
> @@ -7526,13 +7526,16 @@
>      StringLiteral *SE = cast<StringLiteral>(E);
>      NewFD->addAttr(::new (Context) AsmLabelAttr(SE->getStrTokenLoc(0), Context,
>                                                  SE->getString(), 0));
> -  } else if (!ExtnameUndeclaredIdentifiers.empty() &&
> -             isDeclTUScopedExternallyVisible(NewFD)) {
> +  } else if (!ExtnameUndeclaredIdentifiers.empty()) {
>      llvm::DenseMap<IdentifierInfo*,AsmLabelAttr*>::iterator I =
>        ExtnameUndeclaredIdentifiers.find(NewFD->getIdentifier());
>      if (I != ExtnameUndeclaredIdentifiers.end()) {
> -      NewFD->addAttr(I->second);
> -      ExtnameUndeclaredIdentifiers.erase(I);
> +      if (isDeclExternC(NewFD)) {
> +        NewFD->addAttr(I->second);
> +        ExtnameUndeclaredIdentifiers.erase(I);
> +      } else
> +        Diag(NewFD->getLocation(), diag::warn_redefine_extname_not_applied)
> +            << 0 << NewFD->getName();

Same suggestions here.

>      }
>    }
>
> @@ -14355,12 +14358,14 @@
>    // 1) declares a function or a variable
>    // 2) has external linkage
>    // already exists, add a label attribute to it.
> -  if (PrevDecl &&
> -      (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl)) &&
> -      PrevDecl->hasExternalFormalLinkage())
> -    PrevDecl->addAttr(Attr);
> +  if (PrevDecl && (isa<FunctionDecl>(PrevDecl) || isa<VarDecl>(PrevDecl))) {
> +    if (isDeclExternC(PrevDecl)) {
> +        PrevDecl->addAttr(Attr);
> +    } else

Elide the braces. :-)

> +      Diag(PrevDecl->getLocation(), diag::warn_redefine_extname_not_applied)
> +          << (isa<FunctionDecl>(PrevDecl) ? 0 : 1) << Name->getName();

And here. :-)

>    // Otherwise, add a label atttibute to ExtnameUndeclaredIdentifiers.
> -  else
> +  } else
>      (void)ExtnameUndeclaredIdentifiers.insert(std::make_pair(Name, Attr));
>  }
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -6333,6 +6333,10 @@
>    InGroup<CastQual>, DefaultIgnore;
>  def warn_cast_qual2 : Warning<"cast from %0 to %1 must have all intermediate "
>    "pointers const qualified to be safe">, InGroup<CastQual>, DefaultIgnore;
> +def warn_redefine_extname_not_applied : Warning<
> +  "pragma redefine_extname is applicable to external C declarations only; "
> +  "not applied to %select{function|variable}0 '%1'">,
> +  InGroup<Pragmas>;

Should be #pragma instead of pragma.

If you remove the getName() from above, you can remove the single
quotes around %1. The diagnostic emitter will automatically quote
things as appropriate then.

>  } // End of general sema category.
>
>  // inline asm.
> Index: test/CodeGen/redefine_extname.c
> ===================================================================
> --- test/CodeGen/redefine_extname.c
> +++ test/CodeGen/redefine_extname.c
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -w -emit-llvm %s -o - | FileCheck %s
> +// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -Wpragmas -emit-llvm %s -o - | FileCheck %s
>
>  #pragma redefine_extname fake real
>  #pragma redefine_extname name alias
> @@ -24,3 +24,9 @@
>  extern int foo() { return 1; }
>  // CHECK: define i32 @bar()
>
> +// Check that pragma redefine_extname applies to external declarations only.
> +#pragma redefine_extname foo_static bar_static
> +static int foo_static() { return 1; }
> +int baz() { return foo_static(); } // expected-warning {{pragma redefine_extname is applicable to external C declarations only; not applied to function 'foo_static'}}

>From reading this warning, I find the wording slightly confusing
because it's talking about the function foo_static on a function call
expression, but not on the function definition. I would have expected
the diagnostic to follow the declaration, not the expression?

> +// CHECK-NOT: call i32 @bar_static()
> +
> Index: test/CodeGenCXX/redefine_extname.cpp
> ===================================================================
> --- test/CodeGenCXX/redefine_extname.cpp
> +++ test/CodeGenCXX/redefine_extname.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -w -emit-llvm %s -o - | FileCheck %s
> +// RUN: %clang_cc1 -triple=i386-pc-solaris2.11 -verify -Wpragmas -emit-llvm %s -o - | FileCheck %s
>
>  extern "C" {
>    struct statvfs64 {
> @@ -20,11 +20,17 @@
>  // same name. PR23923.
>  #pragma redefine_extname foo bar
>  int f() {
> -  int foo = 0;
> +  int foo = 0; // expected-warning {{pragma redefine_extname is applicable to external C declarations only; not applied to variable 'foo'}}
>    return foo;
>  }
>  extern "C" {
>    int foo() { return 1; }
>  // CHECK: define i32 @bar()
>  }
>
> +// Check that pragma redefine_extname applies to C code only, and shouldn't be
> +// applied to C++.
> +#pragma redefine_extname foo_cpp bar_cpp
> +extern int foo_cpp() { return 1; } // expected-warning {{pragma redefine_extname is applicable to external C declarations only; not applied to function 'foo_cpp'}}
> +// CHECK-NOT: define i32 @bar_cpp()
> +
>

~Aaron