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

[Aaron Ballman]

LGTM, but you should wait for Richard to weigh in as well.

Thank you!

~Aaron

On Wed, Jul 8, 2015 at 9:36 AM, Andrey Bokhanko
<andreybokhanko at gmail.com> wrote:
> 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