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

David Blaikie dblaikie at gmail.com
Sat Nov 17 12:01:26 PST 2012


On Sat, Nov 17, 2012 at 6:20 AM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
>
>
> On Fri, Nov 16, 2012 at 8:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> 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.
>>
>
> Actually, any casting involving a temporary is pretty weird:
> - downcasting a temporary makes no sense, the dynamic type matches the
> static type

Right - but it looks like, somehow, the llvm::cast machinery is
handling this case in some way.

Though it looks like the cast still returns a reference - but I think
it must be a reference to a temporary, so this code (& other uses of
the cast of TypeLocs) is still bogus, I think... but perhaps less
dangerously so.

If we could fix this to return by value, then we wouldn't have such a
problem, the original code would've failed to compile (taking a
non-const reference to a temporary) & const refs would at least do
proper lifetime extension.

> - upcasting a temporary introduces object-slicing
>
> Therefore it might be sufficient to just define:
>
> template <typename X, typename Y> BLABLABLA<X>::type cast(Y&); // near same
> definition as above, except that we will the `const` be deduced as part of
> "Y"
>
> template <typename X, typename Y> void cast(Y&&) = delete;
>
> would not it ?
>
> And then the increasing number of people compiling with C++11 compliant
> compilers would report the errors (if not already caught internally).
>
> -- Matthieu
>
>>
>> >
>> > 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
>> >
>
>




More information about the cfe-dev mailing list