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

[Duncan P. N. Exon Smith]

> On 2015 Jun 20, at 01:39, Justin Bogner <mail at justinbogner.com> wrote:
> 
> 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.

Just needed another template specialization for `const WeakVH`.  I added
one (and used it in IndVarSimplify) in r240599.

> 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.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits