From 99582718215387e7231316f3e22de9e6709cc01c Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Tue, 10 May 2016 01:59:34 +0000 Subject: [PATCH] [Sema] Fix an overload resolution bug with enable_if. Currently, if clang::isBetterOverloadCandidate encounters an enable_if attribute on either candidate that it's inspecting, it will ignore all lower priority attributes (e.g. pass_object_size). This is problematic in cases like: ``` void foo(char *c) __attribute__((enable_if(1, ""))); void foo(char *c __attribute__((pass_object_size(0)))) __attribute__((enable_if(1, ""))); ``` ...Because we would ignore the pass_object_size attribute in the second `foo`, and consider any call to `foo` to be ambiguous. This patch makes overload resolution consult further tiebreakers (e.g. pass_object_size) if two candidates have equally good enable_if attributes. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@269005 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Sema/SemaOverload.cpp | 58 +++++++++++++++++++++++++-------------- test/CodeGen/enable_if.c | 13 +++++++++ 2 files changed, 50 insertions(+), 21 deletions(-) diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp index 4a6831d88d..36e79f7bb4 100644 --- a/lib/Sema/SemaOverload.cpp +++ b/lib/Sema/SemaOverload.cpp @@ -8503,16 +8503,31 @@ Sema::AddArgumentDependentLookupCandidates(DeclarationName Name, } } -// Determines whether Cand1 is "better" in terms of its enable_if attrs than -// Cand2 for overloading. This function assumes that all of the enable_if attrs -// on Cand1 and Cand2 have conditions that evaluate to true. -// -// Cand1's set of enable_if attributes are said to be "better" than Cand2's iff -// Cand1's first N enable_if attributes have precisely the same conditions as -// Cand2's first N enable_if attributes (where N = the number of enable_if -// attributes on Cand2), and Cand1 has more than N enable_if attributes. -static bool hasBetterEnableIfAttrs(const Sema &S, const FunctionDecl *Cand1, - const FunctionDecl *Cand2) { +namespace { +enum class Comparison { Equal, Better, Worse }; +} + +/// Compares the enable_if attributes of two FunctionDecls, for the purposes of +/// overload resolution. +/// +/// Cand1's set of enable_if attributes are said to be "better" than Cand2's iff +/// Cand1's first N enable_if attributes have precisely the same conditions as +/// Cand2's first N enable_if attributes (where N = the number of enable_if +/// attributes on Cand2), and Cand1 has more than N enable_if attributes. +/// +/// Note that you can have a pair of candidates such that Cand1's enable_if +/// attributes are worse than Cand2's, and Cand2's enable_if attributes are +/// worse than Cand1's. +static Comparison compareEnableIfAttrs(const Sema &S, const FunctionDecl *Cand1, + const FunctionDecl *Cand2) { + // Common case: One (or both) decls don't have enable_if attrs. + bool Cand1Attr = Cand1->hasAttr(); + bool Cand2Attr = Cand2->hasAttr(); + if (!Cand1Attr || !Cand2Attr) { + if (Cand1Attr == Cand2Attr) + return Comparison::Equal; + return Cand1Attr ? Comparison::Better : Comparison::Worse; + } // FIXME: The next several lines are just // specific_attr_iterator but going in declaration order, @@ -8520,10 +8535,10 @@ static bool hasBetterEnableIfAttrs(const Sema &S, const FunctionDecl *Cand1, auto Cand1Attrs = getOrderedEnableIfAttrs(Cand1); auto Cand2Attrs = getOrderedEnableIfAttrs(Cand2); - // Candidate 1 is better if it has strictly more attributes and - // the common sequence is identical. - if (Cand1Attrs.size() <= Cand2Attrs.size()) - return false; + // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1 + // has fewer enable_if attributes than Cand2. + if (Cand1Attrs.size() < Cand2Attrs.size()) + return Comparison::Worse; auto Cand1I = Cand1Attrs.begin(); llvm::FoldingSetNodeID Cand1ID, Cand2ID; @@ -8535,10 +8550,10 @@ static bool hasBetterEnableIfAttrs(const Sema &S, const FunctionDecl *Cand1, Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true); Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true); if (Cand1ID != Cand2ID) - return false; + return Comparison::Worse; } - return true; + return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better; } /// isBetterOverloadCandidate - Determines whether the first overload @@ -8649,10 +8664,11 @@ bool clang::isBetterOverloadCandidate(Sema &S, const OverloadCandidate &Cand1, } // Check for enable_if value-based overload resolution. - if (Cand1.Function && Cand2.Function && - (Cand1.Function->hasAttr() || - Cand2.Function->hasAttr())) - return hasBetterEnableIfAttrs(S, Cand1.Function, Cand2.Function); + if (Cand1.Function && Cand2.Function) { + Comparison Cmp = compareEnableIfAttrs(S, Cand1.Function, Cand2.Function); + if (Cmp != Comparison::Equal) + return Cmp == Comparison::Better; + } if (S.getLangOpts().CUDA && Cand1.Function && Cand2.Function) { FunctionDecl *Caller = dyn_cast(S.CurContext); @@ -10331,7 +10347,7 @@ private: // disambiguate for us if there are multiple candidates and no exact match. return candidateHasExactlyCorrectType(A) && (!candidateHasExactlyCorrectType(B) || - hasBetterEnableIfAttrs(S, A, B)); + compareEnableIfAttrs(S, A, B) == Comparison::Better); } /// \return true if we were able to eliminate all but one overload candidate, diff --git a/test/CodeGen/enable_if.c b/test/CodeGen/enable_if.c index f863d80c14..5e9f904fdd 100644 --- a/test/CodeGen/enable_if.c +++ b/test/CodeGen/enable_if.c @@ -80,3 +80,16 @@ void test4() { // CHECK: store void (i32)* @_Z3quxUa9enable_ifIXLi1EEXL_Z9TRUEFACTSEEEi p = &qux; } + +// There was a bug where, when enable_if was present, overload resolution +// wouldn't pay attention to lower-priority attributes. +// (N.B. `foo` with pass_object_size should always be preferred) +// CHECK-LABEL: define void @test5 +void test5() { + int foo(char *i) __attribute__((enable_if(1, ""), overloadable)); + int foo(char *i __attribute__((pass_object_size(0)))) + __attribute__((enable_if(1, ""), overloadable)); + + // CHECK: call i32 @_Z3fooUa9enable_ifIXLi1EEEPcU17pass_object_size0 + foo((void*)0); +} -- 2.40.0