[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #17 from Andrew Pinski --- (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 = *(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() .
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #16 from Christian Prochaska --- (In reply to Andrew Pinski from comment #14) > > There was a deferencing of myself before: > Nova::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?
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #15 from Andrew Pinski --- (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 ffcmpq $0x0,-0xb0(%rbp) > >85f7f: 00 > >85f80: 48 c7 c3 ff ff ff ffmov$0x,%rbx > >85f87: 74 1d je 85fa6 > > > 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 = *(Nova::Utcb *)myself->utcb(); Line 59 so it is definitely not a bug in gcc.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #14 from Andrew Pinski --- (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 ffcmpq $0x0,-0xb0(%rbp) >85f7f: 00 >85f80: 48 c7 c3 ff ff ff ffmov$0x,%rbx >85f87: 74 1d je 85fa6 > 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 = *(Nova::Utcb *)myself->utcb();
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 Christian Prochaska changed: What|Removed |Added CC||christian.prochaska@genode- ||labs.com --- Comment #13 from Christian Prochaska --- 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 ffcmpq $0x0,-0xb0(%rbp) 85f7f: 00 85f80: 48 c7 c3 ff ff ff ffmov$0x,%rbx 85f87: 74 1d je 85fa6
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 Andrew Macleod changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #12 from Andrew Macleod --- fixed.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #11 from CVS Commits --- The master branch has been updated by Andrew Macleod : https://gcc.gnu.org/g:c6bb1db76b3ac127aff7dacf391fc1798a94bb7d commit r12-7128-gc6bb1db76b3ac127aff7dacf391fc1798a94bb7d Author: Andrew MacLeod 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #10 from CVS Commits --- The releases/gcc-11 branch has been updated by Andrew Macleod : https://gcc.gnu.org/g:ed35d4205e8c139d27d3d47c528aaa9f82f0ac1b commit r11-9543-ged35d4205e8c139d27d3d47c528aaa9f82f0ac1b Author: Andrew MacLeod 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #9 from Andrew Macleod --- 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #8 from 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 --- > 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? ;)
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288
--- Comment #7 from Andrew Macleod ---
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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #6 from Jakub Jelinek --- 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 --- Comment #5 from Andrew Macleod --- 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 Richard Biener changed: What|Removed |Added Priority|P3 |P1 --- Comment #4 from Richard Biener --- 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.
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 Jakub Jelinek 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 --- Started with r11-3685-gfcae5121154d1c3382b056bcc2c563cedac28e74
[Bug tree-optimization/104288] [11/12 Regression] EVRP null pointer check removal for strcmp (and maybe others) is not flow senative
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104288 Andrew Pinski changed: What|Removed |Added Keywords||needs-bisection Component|middle-end |tree-optimization --- Comment #2 from Andrew Pinski --- 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.
