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

[Aaron Ballman]

On Wed, Jul 8, 2015 at 5:50 PM, Andrey Bokhanko
<andreybokhanko at gmail.com> wrote:
> Aaron, thank you for investing your time for reviews! -- your comments
> give me a good run for my money, but patches definitely become better
> in result. :-)

My pleasure! :-)

~Aaron

>
> Yours,
> Andrey
>
>
> On Wed, Jul 8, 2015 at 9:48 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
>> 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