[patch][pr17535] Reject alias attributes that point to undefined names.
Aaron Ballman
aaron at aaronballman.com
Sun Oct 20 07:56:42 PDT 2013
Some minor comments below:
> diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
> index 381eaeb..37d8a4d 100644
> --- a/include/clang/Basic/Attr.td
> +++ b/include/clang/Basic/Attr.td
> @@ -176,6 +176,7 @@ def AddressSpace : TypeAttr {
> def Alias : InheritableAttr {
> let Spellings = [GNU<"alias">, CXX11<"gnu", "alias">];
> let Args = [StringArgument<"Aliasee">];
> + let AdditionalMembers = [{ NamedDecl *AliaseeDecl; }];
> }
>
> def Aligned : InheritableAttr {
> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
> index d8fef68..3250e4f 100644
> --- a/include/clang/Basic/DiagnosticSemaKinds.td
> +++ b/include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2015,6 +2015,8 @@ def err_attribute_weakref_without_alias : Error<
> "weakref declaration of '%0' must also have an alias attribute">;
> def err_alias_not_supported_on_darwin : Error <
> "only weak aliases are supported on darwin">;
> +def err_alias_to_undefined : Error<
> + "alias must point to a defined variable or function">;
I would prefer to see what the aliasee's name is as part of the
diagnostic. Also, I think we're trending towards having the
attribute's name come from the attribute itself, and be quoted.
> def warn_attribute_wrong_decl_type : Warning<
> "%0 attribute only applies to %select{functions|unions|"
> "variables and functions|functions and methods|parameters|"
> diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
> index 65f07a8..c161f53 100644
> --- a/include/clang/Sema/Sema.h
> +++ b/include/clang/Sema/Sema.h
> @@ -372,6 +372,8 @@ public:
> /// cycle detection at the end of the TU.
> DelegatingCtorDeclsType DelegatingCtorDecls;
>
> + SmallVector<Decl*, 2> Aliases;
> +
> /// \brief All the overriding destructors seen during a class definition
> /// (there could be multiple due to nested classes) that had their exception
> /// spec checks delayed, plus the overridden destructor.
> @@ -954,6 +956,8 @@ public:
>
> void ActOnEndOfTranslationUnit();
>
> + void CheckAliases();
> +
> void CheckDelegatingCtorCycles();
>
> Scope *getScopeForContext(DeclContext *Ctx);
> diff --git a/lib/CodeGen/CGExprCXX.cpp b/lib/CodeGen/CGExprCXX.cpp
> index 2844fe2..6d146d6 100644
> --- a/lib/CodeGen/CGExprCXX.cpp
> +++ b/lib/CodeGen/CGExprCXX.cpp
> @@ -928,9 +928,8 @@ static RValue EmitNewDeleteCall(CodeGenFunction &CGF,
> /// to a replaceable global allocation function.
> ///
> /// We model such elidable calls with the 'builtin' attribute.
> - llvm::Function *Fn = dyn_cast<llvm::Function>(CalleeAddr);
> if (Callee->isReplaceableGlobalAllocationFunction() &&
> - Fn && Fn->hasFnAttribute(llvm::Attribute::NoBuiltin)) {
> + !Callee->hasAttr<AliasAttr>()) {
> // FIXME: Add addAttribute to CallSite.
> if (llvm::CallInst *CI = dyn_cast<llvm::CallInst>(CallOrInvoke))
> CI->addAttribute(llvm::AttributeSet::FunctionIndex,
> diff --git a/lib/CodeGen/CodeGenModule.cpp b/lib/CodeGen/CodeGenModule.cpp
> index 96ae437..1ce6285 100644
> --- a/lib/CodeGen/CodeGenModule.cpp
> +++ b/lib/CodeGen/CodeGenModule.cpp
> @@ -933,6 +933,12 @@ void CodeGenModule::EmitDeferred() {
> GlobalDecl D = DeferredDeclsToEmit.back();
> DeferredDeclsToEmit.pop_back();
>
> + const ValueDecl *Global = cast<ValueDecl>(D.getDecl());
> + if (Global->hasAttr<AliasAttr>()) {
> + EmitAliasDefinition(D);
> + continue;
> + }
> +
> // Check to see if we've already emitted this. This is necessary
> // for a couple of reasons: first, decls can end up in the
> // deferred-decls queue multiple times, and second, decls can end
> @@ -1098,7 +1104,7 @@ void CodeGenModule::EmitGlobal(GlobalDecl GD) {
> // If this is an alias definition (which otherwise looks like a declaration)
> // emit it now.
> if (Global->hasAttr<AliasAttr>())
> - return EmitAliasDefinition(GD);
> + DeferredDeclsToEmit.push_back(GD);
>
> // If this is CUDA, be selective about which declarations we emit.
> if (LangOpts.CUDA) {
> @@ -2093,12 +2099,14 @@ void CodeGenModule::EmitAliasDefinition(GlobalDecl GD) {
> // Create a reference to the named value. This ensures that it is emitted
> // if a deferred decl.
> llvm::Constant *Aliasee;
> - if (isa<llvm::FunctionType>(DeclTy))
> - Aliasee = GetOrCreateLLVMFunction(AA->getAliasee(), DeclTy, GD,
> - /*ForVTable=*/false);
> - else
> - Aliasee = GetOrCreateLLVMGlobal(AA->getAliasee(),
> - llvm::PointerType::getUnqual(DeclTy), 0);
> + if (FunctionDecl *F = dyn_cast<FunctionDecl>(AA->AliaseeDecl)) {
> + GlobalDecl GDA(F);
> + Aliasee = GetAddrOfFunction(GDA, DeclTy, /*ForVTable=*/false);
> + }
> + else {
> + VarDecl *Var = cast<VarDecl>(AA->AliaseeDecl);
> + Aliasee = GetAddrOfGlobalVar(Var,DeclTy);
> + }
>
> // Create the new alias itself, but don't set a name yet.
> llvm::GlobalValue *GA =
> diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp
> index 41f72a6..96f0a3e 100644
> --- a/lib/Sema/Sema.cpp
> +++ b/lib/Sema/Sema.cpp
> @@ -557,6 +557,36 @@ static bool IsRecordFullyDefined(const CXXRecordDecl *RD,
> return Complete;
> }
>
> +static bool isValidAliasee(NamedDecl *ND) {
> + if (!ND)
> + return false;
> + FunctionDecl *FD = dyn_cast<FunctionDecl>(ND);
> + if (FD)
> + return FD->isDefined();
> + VarDecl *VD = dyn_cast<VarDecl>(ND);
> + if (!VD)
> + return false;
> + return VD->hasDefinition() != VarDecl::DeclarationOnly;
> +}
> +
> +void Sema::CheckAliases() {
> + for (SmallVectorImpl<Decl*>::iterator I = Aliases.begin(), E = Aliases.end();
> + I != E; ++I) {
> + Decl *D = *I;
> + if (D->hasAttr<WeakRefAttr>())
> + continue;
> + AliasAttr *AA = D->getAttr<AliasAttr>();
Can a Decl have multiple AliasAttrs? If so, this becomes problematic.
If it's not allowed, we should have a diagnostic and test for it.
> + StringRef Name = AA->getAliasee();
> + const IdentifierInfo *NameI = &Context.Idents.get(Name);
> + SourceLocation NameLoc = AA->getLocation();
> + NamedDecl *Aliasee =
> + LookupSingleName(TUScope, NameI, NameLoc, LookupOrdinaryName);
> + if (!isValidAliasee(Aliasee))
> + Diag(NameLoc, diag::err_alias_to_undefined);
> + AA->AliaseeDecl = Aliasee;
> + }
> +}
> +
> /// ActOnEndOfTranslationUnit - This is called at the very end of the
> /// translation unit when EOF is reached and all but the top-level scope is
> /// popped.
> @@ -638,6 +668,8 @@ void Sema::ActOnEndOfTranslationUnit() {
> << I->first;
> }
>
> + CheckAliases();
> +
> if (LangOpts.CPlusPlus11 &&
> Diags.getDiagnosticLevel(diag::warn_delegating_ctor_cycle,
> SourceLocation())
> diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
> index d7c4f78..317bde4 100644
> --- a/lib/Sema/SemaDecl.cpp
> +++ b/lib/Sema/SemaDecl.cpp
> @@ -1977,7 +1977,10 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D, InheritableAttr *Attr,
> else if (SectionAttr *SA = dyn_cast<SectionAttr>(Attr))
> NewAttr = S.mergeSectionAttr(D, SA->getRange(), SA->getName(),
> AttrSpellingListIndex);
> - else if (isa<AlignedAttr>(Attr))
> + else if (isa<AliasAttr>(Attr)) {
> + NewAttr = cast<InheritableAttr>(Attr->clone(S.Context));
> + S.Aliases.push_back(D);
> + } else if (isa<AlignedAttr>(Attr))
> // AlignedAttrs are handled separately, because we need to handle all
> // such attributes on a declaration at the same time.
> NewAttr = 0;
> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
> index 3eabf87..a0bcace 100644
> --- a/lib/Sema/SemaDeclAttr.cpp
> +++ b/lib/Sema/SemaDeclAttr.cpp
> @@ -1681,10 +1681,10 @@ static void handleAliasAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> return;
> }
>
> - // FIXME: check if target symbol exists in current file
>
> D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,
> Attr.getAttributeSpellingListIndex()));
> + S.Aliases.push_back(D);
> }
>
> static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
> @@ -5015,10 +5015,17 @@ void Sema::DeclApplyPragmaWeak(Scope *S, NamedDecl *ND, WeakInfo &W) {
> W.setUsed(true);
> if (W.getAlias()) { // clone decl, impersonate __attribute(weak,alias(...))
> IdentifierInfo *NDId = ND->getIdentifier();
> +
> + NamedDecl *Aliasee = LookupSingleName(TUScope, W.getAlias(),
> + W.getLocation(), LookupOrdinaryName);
> + if (Aliasee && Aliasee->hasAttr<AliasAttr>())
> + return;
> +
> NamedDecl *NewD = DeclClonePragmaWeak(ND, W.getAlias(), W.getLocation());
> NewD->addAttr(::new (Context) AliasAttr(W.getLocation(), Context,
> NDId->getName()));
> NewD->addAttr(::new (Context) WeakAttr(W.getLocation(), Context));
> + Aliases.push_back(NewD);
> WeakTopLevelDecl.push_back(NewD);
> // FIXME: "hideous" code from Sema::LazilyCreateBuiltin
> // to insert Decl at TU scope, sorry.
> diff --git a/test/CodeGen/alias.c b/test/CodeGen/alias.c
> index efa94b3..31ba9a9 100644
> --- a/test/CodeGen/alias.c
> +++ b/test/CodeGen/alias.c
> @@ -6,6 +6,10 @@ int g0;
> static int bar1 = 42;
> // CHECKBASIC: @bar1 = internal global i32 42
>
> +static int bar2() {
> + return 0;
> +}
> +
> extern int g1;
> extern int g1 __attribute((alias("g0")));
> // CHECKBASIC-DAG: @g1 = alias i32* @g0
> @@ -34,11 +38,11 @@ static int inner_weak(int a) { return 0; }
> extern __typeof(inner) inner_a __attribute__((alias("inner")));
> static __typeof(inner_weak) inner_weak_a __attribute__((weakref, alias("inner_weak")));
> // CHECKCC: @inner_a = alias i32 (i32)* @inner
> -// CHECKCC: define internal arm_aapcs_vfpcc i32 @inner(i32 %a) [[NUW:#[0-9]+]] {
Why is this being moved lower and losing the [0-9]+ check?
>
> int outer(int a) { return inner(a); }
> -// CHECKCC: define arm_aapcs_vfpcc i32 @outer(i32 %a) [[NUW]] {
> +// CHECKCC: define arm_aapcs_vfpcc i32 @outer(i32 %a) [[NUW:#[0-9]+]] {
> // CHECKCC: call arm_aapcs_vfpcc i32 @inner(i32 %{{.*}})
> +// CHECKCC: define internal arm_aapcs_vfpcc i32 @inner(i32 %a) [[NUW]] {
>
> int outer_weak(int a) { return inner_weak_a(a); }
> // CHECKCC: define arm_aapcs_vfpcc i32 @outer_weak(i32 %a) [[NUW]] {
> diff --git a/test/CodeGenCXX/attr.cpp b/test/CodeGenCXX/attr.cpp
> index 8bcff36..4a78054 100644
> --- a/test/CodeGenCXX/attr.cpp
> +++ b/test/CodeGenCXX/attr.cpp
> @@ -29,6 +29,6 @@ void C::bar4() { }
> // CHECK-LABEL: define i32 @_Z5test1v()
> int test1() { return 10; }
> // CHECK at top of file
> -extern "C" int test2() __attribute__((alias("_Z5test1v")));
> +extern "C" int test2() __attribute__((alias("test1")));
So are we removing the functionality allowing to look up mangled names?
>
> // CHECK: attributes [[NUW]] = { nounwind{{.*}} }
> diff --git a/test/Misc/ast-dump-attr.cpp b/test/Misc/ast-dump-attr.cpp
> index 3efcd09..2ea1f9e 100644
> --- a/test/Misc/ast-dump-attr.cpp
> +++ b/test/Misc/ast-dump-attr.cpp
> @@ -79,6 +79,7 @@ void TestInt(void) __attribute__((constructor(123)));
> // CHECK: FunctionDecl{{.*}}TestInt
> // CHECK-NEXT: ConstructorAttr{{.*}} 123
>
> +int alias1;
> int TestString __attribute__((alias("alias1")));
> // CHECK: VarDecl{{.*}}TestString
> // CHECK-NEXT: AliasAttr{{.*}} "alias1"
> diff --git a/test/Sema/attr-alias-elf.c b/test/Sema/attr-alias-elf.c
> new file mode 100644
> index 0000000..55cae9b
> --- /dev/null
> +++ b/test/Sema/attr-alias-elf.c
> @@ -0,0 +1,22 @@
> +// RUN: %clang_cc1 -triple x86_64-pc-linux -fsyntax-only -verify %s
> +
> +void f1(void) __attribute__((alias("g1")));
> +void g1(void) {
> +}
> +
> +void f2(void) __attribute__((alias("g2"))); // expected-error {{alias must point to a defined variable or function}}
> +
> +
> +void f3(void) __attribute__((alias("g3"))); // expected-error {{alias must point to a defined variable or function}}
> +void g3(void);
> +
> +extern int a1 __attribute__((alias("b1")));
> +int b1 = 42;
> +
> +extern int a2 __attribute__((alias("b2"))); // expected-error {{alias must point to a defined variable or function}}
> +
> +extern int a3 __attribute__((alias("b3"))); // expected-error {{alias must point to a defined variable or function}}
> +extern int b3;
> +
> +extern int a4 __attribute__((alias("b4"))); // expected-error {{alias must point to a defined variable or function}}
Ugh... I wish there was a way we could use the existing mechanisms for
dealing with this. Namely, using a resolved identifier instead of a
string literal. This way, the error reporting can be more
consistent...
> +typedef int b4;
> diff --git a/test/SemaCXX/cxx11-gnu-attrs.cpp b/test/SemaCXX/cxx11-gnu-attrs.cpp
> index def83a9..f587805 100644
> --- a/test/SemaCXX/cxx11-gnu-attrs.cpp
> +++ b/test/SemaCXX/cxx11-gnu-attrs.cpp
> @@ -12,7 +12,7 @@ int *[[gnu::unused]] attr_on_ptr;
> // Valid cases.
>
> void alias1() {}
> -void alias2 [[gnu::alias("_Z6alias1v")]] ();
> +void alias2 [[gnu::alias("alias1")]] ();
Same here as above.
>
> [[gnu::aligned(8)]] int aligned;
> void aligned_fn [[gnu::aligned(32)]] ();
>
~Aaron
On Sat, Oct 19, 2013 at 6:54 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> ping
>
>> On 14 October 2013 12:31, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>> The attached patch causes clang to reject alias attributes that point
>>> to undefined names. For example, with this patch we now reject
>>>
>>> void f1(void) __attribute__((alias("g1")));
>>>
>>> if g1 is not defined in this file. This matches "modern" gcc (>= 4.2
>>> at least) behavior. What is different in this implementation is that
>>> we use the C/C++ level names, not the assembly names like gcc does.
>>> This is consistent with our implementation of #pragma weak, with has
>>> the same difference to gcc's implementation.
>>>
>>> Other options include
>>> * Using the mangled name from sema. This would be very expensive and
>>> we would have to mangle each decl to see if we got it.
>>> * Produce a terse error from CodeGen. We normally avoid this, but it
>>> is probably the right thing to do here if we decide to use the mangled
>>> names.
>
> Cheers,
> Rafael
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list