From https://stackoverflow.com/questions/56655137/undefined-behaviour-or-gcc-optimization-bug int puts(const char*); void free(void*); void* malloc(unsigned long); #define NULL 0 struct Node { struct Node* child; }; void walk(struct Node* module, int cleanup) { if (module == NULL) { return; } if (!cleanup) { puts("No cleanup"); } walk(module->child, cleanup); if (cleanup) { free(module); } } int main() { struct Node* node = malloc(sizeof(struct Node)); node->child = NULL; walk(node, 1); } This crashes when compiled with -O1 -foptimize-sibling-calls -ftree-vrp since r270574 PR tree-optimization/90037 * Makefile.in (OBJS): Remove tree-ssa-phionlycprop.c * passes.def: Replace all instance of phi-only cprop with the lattice propagator. Move propagation pass from after erroneous path isolation to before erroneous path isolation. * tree-ssa-phionlycprop.c: Remove. * gcc.dg/tree-ssa/20030710-1.c: Update dump file to scan. * gcc.dg/isolate-2.c: Likewise. * gcc.dg/isolate-4.c: Likewise. * gcc.dg/pr19431.c: Accept either ordering of PHI args. * gcc.dg/pr90037.c: New test.
Yea, it looks like we don't have NULL in the points-to set for "module", as a result we think the "module" parameter can never be NULL and the check gets eliminated. I don't think the referenced change introduced the issue, but certainly could have taken a latent issue and made it active. Anyway, poking at it now.
OK. I see what's happening here. At the end of SRA we have the following key block: ;; basic block 7, loop depth 0 ;; pred: 4 ;; 3 # module_12 = PHI <_14(4), module_16(D)(3)> # cleanup_19 = PHI <cleanup_10(4), cleanup_13(D)(3)> _17 = module_12->child; walk (_17, cleanup_19); free (module_12); goto <bb 6>; [100.00%] ;; succ: 6 DOM fires up and is going to correctly determine the edge 4->7 is not executable. DOM (via evrp analysis) is also going to know that module_16 _in this context_ must have a non-null value. The combination of those two factors will mean that module_12 always has a non-null value. DOM completes and we're left with that block looking like: ;; basic block 6, loop depth 0 ;; pred: 3 # module_12 = PHI <module_16(D)(3)> # cleanup_19 = PHI <cleanup_13(D)(3)> _17 = module_16(D)->child; walk (_17, cleanup_13(D)); free (module_16(D)); goto <bb 5>; [100.00%] ;; succ: 5 Now copyprop comes along. The degenerate PHI is just a copy so it's going to want to propagate it away. module_12 is a copy of module_16. We have no PTA information for module_16, but we do have PTA information for module_12. The copy propagator will copy the PTA information from module_12 to module_16, including the nonnull state (which remember was _context dependent_). So PTA now says that module_16 must be non-null and a subsequent pass deletes this test at the start of the function: ;; basic block 6, loop depth 0 ;; pred: 3 # module_12 = PHI <module_16(D)(3)> # cleanup_19 = PHI <cleanup_13(D)(3)> _17 = module_16(D)->child; walk (_17, cleanup_13(D)); free (module_16(D)); goto <bb 5>; [100.00%] ;; succ: 5 And we lose, badly. The copy propagator is already aware that it has to be careful with context sensitive information getting copied over like this (alignment info), but it didn't have a case for non-null status which is also context sensitive. A fix is in testing now. And all that explains why the referenced change is the trigger. Prior to that change we used a specialized copy propagator which didn't try to copy the PTA information. After the change we use the standard copy propagator which copies the PTA information.
Hmm, non-nullness isn't properly tracked by PTA but I remember somebody added computing pt.null from VRP. But before that PTA info was flow-insensitive and thus we didn't have to be careful with propagating it. After this change it _is_ now flow-sensitive and we at least have to reset null-ness info (set pi->pt.null to 1). There's helpers to reset flow-sensitive info on SSA names/stmts. And the issue is latent since the introduction of {set,get}_ptr_nonnull.
Precisely. I didn't see a helper to set pt.null to 1/true, but it's trivial enough to add one.
Hi, I am the guy who isolated the issue for c++, behind the question. If this isn't too much, could anyone explain why does changing type of cleanup from int to bool solves the issue? Everything else I can understand, because those are changes in observable behaviour. Do the optimizers treat boolean and integers differently, even in bool contexts? This is intriguing. I know this is tangential, but it really bugs me and I am not sure if I can study the sourcecode of gcc optimizer in reasonable time.
In response to c#5. The difference is when you use a bool for cleanup, it has to be promoted at the recursive call call to walk(). That's just enough to change the decision between using tail call (which still looks like a call) and tail recursion elimination which turns the recursive call into a simple backwards jump. In the former case (bool) we never have the copy propagation opportunity that triggers the problem. In the latter case, the backwards jump creates a loop and we determine it's profitable to copy the loop header which creates the PHI node with the dead incoming edge that triggers the copy propagation that causes the problems.
Author: law Date: Fri Jun 21 16:36:00 2019 New Revision: 272555 URL: https://gcc.gnu.org/viewcvs?rev=272555&root=gcc&view=rev Log: PR tree-optimization/90949 * tree-ssa-copy.c (fini_copy_prop): Use reset_flow_sensitive_info. * tree-ssanames.c (reset_flow_sensitive_info): Reset non-null state. * gcc.c-torture/execute/pr90949.c: New test. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr90949.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-ssa-copy.c trunk/gcc/tree-ssanames.c
Fixed on trunk so far. I'll backport to gcc-9 after a few days. I'll also evaluate if we need this in gcc-8 as well (I suspect yes, even if the bug is currently latent there).
On Fri, 21 Jun 2019, law at redhat dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90949 > > Jeffrey A. Law <law at redhat dot com> changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > Summary|[9/10 Regression] null |[9 Regression] null pointer > |pointer check removed |check removed > > --- Comment #8 from Jeffrey A. Law <law at redhat dot com> --- > Fixed on trunk so far. I'll backport to gcc-9 after a few days. I'll also > evaluate if we need this in gcc-8 as well (I suspect yes, even if the bug is > currently latent there). I'd put it on all active branches where the non-nullness is tracked.
Author: law Date: Fri Jun 28 20:21:05 2019 New Revision: 272793 URL: https://gcc.gnu.org/viewcvs?rev=272793&root=gcc&view=rev Log: PR tree-optimization/90949 * tree-ssa-copy.c (fini_copy_prop): Use reset_flow_sensitive_info. * tree-ssanames.c (reset_flow_sensitive_info): Reset non-null state. * gcc.c-torture/execute/pr90949.c: New test. Added: branches/gcc-9-branch/gcc/testsuite/gcc.c-torture/execute/pr90949.c Modified: branches/gcc-9-branch/gcc/ChangeLog branches/gcc-9-branch/gcc/testsuite/ChangeLog branches/gcc-9-branch/gcc/tree-ssa-copy.c branches/gcc-9-branch/gcc/tree-ssanames.c
Author: law Date: Fri Jun 28 20:58:42 2019 New Revision: 272797 URL: https://gcc.gnu.org/viewcvs?rev=272797&root=gcc&view=rev Log: PR tree-optimization/90949 * tree-ssa-copy.c (fini_copy_prop): Use reset_flow_sensitive_info. * tree-ssanames.c (reset_flow_sensitive_info): Reset non-null state. * gcc.c-torture/execute/pr90949.c: New test. Added: branches/gcc-8-branch/gcc/testsuite/gcc.c-torture/execute/pr90949.c Modified: branches/gcc-8-branch/gcc/ChangeLog branches/gcc-8-branch/gcc/testsuite/ChangeLog branches/gcc-8-branch/gcc/tree-ssa-copy.c branches/gcc-8-branch/gcc/tree-ssanames.c
Author: law Date: Fri Jun 28 21:01:56 2019 New Revision: 272798 URL: https://gcc.gnu.org/viewcvs?rev=272798&root=gcc&view=rev Log: PR tree-optimization/90949 * tree-ssa-copy.c (fini_copy_prop): Use reset_flow_sensitive_info. * tree-ssanames.c (reset_flow_sensitive_info): Reset non-null state. * gcc.c-torture/execute/pr90949.c: New test. Added: branches/gcc-7-branch/gcc/testsuite/gcc.c-torture/execute/pr90949.c Modified: branches/gcc-7-branch/gcc/ChangeLog branches/gcc-7-branch/gcc/testsuite/ChangeLog branches/gcc-7-branch/gcc/tree-ssa-copy.c branches/gcc-7-branch/gcc/tree-ssanames.c
Cherry-picked to gcc-7, gcc-8 and gcc-9. It was latent on gcc-7 and gcc-8, but fixing still seemed like the right choice.
Since 10.1, gcc does crazy things with bloaty codegen for this case https://godbolt.org/z/Qb3yHZ
(In reply to Dávid Bolvanský from comment #14) > Since 10.1, gcc does crazy things with bloaty codegen for this case > > https://godbolt.org/z/Qb3yHZ It is called recursive inlining. Not really bloated.
For -O3 it is okay, but for -O2 this is questionable
On Wed, 3 Jun 2020, david.bolvansky at gmail dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90949 > > --- Comment #16 from Dávid Bolvanský <david.bolvansky at gmail dot com> --- > For -O3 it is okay, but for -O2 this is questionable Can you open a new bugreport please?
Yes, PR95492