i#4128: Add cleancall reg read/write flags (#5164)

Adds new dr_cleancall_save_t flags which are required for proper
interaction between clean calls and drreg:
DR_CLEANCALL_READS_APP_CONTEXT must be set for dr_get_mcontext() to
obtain the proper values, and #DR_CLEANCALL_WRITES_APP_CONTEXT must be
set to ensure that dr_set_mcontext() is persistent.
DR_CLEANCALL_MULTIPATH must be additionally set for might-skip calls.

Adds a clean call insertion event to enable drreg to know about clean
calls at the time they are inserted.  dr_insert_clean_call_ex()
invokes the callback and passes the flags to drreg, who then treats
the clean call as an app instruction.

For annotations, for now we leave drreg looking for the annotation
label (possible future changes #5160 or #5161 would eliminate this
special case).

dr_insert_{cbr,ubr,mbr,call}_instrumentation() always set both labels.

drwrap always sets both labels for pre and post callbacks.

Updates uses throughout our tests and samples to use the new flags as
appropriate.

Adds drreg_statelessly_restore_all() for clean call multipath restoration.

Adds a new dedicated test client.drwrap-drreg-test which tests both a
drwrap call and direct clean calls.

Fixes a missing drwrap cache invalidation on module unload that the
new test uncovers.

This likely fixes #4711 as its code was passing the same location for
the where_respill as where_restore for stateless drreg restoration;
the automated restore here correctly passes the post-instr location.

Issue: #4128, #4711
Fixes #4128

api/docs/release.dox

@@ -242,6 +242,15 @@ Further non-compatibility-affecting changes include:
    This is currently an experimental option.
  - Added \ref page_drcallstack Extension for walking application callstacks, with
    an initial Linux-only implementation.
+ - Added new #dr_cleancall_save_t flags which are required for proper interaction
+   between clean calls and drreg: #DR_CLEANCALL_READS_APP_CONTEXT must be set for
+   dr_get_mcontext() to obtain the proper values, and #DR_CLEANCALL_WRITES_APP_CONTEXT
+   must be set to ensure that dr_set_mcontext() is persistent.
+   If the call may be skipped by tool-inserted control flow, #DR_CLEANCALL_MULTIPATH
+   must additionally be set.
+ - Added a new event dr_register_clean_call_insertion_event(), meant for use by
+   register management libraries.
+ - Added drreg_statelessly_restore_all().
 
 **************************************************
 <hr>

api/samples/cbr.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2014-2020 Google, Inc.  All rights reserved.
+ * Copyright (c) 2014-2021 Google, Inc.  All rights reserved.
  * Copyright (c) 2008 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -354,10 +354,11 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst
             /* Callout for the not-taken case.  Insert after
              * the cbr (i.e., 3rd argument is NULL).
              */
-            dr_insert_clean_call(drcontext, bb, NULL, (void *)at_not_taken,
-                                 false /* don't save fp state */,
-                                 2 /* 2 args for at_not_taken */, OPND_CREATE_INTPTR(src),
-                                 OPND_CREATE_INTPTR(fall));
+            dr_insert_clean_call_ex(drcontext, bb, NULL, (void *)at_not_taken,
+                                    DR_CLEANCALL_READS_APP_CONTEXT |
+                                        DR_CLEANCALL_MULTIPATH,
+                                    2 /* 2 args for at_not_taken */,
+                                    OPND_CREATE_INTPTR(src), OPND_CREATE_INTPTR(fall));
         }
 
         /* After the callout, jump to the original fallthrough
@@ -380,10 +381,11 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst
 
         if (insert_taken) {
             /* Callout for the taken case */
