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

David Blaikie dblaikie at gmail.com
Thu Jun 25 10:27:50 PDT 2015


On Wed, Jun 24, 2015 at 3:28 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > 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.
>

Is this a bug in the way the traits work - such that it should work for
const and non-const? Something we could fix more generally? (or should we
make it so that a specialization for simplify_type of const T also works
for T? So shallow-const handles like this only need one specialization)


>
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150625/2ab2b48d/attachment.html>


More information about the llvm-commits mailing list