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

[David Blaikie]

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