-            dr_insert_clean_call(drcontext, bb, NULL, (void *)at_taken,
-                                 false /* don't save fp state */,
-                                 2 /* 2 args for at_taken */, OPND_CREATE_INTPTR(src),
-                                 OPND_CREATE_INTPTR(targ));
+            dr_insert_clean_call_ex(drcontext, bb, NULL, (void *)at_taken,
+                                    DR_CLEANCALL_READS_APP_CONTEXT |
+                                        DR_CLEANCALL_MULTIPATH,
+                                    2 /* 2 args for at_taken */, OPND_CREATE_INTPTR(src),
+                                    OPND_CREATE_INTPTR(targ));
         }
 
         /* After the callout, jump to the original target

clients/drcachesim/tracer/tracer.cpp

@@ -1708,34 +1708,10 @@ event_delay_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t
     }
 #        endif
 
-    /* hit_instr_count_threshold does not always return. Restore scratch registers and
-     * aflags.
-     */
-#        ifdef X86_64
-    /* FIXME i#4711: Need to restore for x86 the arithmetic flags and (if used) the
-     * scratch register before the call to hit_instr_count_threshold. However, this
-     * fix seems to cause instability. So, we're leaving x86 as technically broken to
-     * keep our tests green until the source of instability is found.
-     */
-#            ifndef DISABLED_FOR_BUG_4711
-    drreg_statelessly_restore_app_value(drcontext, bb, DR_REG_NULL, instr, instr, NULL,
-                                        NULL);
-    if (scratch != DR_REG_NULL) {
-        drreg_statelessly_restore_app_value(drcontext, bb, scratch, instr, instr, NULL,
-                                            NULL);
-    }
-#            endif
-#        elif defined(AARCH64)
-    drreg_statelessly_restore_app_value(drcontext, bb, scratch1, instr, instr, NULL,
-                                        NULL);
-    if (scratch2 != DR_REG_NULL) {
-        drreg_statelessly_restore_app_value(drcontext, bb, scratch2, instr, instr, NULL,
-                                            NULL);
-    }
-#        endif
-    dr_insert_clean_call(drcontext, bb, instr, (void *)hit_instr_count_threshold,
-                         false /*fpstate */, 1,
-                         OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr)));
+    dr_insert_clean_call_ex(drcontext, bb, instr, (void *)hit_instr_count_threshold,
+                            static_cast<dr_cleancall_save_t>(
+                                DR_CLEANCALL_READS_APP_CONTEXT | DR_CLEANCALL_MULTIPATH),
+                            1, OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr)));
     MINSERT(bb, instr, skip_call);
 
 #        ifdef X86_64
@@ -1759,9 +1735,10 @@ event_delay_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t
      * inlining of check_instr_count_threshold is not implemented for i386. For now we pay
      * the cost of a clean call every time for 32-bit architectures.
      */
-    dr_insert_clean_call(drcontext, bb, instr, (void *)check_instr_count_threshold,
-                         false /*fpstate */, 2, OPND_CREATE_INT32(num_instrs),
-                         OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr)));
+    dr_insert_clean_call_ex(drcontext, bb, instr, (void *)check_instr_count_threshold,
+                            DR_CLEANCALL_READS_APP_CONTEXT, 2,
+                            OPND_CREATE_INT32(num_instrs),
+                            OPND_CREATE_INTPTR((ptr_uint_t)instr_get_app_pc(instr)));
 #endif
     return DR_EMIT_DEFAULT;
 }

clients/drcpusim/drcpusim.cpp

