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

[David Blaikie]

On Fri, Nov 16, 2012 at 11:52 AM, Jordan Rose <jordan_rose at apple.com> 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 <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
>