Hi,
On Fri, Apr 25, 2014 at 11:22:22AM +0100, Andrew Haley wrote:
> Summary: Devirtualization uses type information to determine if a
> virtual method is reachable from a call site. If type information
> indicates that it is not, devirt marks the site as unreachable. I
> think this is wrong, and it breaks some programs. At least, it should
> not do this if the user specifies -fno-strict-aliasing.
>
> Consider this class:
>
> class Container {
> void *buffer[5];
> public:
> EmbeddedObject *obj() { return (EmbeddedObject*)buffer; }
> Container() { new (buffer) EmbeddedObject(); }
> };
>
> Placement new is used to embed an object in a buffer inside another
> object. Its address can be retrieved. This usage of placement new is
> common, and it even appears as the canonical use of placement new in
> the in the C++ FAQ at
> http://www.parashift.com/c++-faq/placement-new.html. (I am aware that
> this is not strictly legal. For one thing, the memory at buffer may
> not be suitably aligned. Please bear with me.)
>
> The embedded object is an instance of:
>
> class EmbeddedObject {
> public:
> virtual int val() { return 2; }
> };
>
> And it is called like this:
>
> extern Container o;
> int main() {
>
> cout << o.obj()->val() << endl;
> }
>
> The devirtualization pass looks into the call to val() and the type of
> o, decides that there is no type inside o that is compatible with
> EmbeddedObject, and inserts a call to __builtin_unreachanble(). As a
> result, instead of printing 2, the program does nothing.
>
> I'm not sure what this transformation is supposed to achieve. It does
> nothing for strictly compliant code
It can be and often is useful. Very often such calls are in a branch
that is guarded by a type check and performs a down-cast.
Devirtualization can figure out the branch is unreachable when it
knows that the call in it would be illegal. (And yes, placement new
has been considered, see PR 45734, especially comment #4.)
In this case, however, I believe that ipa-devirt should not mark its
result as final if the offset with outer_type leads to a
non-artificial field, certainly not if such a field is a POD,
regardless of any compiler option. I believe you should open a PR for
this.
Martin
> and it silently breaks non-
> compliant code like this. GCC users would be better off if it were
> not done. At least an error message could be printed instead of silently
> failing to generate code.
>
> IMO, this transformation should not be performed when
> -fno-strict-aliasing is used.
>
> `-fstrict-aliasing'
> Allow the compiler to assume the strictest aliasing rules
> applicable to the language being compiled. For C (and C++), this
> activates optimizations based on the type of expressions.
>
> I have appended a suggested patch to this message.
>
> Andrew.
>
>
> Index: gcc/ipa-devirt.c
> ===================================================================
> --- gcc/ipa-devirt.c (revision 209656)
> +++ gcc/ipa-devirt.c (working copy)
> @@ -1362,8 +1362,9 @@
>
> /* Only type inconsistent programs can have otr_type that is
> not part of outer type. */
> - if (!contains_type_p (TREE_TYPE (base),
> - context->offset + offset2, *otr_type))
> + if (flag_strict_aliasing
> + && !contains_type_p (TREE_TYPE (base),
> + context->offset + offset2,
> *otr_type))
> {
> /* Use OTR_TOKEN = INT_MAX as a marker of probably type
> inconsistent
> code sequences; we arrange the calls to be
> builtin_unreachable
> @@ -1441,7 +1442,8 @@
> gcc_assert (!POINTER_TYPE_P (context->outer_type));
> /* Only type inconsistent programs can have otr_type that is
> not part of outer type. */
> - if (!contains_type_p (context->outer_type, context->offset,
> + if (flag_strict_aliasing
> + && !contains_type_p (context->outer_type, context->offset,
> *otr_type))
> {
> /* Use OTR_TOKEN = INT_MAX as a marker of probably type
> inconsistent
>
