On Wed, Dec 5, 2012 at 1:06 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Thu, Nov 29, 2012 at 8:16 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> On Thu, Nov 29, 2012 at 7:43 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Thu, Nov 29, 2012 at 3:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> > Moving to LLVM dev to discuss the possibility of extending the cast<br>
>> > infrastructure to handle this.<br>
>> ><br>
>> > On Tue, Nov 20, 2012 at 5:51 PM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
>> >> On Nov 18, 2012, at 5:05 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> >>> TypeLoc casting looks bogus.<br>
>> >>><br>
>> >>> TypeLoc derived types return true from classof when the dynamic type<br>
>> >>> is not actually an instance of the derived type. The TypeLoc is then<br>
>> >>> (erroneously) cast to the derived type and the derived type's implicit<br>
>> >>> copy ctor is executed (so long as you remember to assign the result of<br>
>> >>> cast<SpecificTypeLoc>), if you copy, otherwise the SpecificTypeLoc's<br>
>> >>> member functions are actually invoked on a TypeLoc object - bogas, but<br>
>> >>> it works (because there's no other members in the SpecificTypeLoc<br>
>> >>> types).<br>
>> >><br>
>> >> Yep. See also LLVM's IntrinsicInst. This kind of UB is very common<br>
>> >> and<br>
>> >> essentially harmless, but if you want to stamp it out, feel free.<br>
>> >><br>
>> >>> Richard / Kostya: what's the word on catching this kind of UB<br>
>> >>> (essentially: calling member functions of one type on an instance not<br>
>> >>> of that type)? (in the specific case mentioned below, as discussed in<br>
>> >>> the original thread, ASan or MSan might be able to help)<br>
>> >>><br>
>> >>> Clang Dev: what should we do to fix this? I don't think it's helpful<br>
>> >>> to add machinery to llvm::cast to handle concrete type conversion, so<br>
>> >>> I think we should consider a non-llvm::cast solution for this<br>
>> >>> hierarchy. Replace the llvm::isa calls with getTypeLocClass calls (or<br>
>> >>> a separate wrapper for that) & add a SpecificTypeLoc(const TypeLoc&)<br>
>> >>> ctor for every specific TypeLoc that has an appropriate assert & calls<br>
>> >>> up through to the base copy ctor. It'll be a fair bit of code to add<br>
>> >>> to TypeLoc, but nothing complicated.<br>
>> >>><br>
>> >>> Any other ideas?<br>
>> >><br>
>> >> I don't see why isa<> and cast<> aren't the right code interface for<br>
>> >> this,<br>
>> >> rather than reinventing something else. You just need to teach cast<><br>
>> >> to construct and return a proper temporary.<br>
>> ><br>
>> > LLVM-dev folks: what do you reckon? Should we extend the cast<br>
>> > infrastructure to be able to handle value type conversions?<br>
>><br>
>> No, I don't think that makes sense; the casting infrastructure is<br>
>> complicated enough without having to deal with this. It would be easy<br>
>> enough to implement a method on TypeLoc to handle this,<br>
>><br>
>> > It doesn't really feel like the right fit to me, but I'll defer to the<br>
>> > community's wisdom if it's overwhelming. I'd just like to see a<br>
>> > solution wherein we can't write this particular kind of bug again,<br>
>> > ideally.<br>
>><br>
>> Attaching proof-of-concept solution which prevents this kind of bug.<br>
>> (Ugly, and requires C++11 support as written, but potentially<br>
>> interesting anyway.)<br>
><br>
><br>
> The C++11 should be easy to remove (move the enable_if to the return type).<br>
> This approach seems reasonable to me.<br>
<br>
</div></div>Thanks Eli - that does seem to do the trick (took me a little while to<br>
wrap my head around it, though). I've modified it, as Richard<br>
suggested, to use enable_if in the return type & it builds in C++98<br>
and correctly catches the TypeLoc usage.<br>
<br>
I'll have to find some time to actually go & fix TypeLoc before we can<br>
submit this, though.<br>
<br>
(& technically we could allow casting where the parameter type is a<br>
temporary, so long as you only cast to the same type - but that's not<br>
much use. I wonder if we should trivially disallow casting when the<br>
source/dest type are the same anyway? But I suppose there's no harm in<br>
allowing that either, except that it might confuse the reader a bit<br>
when they see a no-op cast like that) </blockquote><div><br></div><div>ISTR we have cases of no-op casts in templates and in code generated by x macros. These seem reasonably harmless.</div></div>