[llvm] r240214 - IndVarSimplify: Avoid UB from binding a reference to a null pointer

[Justin Bogner]

David Blaikie <dblaikie at gmail.com> writes:
> On Fri, Jun 19, 2015 at 11:52 PM, Justin Bogner <mail at justinbogner.com> wrote:
>> David Blaikie <dblaikie at gmail.com> writes:
>>> On Fri, Jun 19, 2015 at 11:24 PM, Justin Bogner <mail at justinbogner.com wrote:
>>>> -  while (!DeadInsts.empty())
>>>> -    if (Instruction *Inst =
>>>> -          dyn_cast_or_null<Instruction>(&*DeadInsts.pop_back_val()))
>>>> +  while (!DeadInsts.empty()) {
>>>> +    Value *V = static_cast<Value *>(DeadInsts.pop_back_val());
>>>> +    if (Instruction *Inst = dyn_cast_or_null<Instruction>(V))
>>>>
>>> Hmm - I think there's some fancy machinery in the llvm cast stuff to allow
>>> us to map through from different types (so we could say that casting a
>>> WeakVH retrieves the Value* first), maybe... (so you could just
>>> dyn_cast_or_null<Value*>(DeadInsts.pop_back_val()) directly)
>>
>> I don't really understand what you're going for here - we need to cast
>> twice. We have a WeakVH, which we can get a Value* out of, then we
>> dyn_cast_or_null that to an Instruction*. There's no need to dyn_cast to
>> a Value* (we want the conversion operator), and obviously WeakVH can't
>> be cast to Instruction* directly, since they're unrelated types.
>
> What I mean is we have a way of making them related, through a template
> called simplify_type. Here's the version for QualType to allow QualTypes to
> be casted to specific clang::Types: http://clang.llvm.org/doxygen/
> structllvm_1_1simplify__type_3_01_1_1clang_1_1QualType_01_4.html 
>
> I think it /might/ work here, but I don't know/recall the specifics -
> perhaps it doesn't fit here for some reason.

Ah, I see what you mean. Interestingly, WeakVH already has a
specialization of simplify_type, so you would expect this to work:

    if (Instruction *Inst =
            dyn_cast_or_null<Instruction>(DeadInsts.pop_back_val()))

but it doesn't - the rvalue gets type `const llvm::Instruction *` so we
get an error for trying to remove the const without a cast. On the other
hand, this does work:

    WeakVH WVH = DeadInsts.pop_back_val();
    if (Instruction *Inst = dyn_cast_or_null<Instruction>(WVH))

So for whatever reason the template specialization chooses the const
version (even though the non-const version is also viable) when we give
it an rvalue. I guess this is a bug or limitation in simplify_type.

As it stands, I find the static_cast clearer than the implicit
conversion, especially since the obvious transformation of getting rid
of the temp variable doesn't work with the implicit conversion, so I'm
going to leave this as is.