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).