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

[David Blaikie]

On Thu, Jun 25, 2015 at 11:46 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Jun-25, at 10:27, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > 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)
>
> I wouldn't say it's a bug, but I like the idea of making the `const T`
> version work for `T` if there isn't one available.
>
> Would this just work?
>
>     template <class T> struct simplify_type<T>


That would be an unspecialized specialization.

So we could do it the other way:

template<typename T> struct simplify_type<const T> : simplify_type<T> {};

But that won't work for all types (& you're right, maybe that's just a
matter of diagnostic quality, not functional correctness)...


> : simplify_type<const T> {};
>
> My guess is that we'd really want an `enable_if<>` in there somewhere
> to keep the compiler errors sane though.  Not exactly sure.
>
> >
> >
> > > 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/eca48bc2/attachment.html>