[PATCH] Disabling of "redefine_extname" pragma for C++ code
Andrey Bokhanko
andreybokhanko at gmail.com
Wed Jul 8 14:50:21 PDT 2015
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. :-)
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
More information about the cfe-commits
mailing list