[Bug c/88367] New: [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] New: [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

            Bug ID: 88367
           Summary: [9 Regression] -fno-delete-null-pointer-checks doesn't
                    work properly
           Product: gcc
           Version: 9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pbutsykin at virtuozzo dot com
  Target Milestone: ---

For some reason gcc9 began to delete NULL pointer checks even with
-fno-delete-null-pointer-checks option (which should prohibit doing so).

There is the following function:
static long kmapset_cmp(struct kmapset_map *map_a, struct kmapset_map *map_b)
{
        struct kmapset_link *link_a, *link_b;

        if (map_a->hash != map_b->hash)
                return map_a->hash - map_b->hash;

        if (map_a->size != map_b->size)
                return map_a->size - map_b->size;

        link_a = hlist_entry(map_a->links.first,
                        struct kmapset_link, map_link);
        link_b = hlist_entry(map_b->links.first,
                        struct kmapset_link, map_link);
        while (&link_a->map_link) {
                if (link_a->key != link_b->key)
                        return (long)link_a->key - (long)link_b->key;
                if (link_a->value != link_b->value)
                        return link_a->value - link_b->value;
                link_a = list_entry(link_a->map_link.next,
                                struct kmapset_link, map_link);
                link_b = list_entry(link_b->map_link.next,
                                struct kmapset_link, map_link);
        }

        return map_a->default_value - map_b->default_value;
}
Full source:
https://github.com/OpenVZ/vzkernel/blob/branch-rh7-3.10.0-123.1.2-ovz/lib/kmapset.c

The problem is that gcc9 removes while condition 'while (&link_a->map_link)'
even with -fno-delete-null-pointer-checks option. There is undefined behavior
with taking the address from lvalue which doesn't designate an object. In the
case when map_a->links.first is NULL, then link_a will be equal to (NULL - 24)
and expression &((struct kmapset_link *)(NULL - 24))->map_link will refer to
NULL. Ok, it's undefined behavior and the compiler can remove this check, but
shouldn't -fno-delete-null-pointer-checks prevent this? GCC8 with
-fno-delete-null-pointer-checks don't remove this check.
GCC9 was built on this commit:
commit 3d4762327aed5cf6cafbaa7a52166df4ef92eb82
Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Tue Dec 4 11:26:14 2018 +0000

    2018-12-04  Richard Biener  <[hidden email]>

        PR tree-optimization/88301
        * tree-vrp.c (register_edge_assert_for_2): Fix sign-conversion
        issues in last commit.

The assembly code generated by gcc9 - https://pastebin.com/dkuEuyLQ
ffffffff81b4bd9c:   48 8b 48 f0             mov    rcx,QWORD PTR
[rax-0x10]  //BUG: unable to handle kernel paging request at fffffffffffffff0
(this is link_a->key)

All gcc flags that are used - https://pastebin.com/6AbyEXgF
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2018-12-05
     Ever confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Please provide preprocessed source as requested.
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |9.0

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
This isn't about NULL pointers.  &link_a->map_link is never NULL unless link_a
is a NULL pointer and map_link is the first member.  Thus GCC preserves the
check
with

struct X { int i; };

int foo (struct X *p)
{
  return &p->i == (void *)0;
}

I suspect your hlist_entry et al have a returns_nonnull attribute or so?
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #3 from Pavel <pbutsykin at virtuozzo dot com> ---
Created attachment 45160
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45160&action=edit
preprocessed source
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #4 from Pavel <pbutsykin at virtuozzo dot com> ---
Jonathan, sorry, attached.


Richard,

I forgot to show the structures:

struct kmapset_map {
        struct kref             kref;
        unsigned                size;
        struct kmapset_set      *set;
        unsigned long           default_value;
        unsigned long           hash;
        struct hlist_head       links;
        union {
                struct rb_node          node;
                struct rcu_head         rcu_head;
        };
};

struct kmapset_link {
        struct kmapset_map      *map;
        struct kmapset_key      *key;
        unsigned long           value;
        struct hlist_node       map_link;
        union {
                struct hlist_node       key_link;
                struct rcu_head         rcu_head;
        };
};



Actually link_a is'n NULL, because map_link isn't the first member and
hlist_entry is just container_of:
#define hlist_entry(ptr, type, member) container_of(ptr,type,member)

"In the case when map_a->links.first is NULL, then link_a will be equal to
(NULL - 24) and expression &((struct kmapset_link *)(NULL - 24))->map_link will
refer to NULL."
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #5 from Pavel <pbutsykin at virtuozzo dot com> ---
Jonathan, sorry, attached.


Richard,

I forgot to show the structures:

struct kmapset_map {
        struct kref             kref;
        unsigned                size;
        struct kmapset_set      *set;
        unsigned long           default_value;
        unsigned long           hash;
        struct hlist_head       links;
        union {
                struct rb_node          node;
                struct rcu_head         rcu_head;
        };
};

struct kmapset_link {
        struct kmapset_map      *map;
        struct kmapset_key      *key;
        unsigned long           value;
        struct hlist_node       map_link;
        union {
                struct hlist_node       key_link;
                struct rcu_head         rcu_head;
        };
};



Actually link_a is'n NULL, because map_link isn't the first member and
hlist_entry is just container_of:
#define hlist_entry(ptr, type, member) container_of(ptr,type,member)

"In the case when map_a->links.first is NULL, then link_a will be equal to
(NULL - 24) and expression &((struct kmapset_link *)(NULL - 24))->map_link will
refer to NULL."
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The dumps aren't very readable with all the instrumentations.
Anyway, if I look at -fdump-tree-all-lineno dumps, I see:
  [/root/src/vzkernel/lib//kmapset.c:47:8] goto <bb 19>; [100.00%]
...
  <bb 19> [local count: 2396963771]:
  # link_a_99 = PHI <[/root/src/vzkernel/lib//kmapset.c:43:9] _96(13),
[/root/src/vzkernel/lib//kmapset.c:52:10] _115(18)>
  # link_b_101 = PHI <[/root/src/vzkernel/lib//kmapset.c:45:9] _98(13),
[/root/src/vzkernel/lib//kmapset.c:54:10] _117(18)>
  # DEBUG link_b => link_b_101
  # DEBUG link_a => link_a_99
  [/root/src/vzkernel/lib//kmapset.c:47:9] _118 =
[/root/src/vzkernel/lib//kmapset.c:47:9]
&[/root/src/vzkernel/lib//kmapset.c:47:16] link_a_99->ma
p_link;
  [/root/src/vzkernel/lib//kmapset.c:47:8] if (_118 != 0B)
    goto <bb 14>; [96.34%]
  else
    goto <bb 20>; [3.66%]

still in thread1 dump, but not in vrp1 dump.
In -fdump-tree-vrp1-lineno-details-alias dump I see:
Visiting statement:
[/root/src/vzkernel/lib//kmapset.c:47:9] # PT = nonlocal escaped null
_118 = [/root/src/vzkernel/lib//kmapset.c:47:9]
&[/root/src/vzkernel/lib//kmapset.c:47:16] link_a_99->map_link;
Found new range for _118: struct hlist_node * ~[0B, 0B]
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
In GCC 8 we had:
      else if (code == POINTER_PLUS_EXPR)
        {
          /* For pointer types, we are really only interested in asserting
             whether the expression evaluates to non-NULL.  */
          if (range_is_nonnull (&vr0) || range_is_nonnull (&vr1))
            set_value_range_to_nonnull (vr, expr_type);
and now we have:
      else if (code == POINTER_PLUS_EXPR)
        {
          /* For pointer types, we are really only interested in asserting
             whether the expression evaluates to non-NULL.  */
          if (!range_includes_zero_p (&vr0)
              || !range_includes_zero_p (&vr1))
            vr->set_nonnull (expr_type);
I think range_is_nonnull (&vr1) was pretty much never true before, that needs
vr1 (which is the integral offset) to be exactly ~[0, 0]; here we had constant
offset there (-24UL).
So, if the UB stuff kernel is doing is meant to be considered ok for
-fno-delete-null-pointer-checks, we either need to guard this condition on
flag_delete_null_pointer_checks, or think of what we want to support and what
we don't.  E.g. do we want to treat pointer wrapping as UB?  A problem is that
POINTER_PLUS_EXPR last argument is unsigned sizetype, so negative offsets
appear as very large positive ones.  So, perhaps do we want to conclude that if
vr0 doesn't include zero and vr1 is guaranteed not to have most significant bit
set (i.e. appear as negative), then the result is nonnull, otherwise varying?
Of course for flag_delete_null_pointer_checks do what we do right now.
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 45166
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=45166&action=edit
gcc9-pr88367.patch

Possible untested patch.
Reply | Threaded
Open this post in threaded view
|

[Bug c/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think even the old

          if (range_is_nonnull (&vr0) || range_is_nonnull (&vr1))
            set_value_range_to_nonnull (vr, expr_type);

code is wrong with -fno-delete-null-pointer-checks (which is overloaded with
the case that address zero is a valid pointer to an object).  So IMHO you
can simplify the patch and guard the affected places with
flag_delete_null_pointer_checks.

Strictly the kernel case of computing NULL - 24 invokes UB and needs
-fwrapv-pointer (but IIRC the kernel uses -fno-strict-overflow which
enables that already).  I don't think we exploint wrapping UB for
pointers in the VRP case though.
Reply | Threaded
Open this post in threaded view
|

[Bug tree-optimization/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Thu Dec  6 23:28:04 2018
New Revision: 266878

URL: https://gcc.gnu.org/viewcvs?rev=266878&root=gcc&view=rev
Log:
        PR c/88367
        * tree-vrp.c (extract_range_from_binary_expr): For POINTER_PLUS_EXPR
        with -fno-delete-null-pointer-checks, set_nonnull only if the pointer
        is non-NULL and offset is known to have most significant bit clear.
        * vr-values.c (vr_values::vrp_stmt_computes_nonzero): For ADDR_EXPR
        of MEM_EXPR, return true if the MEM_EXPR has non-zero offset with
        most significant bit clear.  If offset does have most significant bit
        set and -fno-delete-null-pointer-checks, don't return true even if
        the base pointer is non-NULL.

        * gcc.dg/tree-ssa/pr88367.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr88367.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c
    trunk/gcc/vr-values.c
Reply | Threaded
Open this post in threaded view
|

[Bug tree-optimization/88367] [9 Regression] -fno-delete-null-pointer-checks doesn't work properly

mpolacek at gcc dot gnu.org
In reply to this post by mpolacek at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88367

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Hopefully fixed now.