[Bug c/106422] New: ice in duplicate_block, at cfghooks.cc:1115

[Bug c/106422] New: ice in duplicate_block, at cfghooks.cc:1115

[dcb314 at hotmail dot com]

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

            Bug ID: 106422
           Summary: ice in duplicate_block, at cfghooks.cc:1115
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

Created attachment 53340
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53340&action=edit
C source code

For the attached C code, recent gcc does this:

$ ../results/bin/gcc -c -O2 bug830.c
during GIMPLE pass: dom
semanage_store.c: In function ‘semanage_exec_prog’:
semanage_store.c:1433:12: internal compiler error: in duplicate_block, at
cfghooks.cc:1115
0x836152 duplicate_block(basic_block_def*, edge_def*, basic_block_def*,
copy_bb_data*)
        ../../trunk.git/gcc/cfghooks.cc:1115
0x100fe0b create_block_for_threading(basic_block_def*, redirection_data*,
unsigned int, bitmap_head **)

The problem first seems to occur sometime between git hash e9ee752bbe2cc563
and 78d5e125c008d87c, a distance of 27 commits.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0
            Version|12.0                        |13.0
            Summary|ice in duplicate_block, at  |[13 Regression] ice in
                   |cfghooks.cc:1115            |duplicate_block, at
                   |                            |cfghooks.cc:1115

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Someone is not checking can_duplicate_block_p before calling duplicate_block.


Most likely exposed by r13-1753-g26cea5f108e0fa which rejects blocks which have
a return twice function at the end.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[dcb314 at hotmail dot com]

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

--- Comment #2 from David Binderman <dcb314 at hotmail dot com> ---
Reduced source code is

int semanage_exec_prog_sh_0;
void vfork() __attribute__((__leaf__));
void semanage_reload_policy() {
  char argv;
  if (argv)
    semanage_exec_prog_sh_0 = __FUNCTION__;
  vfork();
  for (; argv;)
    free(argv);
}

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

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

--- Comment #3 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Thanks for the report and reduced testcase. There are at least two issues here:

1. Somehow GCC thinks that attribute-leaf functions cannot resume an earlier
returns_twice function. This is a wrong belief, causing GCC to omit
ABNORMAL_DISPATCHER and associated edges for even simple testcases like

#include <unistd.h>
#include <signal.h>

void f()
{
    if (!vfork())
        raise(SIGKILL);
}

(when a block with a returns_twice function does not have abnormal edges, we
could say that it's fine to duplicate because all other calls that might have
resumed its execution have been eliminated, but with the above issue corrected
there will always be an edge from the function itself, e.g. a later vfork can
cause an earlier vfork at the same call site to resume)

2. It seems backward jump threading uses can_copy_bbs_p and copy_bbs, which
disallows duplicating a returns_twice call, while forward jump threading calls
duplicate_block without checking apparently?

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

--- Comment #4 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Regarding point 1 above, I should mention that Glibc headers mark both 'vfork'
and 'raise' as leaf.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-07-25
                 CC|                            |marxin at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
> 
> Most likely exposed by r13-1753-g26cea5f108e0fa which rejects blocks which
> have a return twice function at the end.

Yes, started with this revision.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think vfork is OK as leaf but raise is not since it can invoke a signal
handler.  Citing docs:

"Note that leaf functions might indirectly run a signal handler defined
in the current compilation unit that uses static variables.  Similarly,
when lazy symbol resolution is in effect, leaf functions might invoke
indirect functions whose resolver function or implementation function is
defined in the current compilation unit and uses static variables.  There
is no standard-compliant way to write such a signal handler, resolver
function, or implementation function, and the best that you can do is to
remove the @code{leaf} attribute or mark all such static variables
@code{volatile}."

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

--- Comment #7 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
I think item 2 from comment #3 (jump threading) still needs to be solved
independently of what is decided about item 1 (leaf functions resuming earlier
returns_twice call).

---

The problem with 'leaf' is that Glibc is taking the longjmp stipulation from
the docs ("a leaf function is not allowed to [...] longjmp into the unit")
literally:

/* All functions, except those with callbacks or those that
   synchronize memory, are leaf functions.  */
# if __GNUC_PREREQ (4, 6) && !defined _LIBC
#  define __LEAF , __leaf__
#  define __LEAF_ATTR __attribute__ ((__leaf__))
# else
#  define __LEAF
#  define __LEAF_ATTR
# endif


and marks many, many functions including 'execve' leaf, while 'execve'
obviously resumes an earlier vfork (if successful).

---

In the original testcase we have 'free' which arguably should not resume any
returns_twice function, so GIMPLE representation is correct and jump threading
needs a fix (or can_duplicate_block_p if we decide it's too conservative and
ought to check presence of abnormal edges). Please let me know if I should
split off the problem with leaf functions in a separate bug.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

--- Comment #8 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
I mean the minimized testcase, the original attachment does execve/_exit after
vfork.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Splitting off the 'leaf' issue would be useful.  And yes, forward threading
would need to be fixed.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

