[Bug tree-optimization/58920] New: Overeager optimization based on TREE_THIS_NOTRAP

[Bug tree-optimization/58920] New: Overeager optimization based on TREE_THIS_NOTRAP

[jason at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

            Bug ID: 58920
           Summary: Overeager optimization based on TREE_THIS_NOTRAP
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jason at gcc dot gnu.org

Created attachment 31108
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31108&action=edit
Patch to set TREE_THIS_NOTRAP for C++ references

The TREE_THIS_NOTRAP macro came up in email the other day, and it seemed to me
that it would be useful to set on C++ references, since they are required to
refer to objects; trying to read from a null reference gives undefined
behavior.  So I tried the attached patch.  But it breaks all the ext_pointer
tests in libstdc++.

Basically, what's happening is that there is a code path which is never taken
which leads to an explicit null dereference.  The optimizers see this happening
within a loop and decide to hoist it out of the loop.  So now it is executed
before the loop starts, and causes a SEGV.

Is this an appropriate optimization?

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[jason at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #1 from Jason Merrill <jason at gcc dot gnu.org> ---
Created attachment 31109
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31109&action=edit
Simplified testcase

Here's a simplified version of one of the ext_pointer testcases.  Compiling it
with -O -fstrict-aliasing (and the attached front end patch) produces the bug.

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The problem is the middle-end treats pointers and references as the same so
sometimes it removes the casting between the pointer type and reference types. 
GCC has no representation of conditional no trapping which is what is needed
here.

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-10-30
                 CC|                            |ebotcazou at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> The TREE_THIS_NOTRAP macro came up in email the other day, and it seemed to
> me that it would be useful to set on C++ references, since they are required
> to refer to objects; trying to read from a null reference gives undefined
> behavior.  So I tried the attached patch.  But it breaks all the ext_pointer
> tests in libstdc++.
> 
> Basically, what's happening is that there is a code path which is never
> taken which leads to an explicit null dereference.  The optimizers see this
> happening within a loop and decide to hoist it out of the loop.  So now it
> is executed before the loop starts, and causes a SEGV.

If the dereference can generate a SEGV before being moved and nevertheless has
the TREE_THIS_NOTRAP flag, then this is the bug and loop invariant motion does
not make things worse.

As Andrew explained, you cannot set TREE_THIS_NOTRAP on all references.  In
Ada,
we set it only on parameters passed by reference, but it needs to be cleared
during inlining because of this kind of issues.

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #3)
> > The TREE_THIS_NOTRAP macro came up in email the other day, and it seemed to
> > me that it would be useful to set on C++ references, since they are required
> > to refer to objects; trying to read from a null reference gives undefined
> > behavior.  So I tried the attached patch.  But it breaks all the ext_pointer
> > tests in libstdc++.
> > 
> > Basically, what's happening is that there is a code path which is never
> > taken which leads to an explicit null dereference.  The optimizers see this
> > happening within a loop and decide to hoist it out of the loop.  So now it
> > is executed before the loop starts, and causes a SEGV.
> 
> If the dereference can generate a SEGV before being moved and nevertheless
> has the TREE_THIS_NOTRAP flag, then this is the bug and loop invariant
> motion does
> not make things worse.
> 
> As Andrew explained, you cannot set TREE_THIS_NOTRAP on all references.  In
> Ada,
> we set it only on parameters passed by reference, but it needs to be cleared
> during inlining because of this kind of issues.

"This kind of issues" is that GCC treats TREE_THIS_NOTRAP as applying
independent of the execution context.  That is, if you have

  if (p)
    *p = 0;

and mark *p with TREE_THIS_NOTRAP then GCC thinks *p will not trap
even when moved before the if (p) check.  So it has no concept of
"conditionally doesn't trap".

Though I thought that for C++ references that would be a non-issue.
As of pointer vs. reference types this shouldn't matter here as you
annotate actualy tcc_reference trees, not types.

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> As of pointer vs. reference types this shouldn't matter here as you
> annotate actualy tcc_reference trees, not types.

What Andrew meant is that one can think that it would be valid to set the flag
on tcc_reference trees if operand #0 has REFERENCE_TYPE instead of
POINTER_TYPE,
but of course that doesn't work because they can be smashed together.

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #5)
> > As of pointer vs. reference types this shouldn't matter here as you
> > annotate actualy tcc_reference trees, not types.
> 
> What Andrew meant is that one can think that it would be valid to set the
> flag
> on tcc_reference trees if operand #0 has REFERENCE_TYPE instead of
> POINTER_TYPE,
> but of course that doesn't work because they can be smashed together.

Yeah, that can't be the way to distinguish them (though on GENERIC they
are not smashed together and the Frontend would do the TREE_THIS_NOTRAP
setting anyway).

[Bug tree-optimization/58920] Overeager optimization based on TREE_THIS_NOTRAP

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58920

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
maybe have the front-end emit (if -fdelete-null-pointer-checks is on),
if (ref == NULL) __builtin_unreachable();

This might most cases, how many I don't know.