r249995 - [Sema] Allow C conversions in C overload logic
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 12 16:53:16 PST 2016
ISTM that there's no established way to tell Sema::Diag to not emit its
diagnostic, and trying to swap to e.g. Sema::CanPerformCopyInitialization
makes us too forgiving. So, given that we need to use
CheckSingleAssignmentConstraints directly, I don't see a better way to
handle this than plumbing the `Diagnose` boolean through.
I'll take a pass at fixing it, and hopefully get a patch out tomorrow.
On Tue, Jan 12, 2016 at 1:51 PM, George Burgess IV <
george.burgess.iv at gmail.com> wrote:
> Sorry for the delayed response; one of my filters silently marked this
> mail as read. Looking into it now. :)
>
> On Fri, Jan 8, 2016 at 12:38 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>> George,
>>
>> This change caused a serious regression for Objective-C method lookup.
>> See PR26085 (http://llvm.org/pr26085).
>>
>> For the test case in that PR, Sema::SelectBestMethod looks at the two
>> candidate "test" methods. It will match the second one, but in the process
>> of considering the first candidate, an error diagnostic is generated. This
>> happens within the call to CheckSingleAssignmentConstraints that was added
>> here in IsStandardConversion. The "Diagnose" argument in that call is set
>> to false, but the diagnostic is generated anyway. For the test case in the
>> PR, the diagnostic comes from CheckObjCARCConversion, but it looks like
>> there are some other diagnostics that could also be generated from within
>> CheckSingleAssignmentConstraints.
>>
>> I think I could manage a fix, e.g., by threading the “Diagnose” flag
>> through all the relevant code and consistently checking it before emitting
>> diagnostics, but I’m not especially familiar with this part of clang. If
>> you or someone else who knows more about this area can figure out the best
>> way to fix it, I would appreciate it.
>>
>> —Bob
>>
>> > On Oct 11, 2015, at 1:13 PM, George Burgess IV via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>> >
>> > Author: gbiv
>> > Date: Sun Oct 11 15:13:20 2015
>> > New Revision: 249995
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=249995&view=rev
>> > Log:
>> > [Sema] Allow C conversions in C overload logic
>> >
>> > C allows for some implicit conversions that C++ does not, e.g. void* ->
>> > char*. This patch teaches clang that these conversions are okay when
>> > dealing with overloads in C.
>> >
>> > Differential Revision: http://reviews.llvm.org/D13604
>> >
>> > Modified:
>> > cfe/trunk/include/clang/Sema/Overload.h
>> > cfe/trunk/include/clang/Sema/Sema.h
>> > cfe/trunk/lib/Sema/SemaExpr.cpp
>> > cfe/trunk/lib/Sema/SemaOverload.cpp
>> > cfe/trunk/test/Sema/overloadable.c
>> >
>> > Modified: cfe/trunk/include/clang/Sema/Overload.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=249995&r1=249994&r2=249995&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/include/clang/Sema/Overload.h (original)
>> > +++ cfe/trunk/include/clang/Sema/Overload.h Sun Oct 11 15:13:20 2015
>> > @@ -83,7 +83,8 @@ namespace clang {
>> > ICK_TransparentUnionConversion, ///< Transparent Union Conversions
>> > ICK_Writeback_Conversion, ///< Objective-C ARC writeback conversion
>> > ICK_Zero_Event_Conversion, ///< Zero constant to event (OpenCL1.2
>> 6.12.10)
>> > - ICK_Num_Conversion_Kinds ///< The number of conversion kinds
>> > + ICK_C_Only_Conversion, ///< Conversions allowed in C, but not
>> C++
>> > + ICK_Num_Conversion_Kinds, ///< The number of conversion kinds
>> > };
>> >
>> > /// ImplicitConversionRank - The rank of an implicit conversion
>> > @@ -95,7 +96,9 @@ namespace clang {
>> > ICR_Promotion, ///< Promotion
>> > ICR_Conversion, ///< Conversion
>> > ICR_Complex_Real_Conversion, ///< Complex <-> Real conversion
>> > - ICR_Writeback_Conversion ///< ObjC ARC writeback conversion
>> > + ICR_Writeback_Conversion, ///< ObjC ARC writeback conversion
>> > + ICR_C_Conversion ///< Conversion only allowed in the C
>> standard.
>> > + /// (e.g. void* to char*)
>> > };
>> >
>> > ImplicitConversionRank GetConversionRank(ImplicitConversionKind Kind);
>> >
>> > Modified: cfe/trunk/include/clang/Sema/Sema.h
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=249995&r1=249994&r2=249995&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/include/clang/Sema/Sema.h (original)
>> > +++ cfe/trunk/include/clang/Sema/Sema.h Sun Oct 11 15:13:20 2015
>> > @@ -8292,19 +8292,23 @@ public:
>> > QualType LHSType,
>> > QualType RHSType);
>> >
>> > - /// Check assignment constraints and prepare for a conversion of the
>> > - /// RHS to the LHS type.
>> > + /// Check assignment constraints and optionally prepare for a
>> conversion of
>> > + /// the RHS to the LHS type. The conversion is prepared for if
>> ConvertRHS
>> > + /// is true.
>> > AssignConvertType CheckAssignmentConstraints(QualType LHSType,
>> > ExprResult &RHS,
>> > - CastKind &Kind);
>> > + CastKind &Kind,
>> > + bool ConvertRHS = true);
>> >
>> > // CheckSingleAssignmentConstraints - Currently used by
>> > // CheckAssignmentOperands, and ActOnReturnStmt. Prior to type
>> checking,
>> > - // this routine performs the default function/array converions.
>> > + // this routine performs the default function/array converions, if
>> ConvertRHS
>> > + // is true.
>> > AssignConvertType CheckSingleAssignmentConstraints(QualType LHSType,
>> > ExprResult &RHS,
>> > bool Diagnose =
>> true,
>> > - bool
>> DiagnoseCFAudited = false);
>> > + bool
>> DiagnoseCFAudited = false,
>> > + bool ConvertRHS =
>> true);
>> >
>> > // \brief If the lhs type is a transparent union, check whether we
>> > // can initialize the transparent union with the given expression.
>> >
>> > Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=249995&r1=249994&r2=249995&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>> > +++ cfe/trunk/lib/Sema/SemaExpr.cpp Sun Oct 11 15:13:20 2015
>> > @@ -5376,13 +5376,13 @@ CastKind Sema::PrepareScalarCast(ExprRes
>> > return CK_IntegralToFloating;
>> > case Type::STK_IntegralComplex:
>> > Src = ImpCastExprToType(Src.get(),
>> > -
>> DestTy->castAs<ComplexType>()->getElementType(),
>> > - CK_IntegralCast);
>> > + DestTy->castAs<ComplexType>()->getElementType(),
>> > + CK_IntegralCast);
>> > return CK_IntegralRealToComplex;
>> > case Type::STK_FloatingComplex:
>> > Src = ImpCastExprToType(Src.get(),
>> > -
>> DestTy->castAs<ComplexType>()->getElementType(),
>> > - CK_IntegralToFloating);
>> > + DestTy->castAs<ComplexType>()->getElementType(),
>> > + CK_IntegralToFloating);
>> > return CK_FloatingRealToComplex;
>> > case Type::STK_MemberPointer:
>> > llvm_unreachable("member pointer type in C");
>> > @@ -6867,7 +6867,7 @@ Sema::CheckAssignmentConstraints(SourceL
>> > ExprResult RHSPtr = &RHSExpr;
>> > CastKind K = CK_Invalid;
>> >
>> > - return CheckAssignmentConstraints(LHSType, RHSPtr, K);
>> > + return CheckAssignmentConstraints(LHSType, RHSPtr, K,
>> /*ConvertRHS=*/false);
>> > }
>> >
>> > /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently
>> > @@ -6889,7 +6889,7 @@ Sema::CheckAssignmentConstraints(SourceL
>> > /// Sets 'Kind' for any result kind except Incompatible.
>> > Sema::AssignConvertType
>> > Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS,
>> > - CastKind &Kind) {
>> > + CastKind &Kind, bool ConvertRHS) {
>> > QualType RHSType = RHS.get()->getType();
>> > QualType OrigLHSType = LHSType;
>> >
>> > @@ -6911,7 +6911,7 @@ Sema::CheckAssignmentConstraints(QualTyp
>> > CheckAssignmentConstraints(AtomicTy->getValueType(), RHS, Kind);
>> > if (result != Compatible)
>> > return result;
>> > - if (Kind != CK_NoOp)
>> > + if (Kind != CK_NoOp && ConvertRHS)
>> > RHS = ImpCastExprToType(RHS.get(), AtomicTy->getValueType(),
>> Kind);
>> > Kind = CK_NonAtomicToAtomic;
>> > return Compatible;
>> > @@ -6941,7 +6941,7 @@ Sema::CheckAssignmentConstraints(QualTyp
>> > // CK_VectorSplat does T -> vector T, so first cast to the
>> > // element type.
>> > QualType elType = cast<ExtVectorType>(LHSType)->getElementType();
>> > - if (elType != RHSType) {
>> > + if (elType != RHSType && ConvertRHS) {
>> > Kind = PrepareScalarCast(RHS, elType);
>> > RHS = ImpCastExprToType(RHS.get(), elType, Kind);
>> > }
>> > @@ -6974,7 +6974,8 @@ Sema::CheckAssignmentConstraints(QualTyp
>> > // Arithmetic conversions.
>> > if (LHSType->isArithmeticType() && RHSType->isArithmeticType() &&
>> > !(getLangOpts().CPlusPlus && LHSType->isEnumeralType())) {
>> > - Kind = PrepareScalarCast(RHS, LHSType);
>> > + if (ConvertRHS)
>> > + Kind = PrepareScalarCast(RHS, LHSType);
>> > return Compatible;
>> > }
>> >
>> > @@ -7099,7 +7100,8 @@ Sema::CheckAssignmentConstraints(QualTyp
>> > // Only under strict condition T^ is compatible with an Objective-C
>> pointer.
>> > if (RHSType->isBlockPointerType() &&
>> > LHSType->isBlockCompatibleObjCPointerType(Context)) {
>> > - maybeExtendBlockObject(RHS);
>> > + if (ConvertRHS)
>> > + maybeExtendBlockObject(RHS);
>> > Kind = CK_BlockPointerToObjCPointerCast;
>> > return Compatible;
>> > }
>> > @@ -7225,9 +7227,16 @@ Sema::CheckTransparentUnionArgumentConst
>> > }
>> >
>> > Sema::AssignConvertType
>> > -Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult
>> &RHS,
>> > +Sema::CheckSingleAssignmentConstraints(QualType LHSType, ExprResult
>> &CallerRHS,
>> > bool Diagnose,
>> > - bool DiagnoseCFAudited) {
>> > + bool DiagnoseCFAudited,
>> > + bool ConvertRHS) {
>> > + // If ConvertRHS is false, we want to leave the caller's RHS
>> untouched. Sadly,
>> > + // we can't avoid *all* modifications at the moment, so we need some
>> somewhere
>> > + // to put the updated value.
>> > + ExprResult LocalRHS = CallerRHS;
>> > + ExprResult &RHS = ConvertRHS ? CallerRHS : LocalRHS;
>> > +
>> > if (getLangOpts().CPlusPlus) {
>> > if (!LHSType->isRecordType() && !LHSType->isAtomicType()) {
>> > // C++ 5.17p3: If the left operand is not of class type, the
>> > @@ -7276,7 +7285,8 @@ Sema::CheckSingleAssignmentConstraints(Q
>> > CastKind Kind;
>> > CXXCastPath Path;
>> > CheckPointerConversion(RHS.get(), LHSType, Kind, Path, false);
>> > - RHS = ImpCastExprToType(RHS.get(), LHSType, Kind, VK_RValue,
>> &Path);
>> > + if (ConvertRHS)
>> > + RHS = ImpCastExprToType(RHS.get(), LHSType, Kind, VK_RValue,
>> &Path);
>> > return Compatible;
>> > }
>> >
>> > @@ -7287,6 +7297,7 @@ Sema::CheckSingleAssignmentConstraints(Q
>> > //
>> > // Suppress this for references: C++ 8.5.3p5.
>> > if (!LHSType->isReferenceType()) {
>> > + // FIXME: We potentially allocate here even if ConvertRHS is false.
>> > RHS = DefaultFunctionArrayLvalueConversion(RHS.get());
>> > if (RHS.isInvalid())
>> > return Incompatible;
>> > @@ -7303,7 +7314,7 @@ Sema::CheckSingleAssignmentConstraints(Q
>> >
>> > CastKind Kind = CK_Invalid;
>> > Sema::AssignConvertType result =
>> > - CheckAssignmentConstraints(LHSType, RHS, Kind);
>> > + CheckAssignmentConstraints(LHSType, RHS, Kind, ConvertRHS);
>> >
>> > // C99 6.5.16.1p2: The value of the right operand is converted to the
>> > // type of the assignment expression.
>> > @@ -7325,7 +7336,8 @@ Sema::CheckSingleAssignmentConstraints(Q
>> > return Compatible;
>> > }
>> >
>> > - RHS = ImpCastExprToType(E, Ty, Kind);
>> > + if (ConvertRHS)
>> > + RHS = ImpCastExprToType(E, Ty, Kind);
>> > }
>> > return result;
>> > }
>> >
>> > Modified: cfe/trunk/lib/Sema/SemaOverload.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOverload.cpp?rev=249995&r1=249994&r2=249995&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/lib/Sema/SemaOverload.cpp (original)
>> > +++ cfe/trunk/lib/Sema/SemaOverload.cpp Sun Oct 11 15:13:20 2015
>> > @@ -130,7 +130,11 @@ ImplicitConversionRank clang::GetConvers
>> > ICR_Complex_Real_Conversion,
>> > ICR_Conversion,
>> > ICR_Conversion,
>> > - ICR_Writeback_Conversion
>> > + ICR_Writeback_Conversion,
>> > + ICR_Exact_Match, // NOTE(gbiv): This may not be completely right --
>> > + // it was omitted by the patch that added
>> > + // ICK_Zero_Event_Conversion
>> > + ICR_C_Conversion
>> > };
>> > return Rank[(int)Kind];
>> > }
>> > @@ -162,7 +166,9 @@ static const char* GetImplicitConversion
>> > "Complex-real conversion",
>> > "Block Pointer conversion",
>> > "Transparent Union Conversion",
>> > - "Writeback conversion"
>> > + "Writeback conversion",
>> > + "OpenCL Zero Event Conversion",
>> > + "C specific type conversion"
>> > };
>> > return Name[Kind];
>> > }
>> > @@ -1411,7 +1417,7 @@ static bool tryAtomicConversion(Sema &S,
>> > bool InOverloadResolution,
>> > StandardConversionSequence &SCS,
>> > bool CStyle);
>> > -
>> > +
>> > /// IsStandardConversion - Determines whether there is a standard
>> > /// conversion sequence (C++ [conv], C++ [over.ics.scs]) from the
>> > /// expression From to the type ToType. Standard conversion sequences
>> > @@ -1434,13 +1440,10 @@ static bool IsStandardConversion(Sema &S
>> > SCS.CopyConstructor = nullptr;
>> >
>> > // There are no standard conversions for class types in C++, so
>> > - // abort early. When overloading in C, however, we do permit
>> > - if (FromType->isRecordType() || ToType->isRecordType()) {
>> > - if (S.getLangOpts().CPlusPlus)
>> > - return false;
>> > -
>> > - // When we're overloading in C, we allow, as standard conversions,
>> > - }
>> > + // abort early. When overloading in C, however, we do permit them.
>> > + if (S.getLangOpts().CPlusPlus &&
>> > + (FromType->isRecordType() || ToType->isRecordType()))
>> > + return false;
>> >
>> > // The first conversion can be an lvalue-to-rvalue conversion,
>> > // array-to-pointer conversion, or function-to-pointer conversion
>> > @@ -1649,9 +1652,9 @@ static bool IsStandardConversion(Sema &S
>> > // tryAtomicConversion has updated the standard conversion sequence
>> > // appropriately.
>> > return true;
>> > - } else if (ToType->isEventT() &&
>> > + } else if (ToType->isEventT() &&
>> > From->isIntegerConstantExpr(S.getASTContext()) &&
>> > - (From->EvaluateKnownConstInt(S.getASTContext()) == 0)) {
>> > + From->EvaluateKnownConstInt(S.getASTContext()) == 0) {
>> > SCS.Second = ICK_Zero_Event_Conversion;
>> > FromType = ToType;
>> > } else {
>> > @@ -1690,11 +1693,28 @@ static bool IsStandardConversion(Sema &S
>> > }
>> > SCS.setToType(2, FromType);
>> >
>> > + if (CanonFrom == CanonTo)
>> > + return true;
>> > +
>> > // If we have not converted the argument type to the parameter type,
>> > - // this is a bad conversion sequence.
>> > - if (CanonFrom != CanonTo)
>> > + // this is a bad conversion sequence, unless we're resolving an
>> overload in C.
>> > + if (S.getLangOpts().CPlusPlus || !InOverloadResolution)
>> > return false;
>> >
>> > + ExprResult ER = ExprResult{From};
>> > + auto Conv = S.CheckSingleAssignmentConstraints(ToType, ER,
>> > + /*Diagnose=*/false,
>> > +
>> /*DiagnoseCFAudited=*/false,
>> > + /*ConvertRHS=*/false);
>> > + if (Conv != Sema::Compatible)
>> > + return false;
>> > +
>> > + SCS.setAllToTypes(ToType);
>> > + // We need to set all three because we want this conversion to rank
>> terribly,
>> > + // and we don't know what conversions it may overlap with.
>> > + SCS.First = ICK_C_Only_Conversion;
>> > + SCS.Second = ICK_C_Only_Conversion;
>> > + SCS.Third = ICK_C_Only_Conversion;
>> > return true;
>> > }
>> >
>> > @@ -4993,6 +5013,7 @@ static bool CheckConvertedConstantConver
>> > case ICK_TransparentUnionConversion:
>> > case ICK_Writeback_Conversion:
>> > case ICK_Zero_Event_Conversion:
>> > + case ICK_C_Only_Conversion:
>> > return false;
>> >
>> > case ICK_Lvalue_To_Rvalue:
>> > @@ -5785,7 +5806,7 @@ ObjCMethodDecl *Sema::SelectBestMethod(S
>> > Match = false;
>> > break;
>> > }
>> > -
>> > +
>> > ImplicitConversionSequence ConversionState
>> > = TryCopyInitialization(*this, argExpr, param->getType(),
>> > /*SuppressUserConversions*/false,
>> >
>> > Modified: cfe/trunk/test/Sema/overloadable.c
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/overloadable.c?rev=249995&r1=249994&r2=249995&view=diff
>> >
>> ==============================================================================
>> > --- cfe/trunk/test/Sema/overloadable.c (original)
>> > +++ cfe/trunk/test/Sema/overloadable.c Sun Oct 11 15:13:20 2015
>> > @@ -85,3 +85,17 @@ void local() {
>> > void after_local_1(int) __attribute__((overloadable)); //
>> expected-error {{conflicting types}}
>> > void after_local_2(int); // expected-error {{must have the
>> 'overloadable' attribute}}
>> > void after_local_3(int) __attribute__((overloadable));
>> > +
>> > +// Make sure we allow C-specific conversions in C.
>> > +void conversions() {
>> > + void foo(char *c) __attribute__((overloadable));
>> > + void foo(char *c) __attribute__((overloadable, enable_if(c,
>> "nope.jpg")));
>> > +
>> > + void *ptr;
>> > + foo(ptr);
>> > +
>> > + void multi_type(unsigned char *c) __attribute__((overloadable));
>> > + void multi_type(signed char *c) __attribute__((overloadable));
>> > + unsigned char *c;
>> > + multi_type(c);
>> > +}
>> >
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160112/b07caa0b/attachment-0001.html>
More information about the cfe-commits
mailing list