--- Comment #10 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
The leaf issue is now PR 106437.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

--- Comment #11 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
A cleaner testcase for jump threading (still ICEs despite presence of
ABNORMAL_DISPATCHER):

void vfork() __attribute__((__leaf__));
void semanage_reload_policy(char *arg, void cb(void)) {
  if (!arg) {
    cb();
    return;
  }
  vfork();
  if (arg)
    __builtin_free(arg);
}

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

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

--- Comment #12 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Aldy, could you please have a look? Forward jump threading calls
duplicate_block without checking can_duplicate_block_p — was that supposed to
work?

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

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

--- Comment #13 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #12)
> Aldy, could you please have a look? Forward jump threading calls
> duplicate_block without checking can_duplicate_block_p — was that supposed
> to work?

[Big caveat here: This is beyond my expertise as I've been careful not to touch
anything dealing with the block copiers in either the backward or the forward
threaders.]

There are two block copiers in tree-ssa-threadupdate.cc
(*_jt_path_registry::update_cfg), one for each of the threader implementations.

It seems the backward one is doing the right thing, because it calls
duplicate_thread_path() which calls can_duplicate_block_p downstream.

For that matter, the ethread pass (backward threader) finds this exact path,
registers it, but then silently drops it because of the checks in
duplicate_thread_path():

  [1] Registering jump thread: (2, 5) incoming edge;  (5, 7) nocopy; 

The forward threader seems to have more ad-hoc ways of determining
threadability, like potentially_threadable_block(), but AFAICT it never checks
can_duplicate_block_p.  I suppose one could just add a can_duplicate_block_p
check to potentially_threadable_block.

But a better approach would be to drop these problematic paths at registration.
 We already have a gate for precisely these things (cancel_invalid_paths), and
it would be shared between both threaders.  Furthermore, it would add a nice
message in the log as to why the path was dropped.

I don't have the cycles to fix this, but I can provide of a proof of concept
that may be enough.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[aldyh at gcc dot gnu.org]

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

--- Comment #14 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Created attachment 53373
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53373&action=edit
Untested patch

The important part is the change to tree-ssa-threadupdate.cc.  The rest is just
making sure the backward threader doesn't print out "SUCCESS" when a path was
actually dropped.

With this patch I see all threaders find and successfully drop this path (with
no ICE ;-)):

 grep 'Found block' a.c.*
a.c.036t.ethread:Found block that cannot be duplicated:   Cancelling jump
thread: (2, 5) incoming edge;  (5, 7) nocopy;
a.c.114t.threadfull1:Found block that cannot be duplicated:   Cancelling jump
thread: (2, 4) incoming edge;  (4, 6) nocopy;
a.c.114t.threadfull1:Found block that cannot be duplicated:   Cancelling jump
thread: (0, 2) incoming edge;  (2, 4) normal (4, 6) nocopy;
a.c.129t.thread1:Found block that cannot be duplicated:   Cancelling jump
thread: (2, 4) incoming edge;  (4, 6) nocopy;
a.c.129t.thread1:Found block that cannot be duplicated:   Cancelling jump
thread: (0, 2) incoming edge;  (2, 4) normal (4, 6) nocopy;
a.c.130t.dom2:Found block that cannot be duplicated:   Cancelling jump thread:
(2, 4) incoming edge;  (4, 6) normal;
a.c.195t.thread2:Found block that cannot be duplicated:   Cancelling jump
thread: (2, 5) incoming edge;  (5, 7) nocopy;
a.c.195t.thread2:Found block that cannot be duplicated:   Cancelling jump
thread: (0, 2) incoming edge;  (2, 5) normal (5, 7) nocopy;
a.c.196t.dom3:Found block that cannot be duplicated:   Cancelling jump thread:
(2, 5) incoming edge;  (5, 7) normal;
a.c.198t.threadfull2:Found block that cannot be duplicated:   Cancelling jump
thread: (2, 5) incoming edge;  (5, 7) nocopy;
a.c.198t.threadfull2:Found block that cannot be duplicated:   Cancelling jump
thread: (0, 2) incoming edge;  (2, 5) normal (5, 7) nocopy;

Perhaps someone can take this over, and maybe clean up the duplicity in
cancel_invalid_paths.  We should probably call can_duplicate_block_p for the
SRC and then the final destination block outside the loop.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:4894ba078692a780a461d2f358b5dfaa25719859

commit r13-1889-g4894ba078692a780a461d2f358b5dfaa25719859
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Jul 29 08:24:52 2022 +0200

    tree-optimization/106422 - verify block copying in forward threading

    The forward threader failed to check whether it can actually duplicate
    blocks.  The following adds this in a similar place the backwards threader
    performs this check.

            PR tree-optimization/106422
            * tree-ssa-threadupdate.cc (fwd_jt_path_registry::update_cfg):
            Check whether we can copy thread blocks and cancel the thread if
not.

            * gcc.dg/torture/pr106422.c: New testcase.

[Bug tree-optimization/106422] [13 Regression] ice in duplicate_block, at cfghooks.cc:1115

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.