@@ -1,5 +1,5 @@
 /* ******************************************************************************
- * Copyright (c) 2015-2017 Google, Inc.  All rights reserved.
+ * Copyright (c) 2015-2021 Google, Inc.  All rights reserved.
  * ******************************************************************************/
 
 /*
@@ -756,8 +756,11 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst
         dr_save_reg(drcontext, bb, instr, DR_REG_XCX, SPILL_SLOT_2);
         // XXX: technically drmgr doesn't want us inserting instrs *after* the
         // app instr but this is the simplest way to go.
-        dr_insert_clean_call(drcontext, bb, instr_get_next(instr), (void *)fake_cpuid,
-                             false, 0);
+        dr_insert_clean_call_ex(
+            drcontext, bb, instr_get_next(instr), (void *)fake_cpuid,
+            static_cast<dr_cleancall_save_t>(DR_CLEANCALL_READS_APP_CONTEXT |
+                                             DR_CLEANCALL_WRITES_APP_CONTEXT),
+            0);
     }
 #endif
     return DR_EMIT_DEFAULT;

core/annotations.c

@@ -388,6 +388,9 @@ instrument_annotation(dcontext_t *dcontext, IN OUT app_pc *start_pc,
                      * The placeholder is "ok to mangle" because it (partially)
                      * implements the app's annotation. The placeholder will be
                      * removed post-client during mangling.
+                     * We only support writing the return value and no other registers
+                     * (otherwise we'd need drrg to further special-case
+                     * DR_NOTE_ANNOTATION).
                      */
                     instr_t *return_placeholder =
                         INSTR_XL8(INSTR_CREATE_mov_st(dcontext, opnd_create_reg(REG_XAX),
@@ -476,6 +479,8 @@ instrument_valgrind_annotation(dcontext_t *dcontext, instrlist_t *bb, instr_t *x
     /* Append `mov $0x0,%edx` so that clients and tools recognize that %xdx will be
      * written here. The placeholder is "ok to mangle" because it (partially) implements
      * the app's annotation. The placeholder will be removed post-client during mangling.
+     * We only support writing the return value and no other registers (otherwise
+     * we'd need drreg to further special-case DR_NOTE_ANNOTATION).
      */
     return_placeholder = INSTR_XL8(
         INSTR_CREATE_mov_st(dcontext, opnd_create_reg(REG_XDX), OPND_CREATE_INT32(0)),

core/arch/x86/mangle.c

@@ -3058,10 +3058,14 @@ mangle_annotation_helper(dcontext_t *dcontext, instr_t *label, instrlist_t *ilis
                 dcontext, (ptr_int_t)pc, dr_reg_spill_slot_opnd(dcontext, SPILL_SLOT_2),
                 ilist, label, NULL, NULL);
         }
-        dr_insert_clean_call_ex_varg(dcontext, ilist, label,
-                                     receiver->instrumentation.callback,
-                                     receiver->save_fpstate ? DR_CLEANCALL_SAVE_FLOAT : 0,
-                                     handler->num_args, args);
+        dr_insert_clean_call_ex_varg(
+            dcontext, ilist, label, receiver->instrumentation.callback,
+            (receiver->save_fpstate ? DR_CLEANCALL_SAVE_FLOAT : 0) |
+                /* Setting a return value is already handled with an inserted app
+                 * instruction, so we do not set the DR_CLEANCALL_WRITES_APP_CONTEXT flag.
+                 */
+                DR_CLEANCALL_READS_APP_CONTEXT,
+            handler->num_args, args);
         if (handler->num_args != 0) {
             HEAP_ARRAY_FREE(dcontext, args, opnd_t, handler->num_args, ACCT_CLEANCALL,
                             UNPROTECTED);

core/lib/dr_events.h

@@ -1412,6 +1412,31 @@ DR_API
 bool
 dr_unregister_low_on_memory_event(void (*func)());
 
+DR_API
+/**
+ * Registers a callback function that is invoked whenever a clean call is inserted
+ * in instrumentation, such as by dr_insert_clean_call(), dr_insert_clean_call_ex(),
+ * or dr_insert_clean_call_ex_varg().
+ * 'where' is a label with note value #DR_NOTE_CLEAN_CALL_END; the clean call
+ * sequence will be inserted prior to the label after all clean call callbacks
+ * are finished (so the clean call sequence is not yet visible at the time this
+ * callback is invoked).
+ */
+void
+dr_register_clean_call_insertion_event(void (*func)(void *drcontext, instrlist_t *ilist,
+                                                    instr_t *where,
+                                                    dr_cleancall_save_t call_flags));
+
+DR_API
+/**
+ * Unregisters a callback function that was registered with
+ * dr_register_call_insertion_event().
+ */
+bool
+dr_unregister_clean_call_insertion_event(void (*func)(void *drcontext, instrlist_t *ilist,
+                                                      instr_t *where,
+                                                      dr_cleancall_save_t call_flags));
+
 /****************************************************************************
  * SECURITY SUPPORT
  */

core/lib/dr_ir_utils.h

@@ -369,9 +369,15 @@ DR_API
  * and will be copied into the proper location for that argument
  * slot as specified by the calling convention.
  *
- * Stores the application state information on the DR stack, where it can
- * be accessed from \c callee using dr_get_mcontext() and modified using
- * dr_set_mcontext().
+ * Stores the application state information on the DR stack, where it can be
+ * accessed from \c callee using dr_get_mcontext() and modified using
+ * dr_set_mcontext().  However, if register reservation code is in use (e.g.,
+ * via the drreg extension library: \ref page_drreg), dr_insert_clean_call_ex()
+ * must be called with its flags argument including
+ * #DR_CLEANCALL_READS_APP_CONTEXT (for dr_get_mcontext() use) and/or
+ * #DR_CLEANCALL_WRITES_APP_CONTEXT (for dr_set_mcontext() use) (and
+ * possibly #DR_CLEANCALL_MULTIPATH) to ensure proper interaction with
+ * register reservations.
  *
  * On x86, if \p save_fpstate is true, preserves the x87 floating-point and
  * MMX state on the
@@ -434,48 +440,6 @@ void
 dr_insert_clean_call(void *drcontext, instrlist_t *ilist, instr_t *where, void *callee,
                      bool save_fpstate, uint num_args, ...);
 
-/**
- * Flags to request non-default preservation of state in a clean call
- * as well as other call options.
- */
-typedef enum {
-    /**
-     * Save legacy floating-point state (x86-specific; not saved by default).
-     * The last floating-point instruction address (FIP) in the saved state is
-     * left in an untranslated state (i.e., it may point into the code cache).
-     * This flag is orthogonal to the saving of SIMD registers and related flags below.
-     */
-    DR_CLEANCALL_SAVE_FLOAT = 0x0001,
-    /**
-     * Skip saving the flags and skip clearing the flags (including
-     * DF) for client execution.  Note that this can cause problems
-     * if dr_redirect_execution() is called from a clean call,
-     * as an uninitialized flags value can cause subtle errors.
-     */
-    DR_CLEANCALL_NOSAVE_FLAGS = 0x0002,
-    /** Skip saving any XMM or YMM registers (saved by default). */
-    DR_CLEANCALL_NOSAVE_XMM = 0x0004,
-    /** Skip saving any XMM or YMM registers that are never used as parameters. */
-    DR_CLEANCALL_NOSAVE_XMM_NONPARAM = 0x0008,
-    /** Skip saving any XMM or YMM registers that are never used as return values. */
-    DR_CLEANCALL_NOSAVE_XMM_NONRET = 0x0010,
-    /**
-     * Requests that an indirect call be used to ensure reachability, both for
-     * reaching the callee and for any out-of-line helper routine calls.
-     * Only honored for 64-bit mode, where r11 will be used for the indirection.
-     */
-    DR_CLEANCALL_INDIRECT = 0x0020,
-    /* internal use only: maps to META_CALL_RETURNS_TO_NATIVE in insert_meta_call_vargs */
-    DR_CLEANCALL_RETURNS_TO_NATIVE = 0x0040,
-    /**
-     * Requests that out-of-line state save and restore routines be used even
-     * when a subset of the state does not need to be preserved for this callee.
-     * Also disables inlining.
-     * This helps guarantee that the inserted code remains small.
-     */
-    DR_CLEANCALL_ALWAYS_OUT_OF_LINE = 0x0080,
-} dr_cleancall_save_t;
-
 DR_API
 /**
  * Identical to dr_insert_clean_call() except it takes in \p
@@ -667,6 +631,9 @@ DR_API
  * two arguments:
  * -# address of call instruction (caller)
  * -# target address of call (callee)
+ * \note Sets #DR_CLEANCALL_READS_APP_CONTEXT and #DR_CLEANCALL_WRITES_APP_CONTEXT.
+ * Conditionally skipping the instrumentation inserted by this routine is not
+ * supported (i.e., #DR_CLEANCALL_MULTIPATH is not supported here).
  */
 void
 dr_insert_call_instrumentation(void *drcontext, instrlist_t *ilist, instr_t *instr,
@@ -682,6 +649,9 @@ DR_API
  * \note Only the address portion of a far indirect branch is considered.
  * \note \p scratch_slot must be <= dr_max_opnd_accessible_spill_slot(). \p
  * scratch_slot is used internally to this routine and will be clobbered.
+ * \note Sets #DR_CLEANCALL_READS_APP_CONTEXT and #DR_CLEANCALL_WRITES_APP_CONTEXT.
+ * Conditionally skipping the instrumentation inserted by this routine is not
+ * supported (i.e., #DR_CLEANCALL_MULTIPATH is not supported here).
  */
 /* If we re-enable -opt_speed (or -indcall2direct directly) we should add back:
  * \note This routine is not supported when the -opt_speed option is specified.
@@ -698,6 +668,9 @@ DR_API
  * -# address of branch instruction
  * -# target address of branch
  * -# 0 if the branch is not taken, 1 if it is taken
+ * \note Sets #DR_CLEANCALL_READS_APP_CONTEXT and #DR_CLEANCALL_WRITES_APP_CONTEXT.
+ * Conditionally skipping the instrumentation inserted by this routine is not
+ * supported (i.e., #DR_CLEANCALL_MULTIPATH is not supported here).
  */
 void
 dr_insert_cbr_instrumentation(void *drcontext, instrlist_t *ilist, instr_t *instr,
@@ -714,6 +687,9 @@ DR_API
  * -# 0 if the branch is not taken, 1 if it is taken
  * -# user defined operand (e.g., TLS slot, immed value, register, etc.)
  * \note The user defined operand cannot use register ebx!
+ * \note Sets #DR_CLEANCALL_READS_APP_CONTEXT and #DR_CLEANCALL_WRITES_APP_CONTEXT.
+ * Conditionally skipping the instrumentation inserted by this routine is not
+ * supported (i.e., #DR_CLEANCALL_MULTIPATH is not supported here).
  */
 void
 dr_insert_cbr_instrumentation_ex(void *drcontext, instrlist_t *ilist, instr_t *instr,
@@ -730,6 +706,9 @@ DR_API
  *
  * \warning Basic block eliding is controlled by -max_elide_jmp.  If that
  * option is set to non-zero, ubrs may never be seen.
+ * \note Sets #DR_CLEANCALL_READS_APP_CONTEXT and #DR_CLEANCALL_WRITES_APP_CONTEXT.
+ * Conditionally skipping the instrumentation inserted by this routine is not
+ * supported (i.e., #DR_CLEANCALL_MULTIPATH is not supported here).
  */
 void
 dr_insert_ubr_instrumentation(void *drcontext, instrlist_t *ilist, instr_t *instr,
@@ -790,7 +769,12 @@ DR_API
  * by the \p flags field of \p context into \p context.
  *
  * This routine may only be called from:
- * - A clean call invoked by dr_insert_clean_call() or dr_prepare_for_call()
+
+ * - A clean call invoked by dr_insert_clean_call() or dr_prepare_for_call().  If
+ *   register reservation code is in use (e.g., via the drreg extension library \ref
+ *   page_drreg), dr_insert_clean_call_ex() must be used with its flags argument
+ *   including #DR_CLEANCALL_READS_APP_CONTEXT (and possibly
+ *   #DR_CLEANCALL_MULTIPATH)to ensure proper interaction with register reservations.
  * - A pre- or post-syscall event (dr_register_pre_syscall_event(),
  *   dr_register_post_syscall_event())
  * - Basic block or trace creation events (dr_register_bb_event(),
@@ -854,7 +838,12 @@ DR_API
  * flags field of \p context to the values in \p context.
  *
  * This routine may only be called from:
- * - A clean call invoked by dr_insert_clean_call() or dr_prepare_for_call()
+ * - A clean call invoked by dr_insert_clean_call() or dr_prepare_for_call().  If
+ *   register reservation code is in use (e.g., via the drreg extension library \ref
+ *   page_drreg), dr_insert_clean_call_ex() must be used with its flags argument
+ *   including #DR_CLEANCALL_WRITES_APP_CONTEXT (and possibly
+ *   #DR_CLEANCALL_MULTIPATH) to ensure proper interaction with register
+ *   reservations.
  * - A pre- or post-syscall event (dr_register_pre_syscall_event(),
  *   dr_register_post_syscall_event())
  *   dr_register_thread_exit_event())