[CHKP] Attempt to fix PR79765 (multiversioning and instrumentation)

[Alexander Ivchenko <aivchenk at gmail dot com>]

Hi,

I'm trying to fix the problem with function multiversioning and MPX
instrumentation (PR79765) and I face several issues. I would
appreciate your advice:


The first problem that arises is that multiversioning tries to make
versions out of thunks, which do not have bodies. This is fixed with
this patch:

diff --git a/gcc/ipa-chkp.c b/gcc/ipa-chkp.c
index c7fc3a0..5de11b3 100644
--- a/gcc/ipa-chkp.c
+++ b/gcc/ipa-chkp.c
@@ -718,6 +718,9 @@ chkp_produce_thunks (bool early)
                             0, CGRAPH_FREQ_BASE);
          node->create_reference (node->instrumented_version,
                               IPA_REF_CHKP, NULL);
+         DECL_ATTRIBUTES (node->decl)
+           = remove_attribute ("target_clones",
+                               DECL_ATTRIBUTES (node->decl));
          /* Thunk shouldn't be a cdtor.  */
          DECL_STATIC_CONSTRUCTOR (node->decl) = 0;
          DECL_STATIC_DESTRUCTOR (node->decl) = 0;

However, that is not all that is needed to be fixed. Consider case:

 __attribute__((target_clones("arch=core-avx2", "default")))
int test_fun()
 {
  return test_fun();
 }

Now, in multiple_target.c: create_dispatcher_calls  we are replacing
the call to test_fun with a call to the ifunc resolvler:

(rr) p debug_function (node->decl, TDF_VOPS)
__attribute__((target ("default"), target_clones ("arch=core-avx2",
"default")))
test_fun ()
{
  <bb 2> [100.00%]:
  _2 = test_fun ();
  # VUSE <.MEM_1(D)>
  return _2;
}

(rr) p e
                                                        $103 =
(cgraph_edge *) 0x7f658d121340
                      (rr) p e->callee

 $104 = <cgraph_node* 0x7f658d257000 "test_fun">
                        (rr) p e->caller

     $105 = <cgraph_node* 0x7f658d257000 "test_fun">
(rr) p inode
                                                    $106 =
<cgraph_node* 0x7f658d2572e0 "test_fun.ifunc">

We do..

     e->redirect_callee (inode);

e->redirect_call_stmt_to_callee ();

And after we have:

(rr) p debug_function (node->decl, TDF_VOPS)

__attribute__((target ("default"), target_clones ("arch=core-avx2",
"default")))       test_fun ()

 {
                                                           <bb 2>
[100.00%]:
  # .MEM = VDEF <.MEM>
                                       _2 = test_fun.ifunc ();
  # VUSE <.MEM_1(D)>
                                         return _2;

                      }

For MPX-instrumented code the outcome of the same procedure is different:

(rr) p e->caller
$5 = <cgraph_node* 0x7f3e6ad8b170 "test_fun.chkp">
(rr) p e->callee
$6 = <cgraph_node* 0x7f3e6ad8b170 "test_fun.chkp">
(rr) p inode
$7 = <cgraph_node* 0x7f3e6ad8b450 "test_fun.chkp.ifunc">

(rr) p debug_function (node->decl, TDF_VOPS)
__attribute__((target ("default"), chkp instrumented, target_clones
("arch=core-avx2", "default")))
test_fun.chkp ()
{
  <bb 2> [100.00%]:
  _2 = test_fun.chkp ();
  # VUSE <.MEM_1(D)>
  return _2;
}

-- We don't have   # .MEM = VDEF <.MEM>  part and hence, we hit
internal compiler error in verify_ssa, which fails. The reason for
that is that in  redirect_call_stmt_to_callee for instrumented code
the path is different and update_stmt_fn is not called:

if (e->callee->clone.combined_args_to_skip
     || skip_bounds)
{..}
else
{
..
update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
}

Calling this function after if-then-else fixes the issue:

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index e505b10..3dde20d 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1527,8 +1527,8 @@ cgraph_edge::redirect_call_stmt_to_callee (void)
     {
       new_stmt = e->call_stmt;
       gimple_call_set_fndecl (new_stmt, e->callee->decl);
-      update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
     }
+  update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);

   /* If changing the call to __cxa_pure_virtual or similar noreturn function,
      adjust gimple_call_fntype too.  */

This call makes sense to me, but is it a correct thing to do?

And finally, the third point. In the same if-then-else, in the "if"
part there is a call to "gsi_replace", which uses "cfun", but
targetclone is ipa pass and hence has cfun defined to NULL. So, is it
a bug at all to use "gsi_replace" in ipa pass?

thanks in advance,
Alexander