Author: gbiv Date: Mon May 9 20:59:34 2016 New Revision: 269005 URL: http://llvm.org/viewvc/llvm-project?rev=269005&view=rev Log: [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. Modified: cfe/trunk/lib/Sema/SemaOverload.cpp cfe/trunk/test/CodeGen/enable_if.c Modified: cfe/trunk/lib/Sema/SemaOverload.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=269005&r1=269004&r2=269005&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaOverload.cpp (original) +++ cfe/trunk/lib/Sema/SemaOverload.cpp Mon May 9 20:59:34 2016 @@ -8503,16 +8503,31 @@ Sema::AddArgumentDependentLookupCandidat } } -// 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<EnableIfAttr>(); + bool Cand2Attr = Cand2->hasAttr<EnableIfAttr>(); + 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<EnableIfAttr> but going in declaration order, @@ -8520,10 +8535,10 @@ static bool hasBetterEnableIfAttrs(const 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 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(Se } // Check for enable_if value-based overload resolution. - if (Cand1.Function && Cand2.Function && - (Cand1.Function->hasAttr<EnableIfAttr>() || - Cand2.Function->hasAttr<EnableIfAttr>())) - 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<FunctionDecl>(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, Modified: cfe/trunk/test/CodeGen/enable_if.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/enable_if.c?rev=269005&r1=269004&r2=269005&view=diff ============================================================================== --- cfe/trunk/test/CodeGen/enable_if.c (original) +++ cfe/trunk/test/CodeGen/enable_if.c Mon May 9 20:59:34 2016 @@ -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); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits