public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c/104288] New: Null pointer check invalidly deleted
@ 2022-01-30 10:09 nrk at disroot dot org
  2022-01-30 10:26 ` [Bug middle-end/104288] [11/12 Regression] " pinskia at gcc dot gnu.org
                   ` (16 more replies)
  0 siblings, 17 replies; 18+ messages in thread
From: nrk at disroot dot org @ 2022-01-30 10:09 UTC (permalink / raw)
  To: gcc-bugs

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

            Bug ID: 104288
           Summary: Null pointer check invalidly deleted
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: nrk at disroot dot org
  Target Milestone: ---

Hi,

In the following code, the null checks are removed when compiled with `-O2` on
gcc v11.2.1. Compiling with `-O2 -fno-delete-null-pointer-checks` produces
proper result.

        #include <stdbool.h>
        #include <stdio.h>
        #include <stdlib.h>
        #include <string.h>


        #define RESULT_PASS "PASS"
        #define RESULT_FAIL "FAIL"


        void test_assert(bool result)
        {
            printf("Assert: %s\n", result ? RESULT_PASS : RESULT_FAIL);

            if (!result)
                exit(EXIT_FAILURE);
        }


        void test_strcmp(const char *value_1, const char *value_2)
        {
            test_assert(value_1 != NULL);
            test_assert(value_2 != NULL);

            bool result = !strcmp(value_1, value_2);

            printf("Test equal: %s\n", result ? RESULT_PASS : RESULT_FAIL);
        }


        int main()
        {
            test_strcmp(NULL, "value 1");
        }

(code snippet taken from:
https://gist.github.com/novns/c84d6e1efd6304b3076811fef34096fd )

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug middle-end/104288] [11/12 Regression] Null pointer check invalidly deleted
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
@ 2022-01-30 10:26 ` pinskia at gcc dot gnu.org
  2022-01-30 10:29 ` [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative pinskia at gcc dot gnu.org
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2022-01-30 10:26 UTC (permalink / raw)
  To: gcc-bugs

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-01-30
            Summary|Null pointer check          |[11/12 Regression] Null
                   |invalidly deleted           |pointer check invalidly
                   |                            |deleted
           Keywords|                            |wrong-code
   Target Milestone|---                         |11.3
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.

Reduced/more self-contained testcase:
void h(int result) __attribute__((noipa));
void h(int result)
{
    if (result)
        __builtin_exit(0);
}
void n(const char *value_1) __attribute__((noipa));
void n(const char *value_1)
{
    h(value_1 == 0);

    int result = !__builtin_strcmp(value_1, "value 1");
    __builtin_exit (!result);
}
int main()
{
    n(0);
}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
  2022-01-30 10:26 ` [Bug middle-end/104288] [11/12 Regression] " pinskia at gcc dot gnu.org
@ 2022-01-30 10:29 ` pinskia at gcc dot gnu.org
  2022-01-30 11:03 ` jakub at gcc dot gnu.org
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2022-01-30 10:29 UTC (permalink / raw)
  To: gcc-bugs

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection
          Component|middle-end                  |tree-optimization

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
EVRP is removing the null pointer check. I suspect the ranger code is not
taking into account where we are in the IR and it thinks the arguments to
strcmp will be null pointers but before we have other function calls which
might not return.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
  2022-01-30 10:26 ` [Bug middle-end/104288] [11/12 Regression] " pinskia at gcc dot gnu.org
  2022-01-30 10:29 ` [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative pinskia at gcc dot gnu.org
@ 2022-01-30 11:03 ` jakub at gcc dot gnu.org
  2022-01-31  8:21 ` rguenth at gcc dot gnu.org
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-01-30 11:03 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |amacleod at redhat dot com,
                   |                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r11-3685-gfcae5121154d1c3382b056bcc2c563cedac28e74

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (2 preceding siblings ...)
  2022-01-30 11:03 ` jakub at gcc dot gnu.org
@ 2022-01-31  8:21 ` rguenth at gcc dot gnu.org
  2022-01-31 14:53 ` amacleod at redhat dot com
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: rguenth at gcc dot gnu.org @ 2022-01-31  8:21 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
strcmp has a nonnull attribute which means all pointer parameters are not NULL
which probably makes ranger conclude that on all paths they cannot be NULL,
bogously eliding the asserts (note s/exit/__builtin_unreachable()/ would make
this conclusion OK I think).

IMHO this is a security issue, making P1 even though we already released 11.1
and 11.2 with this bug.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (3 preceding siblings ...)
  2022-01-31  8:21 ` rguenth at gcc dot gnu.org
@ 2022-01-31 14:53 ` amacleod at redhat dot com
  2022-01-31 15:34 ` jakub at gcc dot gnu.org
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: amacleod at redhat dot com @ 2022-01-31 14:53 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #5 from Andrew Macleod <amacleod at redhat dot com> ---
The issue is that the routine to determine non-nullness is being called to
check for range-on-entry of the current block instead of just the dominators. 

The trace shows:
24   range_on_entry (value_1_7(D)) to BB 2
25     range_of_stmt (value_1_7(D)) at stmt GIMPLE_NOP
       TRUE : (25) range_of_stmt (value_1_7(D)) const char * VARYING
     TRUE : (24) range_on_entry (value_1_7(D)) const char * [1B, +INF]

so it correctly determines that the range of value_1_7 is VARYING, but
range-on-entry to bb2 is adjusted to be non-null.  Ultimately,this is because
then routine in question answers the question "is there a non-null reference in
block BB".

Range on entry should not consider the current block, it should instead start
looking at the dominator to this block.

That section of code has changed between gcc11 and 12, so there will be 2
slightly different patches coming.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (4 preceding siblings ...)
  2022-01-31 14:53 ` amacleod at redhat dot com
@ 2022-01-31 15:34 ` jakub at gcc dot gnu.org
  2022-01-31 22:39 ` amacleod at redhat dot com
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: jakub at gcc dot gnu.org @ 2022-01-31 15:34 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Pedantically, even in the same bb the non-NULLness applies, but only for the
stmt with the non-NULL access (e.g. dereference or strcmp call like in this
testcase) or in stmts before it unless there is a stmt that might not return
(exit/abort/loop forever), might throw externally (internal throw would mean
different bbs), might longjmp out of it etc. or, depending on the recent
discussions perhaps volatile accesses.
But most passes that use the ranger don't track easily which stmt comes before
another one in the same bb (with the exception of say reassoc), so maybe what
you talk about is the only thing ranger can do at least for now.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (5 preceding siblings ...)
  2022-01-31 15:34 ` jakub at gcc dot gnu.org
@ 2022-01-31 22:39 ` amacleod at redhat dot com
  2022-02-01  8:04 ` rguenther at suse dot de
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: amacleod at redhat dot com @ 2022-01-31 22:39 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #7 from Andrew Macleod <amacleod at redhat dot com> ---
its a bit more tightly intertwined than that unfortunately.  I had plans to
replace the current non-null processing with a range_after_stmt side-effect API
which would work in conjunction with dominator walks, but never got to it in
this release.

So ranger makes the assumption currently that if the non-null property is found
anywhere in the BB, it applies to the entire BB, unless
can_throw_non_call_exceptions is true, then it just bails.

The unfortunate side effect of this is we can no longer determine the
difference between this test case and (vrp111.c):


void foo (void *p) __attribute__((nonnull(1)));
void bar (void *p)
{
  foo (p);
  if (!p)
    __builtin_abort ();
}

where the use determining non-null happens first, and then we can eliminate
branches safely.  ie,so a problematic test case would be a combination:

void h(int result)
{
    if (result)
        __builtin_exit(0);
}
void foo (void *p) __attribute__((nonnull(1)));
void bar (void *p)
{
  h(p == 0);    // can't eliminate this
  foo (p);      // this makes p non=null
  if (!p)       // should be able to eliminate this
    __builtin_abort ();
}



I'm contemplating the situation.  The trick is to find something with minimal
change that is functionally acceptable.   I think the goal for this release
would be to continue to get vrp111.c, and simply not get it in the problematic
case..  ie, if the first use in the block is unknown and then LATER in the
block we figure out that it is non-null, we will miss that case through-out 
the block for now.  

Effectively: if no dominator block contains non-null then whatever the first
use in a block determines will apply to the whole block, for the duration of
the block.. but upon exit to the block, if it was non-null anywhere, then all
following blocks get that knowledge.  

I think this could be done reasonably efficiently with a minimum of risk/churn.
 At least that is what I'M currently trying.  would this be OK?

And for GCC 11 it is not much of an issue. Since it runs in hybrid mode, I can
simply switch ranger to be more pessimistic and deal with just dominators if
they are available, EVRP picks up the slack,  and it still pass all the
testsuite cases.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (6 preceding siblings ...)
  2022-01-31 22:39 ` amacleod at redhat dot com
@ 2022-02-01  8:04 ` rguenther at suse dot de
  2022-02-02 21:52 ` amacleod at redhat dot com
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: rguenther at suse dot de @ 2022-02-01  8:04 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 31 Jan 2022, amacleod at redhat dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288
> 
> --- Comment #7 from Andrew Macleod <amacleod at redhat dot com> ---
> I'm contemplating the situation.  The trick is to find something with minimal
> change that is functionally acceptable.   I think the goal for this release
> would be to continue to get vrp111.c, and simply not get it in the problematic
> case..  ie, if the first use in the block is unknown and then LATER in the
> block we figure out that it is non-null, we will miss that case through-out 
> the block for now.  
> 
> Effectively: if no dominator block contains non-null then whatever the first
> use in a block determines will apply to the whole block, for the duration of
> the block.. but upon exit to the block, if it was non-null anywhere, then all
> following blocks get that knowledge.  
> 
> I think this could be done reasonably efficiently with a minimum of risk/churn.
>  At least that is what I'M currently trying.  would this be OK?

Let's see what you can come up with.

For ranges derived from stmts that are not the last in a BB (in which case
they apply to outgoing edges) you have to strictly adhere to the notion
that you have ranges incoming into a stmt and ranges outgoing so each
stmt is the source of new ranges.  That applies to divisions
(nonzero divisor) or shifts (no out of range shift operand).  But you
may not apply the range to all stmts of a block (including the stmt
producing the range itself).  With the old EVRP scheme I failed to
nicely support deriving non-zero divisor ranges for this reason for
example.

That makes it complicated to handle in an on-demand fashion of course
since you'd need a per stmt cache that is much more difficult to manage
and look up.

(which is why I really did like to have the old EVRP since conceptually
it's easy to have a very fine-grained "context")

Practically I think it's OK for true on-demand users to have the
less precise per-BB non-NULL data.  But the value-range passes
should really be able to keep a precise per-stmt "context".

Did I say I liked the old EVRP way of doing things? ;)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (7 preceding siblings ...)
  2022-02-01  8:04 ` rguenther at suse dot de
@ 2022-02-02 21:52 ` amacleod at redhat dot com
  2022-02-08 15:03 ` cvs-commit at gcc dot gnu.org
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: amacleod at redhat dot com @ 2022-02-02 21:52 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #9 from Andrew Macleod <amacleod at redhat dot com> ---
 risk/churn.
> >  At least that is what I'M currently trying.  would this be OK?
> 
> Let's see what you can come up with.


> (which is why I really did like to have the old EVRP since conceptually
> it's easy to have a very fine-grained "context")
> 
> Practically I think it's OK for true on-demand users to have the
> less precise per-BB non-NULL data.  But the value-range passes
> should really be able to keep a precise per-stmt "context".
> 
> Did I say I liked the old EVRP way of doing things? ;)

Intention has always been to introduce a range_after_stmt to the API for
side-effects, which would merge some aspects of the way EVRP did things.

I have a patchset which I will submit shortly.. once everything passes mustard.
Its actually not that significant of a change.

Ranger currently tracks non-null for an ssa-name in blocks with a single bit. 
I change that to 2 bits so we can tell if all uses in the block have the
non-null property, or if there are some uses which do not have the property.

range-on-entry is changed to only check the dominators, and range-of-expr is
changed to only apply the non-null property if the entire block has only uses
with the non-null property.

After trying to simply/fold each statement in the domwalk, I now check if the
statement sets the non-null property. If so, then mark this block as non-null
throughout, and any further queries through range_of_expr within this block
will now pick up the non-null property. 

This allows us the flexibility of EVRPs granular walk, while remaining
conservative with any on demand clients.  This also solves the problematic
testcase I showed earlier, producing the desired:
  _1 = p_3(D) == 0B;
  _2 = (int) _1;
  h (_2);
  foo (p_3(D));
  return;

Anyway, patch coming soon. I believe it to be low risk as the changes are all
localized, and results should always be more conservative with the additonal 
granularity.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (8 preceding siblings ...)
  2022-02-02 21:52 ` amacleod at redhat dot com
@ 2022-02-08 15:03 ` cvs-commit at gcc dot gnu.org
  2022-02-09 14:10 ` cvs-commit at gcc dot gnu.org
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-02-08 15:03 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Andrew Macleod
<amacleod@gcc.gnu.org>:

https://gcc.gnu.org/g:ed35d4205e8c139d27d3d47c528aaa9f82f0ac1b

commit r11-9543-ged35d4205e8c139d27d3d47c528aaa9f82f0ac1b
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Mon Jan 31 11:37:16 2022 -0500

    Range on entry should only check dominators for non-null.

    Range-on-entry checks should no check the state of non-null within the
current
    block.   If dominators are present, use the dominator.

            PR tree-optimization/104288
            gcc/
            * gimple-range-cache.cc (ssa_range_in_bb): Only use non-null from
the
            dominator entry ranges.
            * gimple-range.cc (gimple_ranger::range_of_expr): Ditto.
            gcc/testsuite/
            * gcc.dg/pr104288.c: New.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (9 preceding siblings ...)
  2022-02-08 15:03 ` cvs-commit at gcc dot gnu.org
@ 2022-02-09 14:10 ` cvs-commit at gcc dot gnu.org
  2022-02-09 14:11 ` amacleod at redhat dot com
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-02-09 14:10 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Andrew Macleod <amacleod@gcc.gnu.org>:

https://gcc.gnu.org/g:c6bb1db76b3ac127aff7dacf391fc1798a94bb7d

commit r12-7128-gc6bb1db76b3ac127aff7dacf391fc1798a94bb7d
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Mon Feb 7 15:52:16 2022 -0500

    Register non-null side effects properly.

    This patch adjusts uses of nonnull to accurately reflect "somewhere in
block".
    It also adds the ability to register statement side effects within a block
    for ranger which will apply for the rest of the block.

            PR tree-optimization/104288
            gcc/
            * gimple-range-cache.cc (non_null_ref::set_nonnull): New.
            (non_null_ref::adjust_range): Move to header.
            (ranger_cache::range_of_def): Don't check non-null.
            (ranger_cache::entry_range): Don't check non-null.
            (ranger_cache::range_on_edge): Check for nonnull on normal edges.
            (ranger_cache::update_to_nonnull): New.
            (non_null_loadstore): New.
            (ranger_cache::block_apply_nonnull): New.
            * gimple-range-cache.h (class non_null_ref): Update prototypes.
            (non_null_ref::adjust_range): Move to here and inline.
            (class ranger_cache): Update prototypes.
            * gimple-range-path.cc (path_range_query::range_defined_in_block):
Do
            not search dominators.
            (path_range_query::adjust_for_non_null_uses): Ditto.
            * gimple-range.cc (gimple_ranger::range_of_expr): Check on-entry
for
            def overrides.  Do not check nonnull.
            (gimple_ranger::range_on_entry): Check dominators for nonnull.
            (gimple_ranger::range_on_edge): Check for nonnull on normal edges..
            (gimple_ranger::register_side_effects): New.
            * gimple-range.h (gimple_ranger::register_side_effects): New.
            * tree-vrp.cc (rvrp_folder::fold_stmt): Call register_side_effects.

            gcc/testsuite/
            * gcc.dg/pr104288.c: New.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (10 preceding siblings ...)
  2022-02-09 14:10 ` cvs-commit at gcc dot gnu.org
@ 2022-02-09 14:11 ` amacleod at redhat dot com
  2023-04-09  3:59 ` christian.prochaska@genode-labs.com
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: amacleod at redhat dot com @ 2022-02-09 14:11 UTC (permalink / raw)
  To: gcc-bugs

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

Andrew Macleod <amacleod at redhat dot com> changed:

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

--- Comment #12 from Andrew Macleod <amacleod at redhat dot com> ---
fixed.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (11 preceding siblings ...)
  2022-02-09 14:11 ` amacleod at redhat dot com
@ 2023-04-09  3:59 ` christian.prochaska@genode-labs.com
  2023-04-09  4:12 ` pinskia at gcc dot gnu.org
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: christian.prochaska@genode-labs.com @ 2023-04-09  3:59 UTC (permalink / raw)
  To: gcc-bugs

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

Christian Prochaska <christian.prochaska@genode-labs.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |christian.prochaska@genode-
                   |                            |labs.com

--- Comment #13 from Christian Prochaska <christian.prochaska@genode-labs.com> ---
I found the "Register non-null side effects properly." commit with git bisect
while debugging a page fault in the Genode OS framework built with GCC 12.2.0.
It turned out that a null pointer check which was present before this commit is
now not present anymore. The C++ code with the null pointer check can be found
on GitHub:

https://github.com/genodelabs/genode/blob/a84af9a9606450471b8038a35f9b55057efa0850/repos/base-nova/src/lib/base/ipc.cc#L71

This is the implementation of the 'Thread::myself()' function which returns a
null pointer in some conditions:

https://github.com/genodelabs/genode/blob/a84af9a9606450471b8038a35f9b55057efa0850/repos/base/src/lib/base/thread_myself.cc#L22

I compared the disassembled code from objdump and this part is missing when the
commit is applied:

Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
Genode::Msgbuf_base&, unsigned long):
/.../repos/base-nova/src/lib/base/ipc.cc:71
    addr_t const manual_rcv_sel = myself ?
myself->native_thread().client_rcv_sel
   85f78:   48 83 bd 50 ff ff ff    cmpq   $0x0,-0xb0(%rbp)
   85f7f:   00
   85f80:   48 c7 c3 ff ff ff ff    mov    $0xffffffffffffffff,%rbx
   85f87:   74 1d                   je     85fa6
<Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
Genode::Msgbuf_base&, unsigned long)
/.../repos/base-nova/src/lib/base/ipc.cc:71 (discriminator 1)

Now I'm not sure if the problem is in the Genode code or in GCC. Any ideas?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (12 preceding siblings ...)
  2023-04-09  3:59 ` christian.prochaska@genode-labs.com
@ 2023-04-09  4:12 ` pinskia at gcc dot gnu.org
  2023-04-09  4:12 ` pinskia at gcc dot gnu.org
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-04-09  4:12 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Christian Prochaska from comment #13)
> I found the "Register non-null side effects properly." commit with git
> bisect while debugging a page fault in the Genode OS framework built with
> GCC 12.2.0. It turned out that a null pointer check which was present before
> this commit is now not present anymore. The C++ code with the null pointer
> check can be found on GitHub:
> 
> https://github.com/genodelabs/genode/blob/
> a84af9a9606450471b8038a35f9b55057efa0850/repos/base-nova/src/lib/base/ipc.
> cc#L71
> 
> This is the implementation of the 'Thread::myself()' function which returns
> a null pointer in some conditions:
> 
> https://github.com/genodelabs/genode/blob/
> a84af9a9606450471b8038a35f9b55057efa0850/repos/base/src/lib/base/
> thread_myself.cc#L22
> 
> I compared the disassembled code from objdump and this part is missing when
> the commit is applied:
> 
> Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
> Genode::Msgbuf_base&, unsigned long):
> /.../repos/base-nova/src/lib/base/ipc.cc:71
>     addr_t const manual_rcv_sel = myself ?
> myself->native_thread().client_rcv_sel
>    85f78:   48 83 bd 50 ff ff ff    cmpq   $0x0,-0xb0(%rbp)
>    85f7f:   00
>    85f80:   48 c7 c3 ff ff ff ff    mov    $0xffffffffffffffff,%rbx
>    85f87:   74 1d                   je     85fa6
> <Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
> Genode::Msgbuf_base&, unsigned long)
> /.../repos/base-nova/src/lib/base/ipc.cc:71 (discriminator 1)
> 
> Now I'm not sure if the problem is in the Genode code or in GCC. Any ideas?

There was a deferencing of myself before:
Nova::Utcb &utcb = *(Nova::Utcb *)myself->utcb();

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (13 preceding siblings ...)
  2023-04-09  4:12 ` pinskia at gcc dot gnu.org
@ 2023-04-09  4:12 ` pinskia at gcc dot gnu.org
  2023-04-09  7:07 ` christian.prochaska@genode-labs.com
  2023-04-09  7:43 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-04-09  4:12 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #15 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #14)
> (In reply to Christian Prochaska from comment #13)
> > I found the "Register non-null side effects properly." commit with git
> > bisect while debugging a page fault in the Genode OS framework built with
> > GCC 12.2.0. It turned out that a null pointer check which was present before
> > this commit is now not present anymore. The C++ code with the null pointer
> > check can be found on GitHub:
> > 
> > https://github.com/genodelabs/genode/blob/
> > a84af9a9606450471b8038a35f9b55057efa0850/repos/base-nova/src/lib/base/ipc.
> > cc#L71
> > 
> > This is the implementation of the 'Thread::myself()' function which returns
> > a null pointer in some conditions:
> > 
> > https://github.com/genodelabs/genode/blob/
> > a84af9a9606450471b8038a35f9b55057efa0850/repos/base/src/lib/base/
> > thread_myself.cc#L22
> > 
> > I compared the disassembled code from objdump and this part is missing when
> > the commit is applied:
> > 
> > Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
> > Genode::Msgbuf_base&, unsigned long):
> > /.../repos/base-nova/src/lib/base/ipc.cc:71
> >     addr_t const manual_rcv_sel = myself ?
> > myself->native_thread().client_rcv_sel
> >    85f78:   48 83 bd 50 ff ff ff    cmpq   $0x0,-0xb0(%rbp)
> >    85f7f:   00
> >    85f80:   48 c7 c3 ff ff ff ff    mov    $0xffffffffffffffff,%rbx
> >    85f87:   74 1d                   je     85fa6
> > <Genode::ipc_call(Genode::Native_capability, Genode::Msgbuf_base&,
> > Genode::Msgbuf_base&, unsigned long)
> > /.../repos/base-nova/src/lib/base/ipc.cc:71 (discriminator 1)
> > 
> > Now I'm not sure if the problem is in the Genode code or in GCC. Any ideas?
> 
> There was a deferencing of myself before:
> Nova::Utcb &utcb = *(Nova::Utcb *)myself->utcb();

Line 59 so it is definitely not a bug in gcc.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (14 preceding siblings ...)
  2023-04-09  4:12 ` pinskia at gcc dot gnu.org
@ 2023-04-09  7:07 ` christian.prochaska@genode-labs.com
  2023-04-09  7:43 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: christian.prochaska@genode-labs.com @ 2023-04-09  7:07 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #16 from Christian Prochaska <christian.prochaska@genode-labs.com> ---
(In reply to Andrew Pinski from comment #14)
> 
> There was a deferencing of myself before:
> Nova::Utcb &utcb = *(Nova::Utcb *)myself->utcb();

I see. The 'Thread::utcb()' function handles the null pointer case internally
with a 'this == 0' check and a local '-fno-delete-null-pointer-checks'
attribute:

https://github.com/genodelabs/genode/blob/a84af9a9606450471b8038a35f9b55057efa0850/repos/base-nova/src/lib/base/stack.cc#L110

So, the elimination of the 'myself' null pointer check is basically a result of
undefined behavior with the 'Thread::utcb()' function?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
  2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
                   ` (15 preceding siblings ...)
  2023-04-09  7:07 ` christian.prochaska@genode-labs.com
@ 2023-04-09  7:43 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-04-09  7:43 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #17 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Christian Prochaska from comment #16)
> (In reply to Andrew Pinski from comment #14)
> > 
> > There was a deferencing of myself before:
> > Nova::Utcb &utcb = *(Nova::Utcb *)myself->utcb();
> 
> I see. The 'Thread::utcb()' function handles the null pointer case
> internally with a 'this == 0' check and a local
> '-fno-delete-null-pointer-checks' attribute:
> 
> https://github.com/genodelabs/genode/blob/
> a84af9a9606450471b8038a35f9b55057efa0850/repos/base-nova/src/lib/base/stack.
> cc#L110
> 
> So, the elimination of the 'myself' null pointer check is basically a result
> of undefined behavior with the 'Thread::utcb()' function?

That only handles one side of where it is undefined. Newer GCC uses it as being
undefined even on the calling side. So you need to use
-fno-delete-null-pointer-checks really on the command line or better yet fixes
all of the places which use ->utcb() .

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-04-09  7:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-30 10:09 [Bug c/104288] New: Null pointer check invalidly deleted nrk at disroot dot org
2022-01-30 10:26 ` [Bug middle-end/104288] [11/12 Regression] " pinskia at gcc dot gnu.org
2022-01-30 10:29 ` [Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative pinskia at gcc dot gnu.org
2022-01-30 11:03 ` jakub at gcc dot gnu.org
2022-01-31  8:21 ` rguenth at gcc dot gnu.org
2022-01-31 14:53 ` amacleod at redhat dot com
2022-01-31 15:34 ` jakub at gcc dot gnu.org
2022-01-31 22:39 ` amacleod at redhat dot com
2022-02-01  8:04 ` rguenther at suse dot de
2022-02-02 21:52 ` amacleod at redhat dot com
2022-02-08 15:03 ` cvs-commit at gcc dot gnu.org
2022-02-09 14:10 ` cvs-commit at gcc dot gnu.org
2022-02-09 14:11 ` amacleod at redhat dot com
2023-04-09  3:59 ` christian.prochaska@genode-labs.com
2023-04-09  4:12 ` pinskia at gcc dot gnu.org
2023-04-09  4:12 ` pinskia at gcc dot gnu.org
2023-04-09  7:07 ` christian.prochaska@genode-labs.com
2023-04-09  7:43 ` pinskia at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).