On Fri, Nov 16, 2012 at 11:52 AM, Jordan Rose <[email protected]> wrote: > We had a similar problem with the analyzer, and you can get part of the way > by overloading on & and &&. The problem with cast is that this is not valid: > > Foo &F = cast<Foo>(getTemporary()); > > but this is > > use(cast<Foo>(getTemporary()))
I'm just wondering how much of this ^ we really have. If we don't have much it might be sufficient. I'm still back at being confused about why we're casting temporaries though... but I guess that's just More Magic we have in our cast machinery. Makes me sad. > > So…yeah. I don't think we have a good fix. > > Jordan > > > On Nov 16, 2012, at 11:01 , David Blaikie <[email protected]> wrote: > >> On Fri, Nov 16, 2012 at 10:45 AM, Matthieu Monrocq >> <[email protected]> wrote: >>> >>> >>> On Fri, Nov 16, 2012 at 2:58 AM, David Blaikie <[email protected]> wrote: >>>> >>>> Can we warn about this? >>>> >>> >>> Unfortunately no. I don't think so. It would require inspecting the body of >>> `cast`... >>> >>> Argyrios implemented a whole class of warnings around this theme a while ago >>> now, and as we drilled into it (and I believe you helped) we just decided >>> that looking through functions was too difficult. As such: `T const& foo(T >>> const& t) { return t; }` completely dulls out binding to temporary warnings >>> => the function itself is correct, but its use may not. >> >> I'm a little confused now, TypeSourceInfo::getTypeLoc returns a >> TypeLoc by /value/, doesn't it? So I'm not sure how casting that works >> - the dynamic type is the same as the static type, yet we're still >> able to cast to some derived type. (glancing at the doxygen though, >> haven't looked at the code in detail) >> >>> However it seems to be that the problem is caused by the implementation of >>> `cast` is wrong. >>> >>> template <class X, class Y> >>> inline typename cast_retty<X, Y>::ret_type cast(const Y &Val) { >>> assert(isa<X>(Val) && "cast<Ty>() argument of incompatible type!"); >>> return cast_convert_val<X, Y, >>> typename simplify_type<Y>::SimpleType>::doit(Val); >>> } >>> >>> We accept a `const&`, allowing bind to temporary, but then the return type >>> can be bound to X& which means the `const` qualifier was dropped on the >>> floor. This is pretty nasty. >> >> Agreed, that's rough. >> >>> The culprit is actually just above: >>> >>> template<class To, class FromTy> struct cast_convert_val<To,FromTy,FromTy> { >>> // This _is_ a simple type, just cast it. >>> static typename cast_retty<To, FromTy>::ret_type doit(const FromTy &Val) { >>> typename cast_retty<To, FromTy>::ret_type Res2 >>> = (typename cast_retty<To, FromTy>::ret_type)const_cast<FromTy&>(Val); >>> return Res2; >>> } >>> }; >>> >>> >>> However, even overloading on const-ness would not make bind-to-const issues >>> disappear. >> >> True, but it would pretty much fix this particular class of problems. >> The person was probably trying to declare a local & just got a ref in >> there by mistake. Less common that people declare locals const so the >> accident would be found pretty consistently. >> >>> The only thing I can thing of is: >>> => allow `cast` (and al) only on non-const reference >>> => allow on non-const and const pointers >>> >>> which is the only way to circumvent bind-to-temporary situations I know of. >>> >>> I am not sure it's worth it. >> >> Probably not. Vaguely I'm wondering if we could have some >> only-under-c++11 extra functionality to detect rvalue cases (do we >> ever have dynamic type xvalues? I don't think so) & do something >> different/break so we can catch them. >> >>> For detection I see only two possibilities: >>> => Interprocedural (*) static analysis: when analyzing the function, note >>> that its return may be an alias of the parameter; when analyzing the use, >>> note that the statement is only valid if the return is not an alias of the >>> temporary; cross-reference the two and warn about the incompatibility >>> => Dynamic instrumentation: I wonder if Asan could catch this (maybe it does >>> already). With the proper instrumentation the destructor of the temporary >>> could trigger poisoning of the memory so that the next use cause an >>> assertion. >>> >>> (*) Interprocedural in the general case, here it would not be. >>> >>> -- Matthieu >>> >>>> >>>> On Thu, Nov 15, 2012 at 5:14 PM, Matt Beaumont-Gay <[email protected]> >>>> wrote: >>>>> Author: matthewbg >>>>> Date: Thu Nov 15 19:14:52 2012 >>>>> New Revision: 168124 >>>>> >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=168124&view=rev >>>>> Log: >>>>> Fix PR14321, a crash when Clang is built with GCC 4.7 at -O1 or greater. >>>>> >>>>> GCC 4.7 reuses stack slots fairly aggressively, which exposes more >>>>> temporary >>>>> lifetime bugs. >>>>> >>>>> No new test, this was caught by the existing >>>>> CodeGenCXX/mangle-ms-templates.cpp. >>>>> >>>>> Modified: >>>>> cfe/trunk/lib/AST/MicrosoftMangle.cpp >>>>> >>>>> Modified: cfe/trunk/lib/AST/MicrosoftMangle.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/MicrosoftMangle.cpp?rev=168124&r1=168123&r2=168124&view=diff >>>>> >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/AST/MicrosoftMangle.cpp (original) >>>>> +++ cfe/trunk/lib/AST/MicrosoftMangle.cpp Thu Nov 15 19:14:52 2012 >>>>> @@ -373,7 +373,7 @@ >>>>> dyn_cast<ClassTemplateSpecializationDecl>(ND)) { >>>>> TypeSourceInfo *TSI = Spec->getTypeAsWritten(); >>>>> if (TSI) { >>>>> - TemplateSpecializationTypeLoc &TSTL = >>>>> + TemplateSpecializationTypeLoc TSTL = >>>>> cast<TemplateSpecializationTypeLoc>(TSI->getTypeLoc()); >>>>> TemplateArgumentListInfo LI(TSTL.getLAngleLoc(), >>>>> TSTL.getRAngleLoc()); >>>>> for (unsigned i = 0, e = TSTL.getNumArgs(); i != e; ++i) >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
