[cfe-commits] r168124 - /cfe/trunk/lib/AST/MicrosoftMangle.cpp

[Jordan Rose]

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()))

So…yeah. I don't think we have a good fix.

Jordan


On Nov 16, 2012, at 11:01 , David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Nov 16, 2012 at 10:45 AM, Matthieu Monrocq
> <matthieu.monrocq at gmail.com> wrote:
>> 
>> 
>> On Fri, Nov 16, 2012 at 2:58 AM, David Blaikie <dblaikie at gmail.com> 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 <matthewbg at google.com>
>>> 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
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
>> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits