[llvm] r240214 - IndVarSimplify: Avoid UB from binding a reference to a null pointer
Duncan P. N. Exon Smith
dexonsmith at apple.com
Sat Jun 27 10:25:56 PDT 2015
> On 2015 Jun 25, at 11:54, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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.
Hah, right.
>
> 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)...
It looks like r178147 implements this kind of thing for const-correct
pointers. I think something like the following would make it work for
no-const and always-const pointers as well:
template <class From> struct does_nonconst_simplify_const {
// Check whether 'const From &' is accepted by
// simplify_type<From>::getSimplifiedType().
static const bool value = ...;
};
// Just use the non-const version directly if it handles const
// pointers.
template <class From, bool IsConstCorrect = false>
struct simplify_type_const : simplify_type<From> {};
// This is the current, in-tree impl from r178147.
template <class From>
struct simplify_type_const<From, /* IsConstCorrect */ true> {
typedef
typename simplify_type<From>::SimpleType NonConstSimpleType;
typedef typename add_const_past_pointer<NonConstSimpleType>::type
SimpleType;
typedef
typename add_lvalue_reference_if_not_pointer<SimpleType>::type
RetType;
static RetType getSimplifiedValue(const From &Val) {
return simplify_type<From>::getSimplifiedValue(
const_cast<From &>(Val));
}
};
// Select the right impl, depending on whether the non-const version
// does all the work for us.
template <class From>
struct simplify_type<const From>
: simplify_type_const<From,
does_nonconst_simplify_const<T>::value> {};
If I get some time later I'll try to hack out the SFINAE to implement
`does_noconst_simplify_const`, and then set about deleting code.
>
> : 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
More information about the llvm-commits
mailing list