i#4940: Fix the wrong aflags estimation in drreg (#4941)

i#4940: Fix the wrong aflags estimation in drreg (#4941)

Fix the wrong aflags estimation in drreg to make sure the functionality of drreg_reserve_aflags/drreg_unreserve_aflags and drreg_aflags_are_dead for conditional instructions like cmovcc and sbb. Make drx_aflags_are_dead as a wrapper of drreg_aflags_are_dead.

Fixes: #4940

ext/drreg/drreg.c

@@ -793,6 +793,8 @@ drreg_forward_analysis(void *drcontext, instr_t *start)
         aflags_new = instr_get_arith_flags(inst, DR_QUERY_INCLUDE_COND_SRCS);
         /* reading and writing counts only as reading */
         aflags_new &= (~(EFLAGS_READ_TO_WRITE(aflags_new)));
+        /* writing doesn't count if already read */
+        aflags_new &= (~(EFLAGS_READ_TO_WRITE(aflags_cur)));
         /* reading doesn't count if already written */
         aflags_new &= (~(EFLAGS_WRITE_TO_READ(aflags_cur)));
         aflags_cur |= aflags_new;

ext/drx/drx.c

@@ -219,32 +219,11 @@ DR_EXPORT
 bool
 drx_aflags_are_dead(instr_t *where)
 {
-    instr_t *instr;
-    uint flags;
-    for (instr = where; instr != NULL; instr = instr_get_next(instr)) {
-        /* we treat syscall/interrupt as aflags read */
-        if (instr_is_syscall(instr) || instr_is_interrupt(instr))
-            return false;
-        flags = instr_get_arith_flags(instr, DR_QUERY_DEFAULT);
-        if (TESTANY(EFLAGS_READ_ARITH, flags))
-            return false;
-        if (TESTALL(EFLAGS_WRITE_ARITH, flags))
-            return true;
-        if (instr_is_cti(instr)) {
-            if (instr_is_app(instr) &&
-                (instr_is_ubr(instr) || instr_is_call_direct(instr))) {
-                instr_t *next = instr_get_next(instr);
-                opnd_t tgt = instr_get_target(instr);
-                /* continue on elision */
-                if (next != NULL && instr_is_app(next) && opnd_is_pc(tgt) &&
-                    opnd_get_pc(tgt) == instr_get_app_pc(next))
-                    continue;
-            }
-            /* unknown target, assume aflags is live */
-            return false;
-        }
-    }
-    return false;
+    bool dead = false;
+    IF_DEBUG(drreg_status_t res =)
+    drreg_are_aflags_dead(dr_get_current_drcontext(), where, &dead);
+    ASSERT(res == DRREG_SUCCESS, "drreg_are_aflags_dead failed!");
+    return dead;
 }
 
 /***************************************************************************

suite/tests/CMakeLists.txt

@@ -2562,6 +2562,7 @@ endif (UNIX)
 tobuild_ci(client.drreg-test client-interface/drreg-test.c "" "" "")
 use_DynamoRIO_extension(client.drreg-test.dll drmgr)
 use_DynamoRIO_extension(client.drreg-test.dll drreg)
+use_DynamoRIO_extension(client.drreg-test.dll drx)
 target_include_directories(client.drreg-test PRIVATE
   ${CMAKE_CURRENT_SOURCE_DIR}/client-interface)
 

suite/tests/client-interface/drreg-test.dll.c

@@ -35,6 +35,7 @@
 #include "dr_api.h"
 #include "drmgr.h"
 #include "drreg.h"
+#include "drx.h"
 #include "client_tools.h"
 #include "drreg-test-shared.h"
 #include <string.h> /* memset */
@@ -388,6 +389,14 @@ static dr_emit_flags_t
 event_app_analysis(void *drcontext, void *tag, instrlist_t *bb, bool for_trace,
                    bool translating, OUT void *user_data)
 {
+    for (instr_t *instr = instrlist_first(bb); instr != NULL;
+         instr = instr_get_next(instr)) {
+        bool dead;
+        CHECK(drreg_are_aflags_dead(drcontext, instr, &dead) == DRREG_SUCCESS,
+              "drreg_are_aflags_dead should always work");
+        CHECK(drx_aflags_are_dead(instr) == dead,
+              "aflags liveness estimation of drx and drreg should always be consistent");
+    }
     return DR_EMIT_DEFAULT;
 }
 
@@ -1139,10 +1148,14 @@ event_instru2instru(void *drcontext, void *tag, instrlist_t *bb, bool for_trace,
 static void
 event_exit(void)
 {
-    if (!drmgr_unregister_bb_insertion_event(event_app_instruction) ||
+    if (!drmgr_unregister_bb_instrumentation_ex_event(event_app2app, event_app_analysis,
+                                                      event_app_instruction,
+                                                      event_instru2instru) ||
         drreg_exit() != DRREG_SUCCESS)
         CHECK(false, "exit failed");
-
+    /* We skip drx_init/drx_exit because that will change the number of total drreg spill
+     * slots available for the drreg tests. Skipping drx_init is okay because we use only
+     * drx_are_aflags_dead which doesn't need any drx state to be initialised. */
     drmgr_exit();
 }