Re: [boost] [TypeIndex] Peer review period for library acceptance begins, ending Thurs 21st Nov

Subject: Re: [boost] [TypeIndex] Peer review period for library acceptance begins, ending Thurs 21st Nov
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2013-11-15 16:09:38

On 15 Nov 2013 at 19:45, Antony Polukhin wrote:

> > ...and can be arbitrarily broken by (unknown) future
> > optimizations, with no recourse.  We wouldn't have
> > all the problems with strict aliasing that we do today
> > if programmers 10 years ago hadn't made exactly the
> > same argument that you are, now.  (Sure, it's undefined
> > behavior, but we know what the compiler actually does,
> > so it's really okay...).
> 
> You're absolutely right. I've liked the idea of "make it in a way that user
> won't notice a difference but typeid issues will be solved" so much, that
> turned a blind eye to a hack with UB.
> 
> This will be fixed (possibly code from v1.0 will be taken, which had no
> boost::type_info and contained type_index class that had a pointer to
> std::type_info).
> 
> Do you have other ideas of what can be fixed/improved?

Before you make any substantial changes here Antony, I have been busy 
pondering on this UB issue and on what to recommend to the review 
wizards. Here are the facts:

1. The problem code is like this:

static_cast<const boost::type_info&>(typeid(no_cvr_t));

... where typeid() returns a std::type_info and where 
boost::type_info singly inherits publicly from std::type_info and no 
other type.

2. Note that std::type_info contains a virtual function table on 
libstdc++, Dinkumware and probably any reasonable STL implementation. 
Therefore boost::type_info will also contain a virtual function 
table.

3. The problematic static cast is an upcast to a derived type, not a 
cast between unrelated types. No multiple or virtual inheritance is 
in play.

4. Which would be legal if and only if typeid originally returned a 
boost::type_info, because you can legally cast to a base type and 
back to a derived type.

Here are my thoughts:

1. I would be happy with the static upcast if no virtual function 
table were in play because it's single inheritance, and static_cast 
provides enough safeguards (e.g. erroring if the type's inheritance 
tree is unknown at the point of static_cast use). It's a very common 
pattern in C++ code and is used throughout (especially older parts 
of) Boost. It is sufficiently common that no future compiler could 
ever disable it or misoperate with it. I agree that could hinder 
significantly future C++ features, but that is not TypeIndex's 
problem because of so much preexisting usage. It's an established 
idiom, and that's that: in my opinion TypeIndex has the right to use 
established idioms if they are currently safe with all possible 
present compiler technologies as implied by the present C++ standard.

2. Unfortunately, there IS a virtual function table, and therefore 
this line would fail:

dynamic_cast<const boost::type_info&>(typeid(no_cvr_t));

where this line does not fail:

static_cast<const boost::type_info&>(typeid(no_cvr_t));

That seems to me to be unacceptable. If a virtual function table is 
present, it MUST be correct with respect to the type the compiler is 
using it as.

So, Antony, if you can get the virtual function table to correctly 
say it's a boost::type_info and not a std::type_info, I'll recommend 
in my report this is fine. I don't mind a certain amount of evil :) 
to achieve that given it's such an isolated case which can be easily 
wrapped in a single location.

Niall

-- 
Currently unemployed and looking for work.
Work Portfolio: http://careers.stackoverflow.com/nialldouglas/