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

[Andrey Bokhanko]

Aaron, thank you! -- I fixed everything; please re-review:
http://reviews.llvm.org/D10805

Yours,
Andrey


On Tue, Jul 7, 2015 at 6:41 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
> 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