i#5786: Add precise clean call mangling identification (#5791)

Adds new labels delimiting clean call sequences.
Converts into a translation record flag when storing translations.

Uses the new labels and flag to precisely identify clean call
mangling, replacing the previous scheme which incorrectly thought
mangled tool pc-relative was a clean call, resulting in incorrect
translations and crashes.

Adds a test case to api.detach_state by adding a client (by converting
it to use static DR) which inserts a pc-relative load.  This
reproduces the crash on detach, and is fixed with this fix.
The added instrumentation caused periodic detach failures which were
solved by setting the translation and adding a restore-state event:
i#4232 covers trying to improve the situation.

Adds a new instr_t.offset field.
Stops using instr_t.note to hold encoding offsets for pc-releative
operands.  Adds a new field instr_t.offset which is used for this
purpose.  This leaves note values in place across encodings, which is
needed for new clean call marking labels and also simplifies rseq
handling code.

This instr_t field is a compatibility break and we bump the version and 
OLDEST_COMPATIBLE_VERSION here to 990.

Updates dr_get_note docs.

Augments logging of xl8 info with new flag info.

Reduces DR_NOTE_FIRST_RESERVED to give DR more reserved labels.
This is another compatibility break, while at it.

Fixes several issues hit in tests that happened to trigger on the
heap bucket size and other changes:
+ Fixes a rank order violation at loglevel 5: xref #1649
+ Writes real xstate_bv into signal frame when setting the xstate context to
   avoid lazy AVX restore problems.
+ Tweaks the thread_churn test to work around non-linearities.

Issue: #5786, #4232
Fixes #5786

CMakeLists.txt

@@ -567,7 +567,7 @@ endif (EXISTS "${PROJECT_SOURCE_DIR}/.svn")
 
 # N.B.: when updating this, update the default version in ci-package.yml.
 # We should find a way to share (xref i#1565).
-set(VERSION_NUMBER_DEFAULT "9.0.${VERSION_NUMBER_PATCHLEVEL}")
+set(VERSION_NUMBER_DEFAULT "9.90.${VERSION_NUMBER_PATCHLEVEL}")
 # do not store the default VERSION_NUMBER in the cache to prevent a stale one
 # from preventing future version updates in a pre-existing build dir
 set(VERSION_NUMBER "" CACHE STRING "Version number: leave empty for default")
@@ -1382,7 +1382,7 @@ math(EXPR VERSION_NUMBER_INTEGER
 # 5.0 broke backcompat in drsyms and xmm opnd sizes
 # 4.1 broke backcompat in drsyms + 64-bit core (opcodes + reachability)
 # 4.0 broke backcompat in drmgr, drsyms, drinjectlib, and dr_get_milliseconds()
-set(OLDEST_COMPATIBLE_VERSION_DEFAULT "900")
+set(OLDEST_COMPATIBLE_VERSION_DEFAULT "990")
 set(OLDEST_COMPATIBLE_VERSION "" CACHE STRING
   "Oldest compatible version: leave empty for default")
 if ("${OLDEST_COMPATIBLE_VERSION}" STREQUAL "")

api/docs/release.dox

@@ -136,6 +136,12 @@ changes:
    the register in the structure from struct.value.reg to
    struct.value.reg_and_element_size.reg. The element size can be accessed directly
    via struct.value.reg_and_element_size.element_size.
+ - Changed the size of the #instr_t structure by appending a field which is used
+   for relative offsets while encoding.  The note field is no longer modified
+   during encoding.
+ - Reduced the value of #DR_NOTE_FIRST_RESERVED.  This is not expected to cause
+   problems unless clients are directly choosing high note values without using
+   drmgr_reserve_note_range().
 
 Further non-compatibility-affecting changes include:
  - Added AArchXX support for attaching to a running process.

core/arch/emit_utils_shared.c

@@ -1048,10 +1048,10 @@ encode_with_patch_list(dcontext_t *dcontext, patch_list_t *patch, instrlist_t *i
     }
 
     /* now encode the instructions */
-    /* must set note fields first with offset */
+    /* Must set offset fields first. */
     len = 0;
     for (inst = instrlist_first(ilist); inst; inst = instr_get_next(inst)) {
-        instr_set_note(inst, (void *)(ptr_uint_t)len);
+        inst->offset = len;
         len += instr_length(dcontext, inst);
     }
 

core/arch/mangle_shared.c

@@ -188,6 +188,10 @@ prepare_for_clean_call(dcontext_t *dcontext, clean_call_info_t *cci, instrlist_t
 {
     uint dstack_offs = 0;
 
+    instr_t *start_label = INSTR_CREATE_label(dcontext);
+    instr_set_note(start_label, (void *)DR_NOTE_CALL_SEQUENCE_START);
+    PRE(ilist, instr, start_label);
+
     if (cci == NULL)
         cci = &default_clean_call_info;
 
@@ -427,6 +431,9 @@ cleanup_after_clean_call(dcontext_t *dcontext, clean_call_info_t *cci, instrlist
         PRE(ilist, instr,
             instr_create_restore_from_dcontext(dcontext, REG_XSP, XSP_OFFSET));
     }
+    instr_t *end_label = INSTR_CREATE_label(dcontext);
+    instr_set_note(end_label, (void *)DR_NOTE_CALL_SEQUENCE_END);
+    PRE(ilist, instr, end_label);
 }
 
 bool
@@ -861,10 +868,6 @@ mangle_rseq_create_label(dcontext_t *dcontext, int type, ptr_uint_t data)
 {
     instr_t *label = INSTR_CREATE_label(dcontext);
     instr_set_note(label, (void *)DR_NOTE_RSEQ);
-    /* XXX: The note doesn't surivive encoding, so we also use a flag.  See comment in
-     * instr.h by this flag: maybe we should move a label's note somewhere persistent?
-     */
-    label->flags |= INSTR_RSEQ_ENDPOINT;
     dr_instr_label_data_t *label_data = instr_get_label_data_area(label);
     label_data->data[0] = type;
     label_data->data[1] = data;
@@ -1455,9 +1458,7 @@ mangle_rseq_finalize(dcontext_t *dcontext, instrlist_t *ilist, fragment_t *f)
     cache_pc rseq_start = NULL, rseq_end = NULL, rseq_abort = NULL;
     DEBUG_DECLARE(int label_sets_found = 0;)
     for (instr = instrlist_first(ilist); instr != NULL; instr = instr_get_next(instr)) {
-        if (instr_is_label(instr) &&
-            (instr_get_note(instr) == (void *)DR_NOTE_RSEQ ||
-             TEST(INSTR_RSEQ_ENDPOINT, instr->flags))) {
+        if (instr_is_label(instr) && instr_get_note(instr) == (void *)DR_NOTE_RSEQ) {
             dr_instr_label_data_t *label_data = instr_get_label_data_area(instr);
             switch (label_data->data[0]) {
             case DR_RSEQ_LABEL_ABORT: rseq_abort = pc; break;
@@ -1781,9 +1782,9 @@ d_r_mangle(dcontext_t *dcontext, instrlist_t *ilist, uint *flags INOUT, bool man
 
         if (!instr_is_cti(instr) || instr_is_meta(instr)) {
             if (TEST(INSTR_CLOBBER_RETADDR, instr->flags) && instr_is_label(instr)) {
-                /* move the value to the note field (which the client cannot
+                /* Move the value to the offset field (which the client cannot
                  * possibly use at this point) so we don't have to search for
-                 * this label when we hit the ret instr
+                 * this label when we hit the ret instr.
                  */
                 dr_instr_label_data_t *data = instr_get_label_data_area(instr);
                 instr_t *tmp;
@@ -1799,7 +1800,7 @@ d_r_mangle(dcontext_t *dcontext, instrlist_t *ilist, uint *flags INOUT, bool man
                 for (tmp = instr_get_next(instr); tmp != NULL;
                      tmp = instr_get_next(tmp)) {
                     if (tmp == ret) {
-                        tmp->note = (void *)data->data[1]; /* the value to use */
+                        tmp->offset = data->data[1]; /* the value to use */
                         break;
                     }
                 }

core/arch/x86/emit_utils.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2010-2021 Google, Inc.  All rights reserved.
+ * Copyright (c) 2010-2022 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -226,48 +226,48 @@ insert_spill_or_restore(dcontext_t *dcontext, cache_pc pc, uint flags, bool spil
     } else
 #endif /* X64 */
         if (shared) {
-        /* mov %ebx, fs:os_tls_offset(tls_offs) */
-        /* trying hard to keep the size of the stub 5 for eax, 6 else */
-        /* FIXME: case 5231 when staying on trace space is better,
-         * when going through this to the IBL routine speed asks for
-         * not adding the prefix.
-         */
-        bool addr16 = (require_addr16 || use_addr_prefix_on_short_disp());
-        if (addr16) {
-            *pc = ADDR_PREFIX_OPCODE;
-            pc++;
-        }
-        *pc = TLS_SEG_OPCODE;
-        pc++;
-        *pc = opcode;
-        pc++;
-        if (reg != REG_XAX) {
-            /* 0x1e for ebx, 0x0e for ecx, 0x06 for eax
-             * w/o addr16 those are 0x1d, 0x0d, 0x05
+            /* mov %ebx, fs:os_tls_offset(tls_offs) */
+            /* trying hard to keep the size of the stub 5 for eax, 6 else */
+            /* FIXME: case 5231 when staying on trace space is better,
+             * when going through this to the IBL routine speed asks for
+             * not adding the prefix.
              */
-            *pc = MODRM_BYTE(0 /*mod*/, reg_get_bits(reg), addr16 ? 6 : 5 /*rm*/);
+            bool addr16 = (require_addr16 || use_addr_prefix_on_short_disp());
+            if (addr16) {
+                *pc = ADDR_PREFIX_OPCODE;
+                pc++;
+            }
+            *pc = TLS_SEG_OPCODE;
             pc++;
-        }
-        if (addr16) {
-            *((ushort *)pc) = os_tls_offset(tls_offs);
-            pc += 2;
+            *pc = opcode;
+            pc++;
+            if (reg != REG_XAX) {
+                /* 0x1e for ebx, 0x0e for ecx, 0x06 for eax
+                 * w/o addr16 those are 0x1d, 0x0d, 0x05
+                 */
+                *pc = MODRM_BYTE(0 /*mod*/, reg_get_bits(reg), addr16 ? 6 : 5 /*rm*/);
+                pc++;
+            }
+            if (addr16) {
+                *((ushort *)pc) = os_tls_offset(tls_offs);
+                pc += 2;
+            } else {
+                *((uint *)pc) = os_tls_offset(tls_offs);
+                pc += 4;
+            }
         } else {
-            *((uint *)pc) = os_tls_offset(tls_offs);
-            pc += 4;
-        }
-    } else {
-        /* mov %ebx,((int)&dcontext)+dc_offs */
-        *pc = opcode;
-        pc++;
-        if (reg != REG_XAX) {
-            /* 0x1d for ebx, 0x0d for ecx, 0x05 for eax */
-            *pc = MODRM_BYTE(0 /*mod*/, reg_get_bits(reg), 5 /*rm*/);
+            /* mov %ebx,((int)&dcontext)+dc_offs */
+            *pc = opcode;
             pc++;
+            if (reg != REG_XAX) {
+                /* 0x1d for ebx, 0x0d for ecx, 0x05 for eax */
+                *pc = MODRM_BYTE(0 /*mod*/, reg_get_bits(reg), 5 /*rm*/);
+                pc++;
+            }
+            IF_X64(ASSERT_NOT_IMPLEMENTED(false));
+            *((uint *)pc) = (uint)(ptr_uint_t)UNPROT_OFFS(dcontext, dc_offs);
+            pc += 4;
         }
-        IF_X64(ASSERT_NOT_IMPLEMENTED(false));
-        *((uint *)pc) = (uint)(ptr_uint_t)UNPROT_OFFS(dcontext, dc_offs);
-        pc += 4;
-    }
     ASSERT(IF_X64_ELSE(false, !shared) ||
            (pc - start_pc) ==
                (reg == REG_XAX ? SIZE_MOV_XAX_TO_TLS(flags, require_addr16)
@@ -420,7 +420,7 @@ nop_pad_ilist(dcontext_t *dcontext, fragment_t *f, instrlist_t *ilist, bool emit
                         ASSERT((int)nop_length == instr_length(dcontext, nop_inst));
                         if (emitting) {
                             /* fixup offsets */
-                            instr_set_note(nop_inst, (void *)(ptr_uint_t)offset);
+                            nop_inst->offset = offset;
                             /* only inc stats for emitting, not for recreating */
                             STATS_PAD_JMPS_ADD(f->flags, num_nops, 1);
                             STATS_PAD_JMPS_ADD(f->flags, nop_bytes, nop_length);
@@ -443,7 +443,7 @@ nop_pad_ilist(dcontext_t *dcontext, fragment_t *f, instrlist_t *ilist, bool emit
             }
         }
         if (emitting)
-            instr_set_note(inst, (void *)(ptr_uint_t)offset); /* used by instr_encode */
+            inst->offset = offset; /* used by instr_encode */
         offset += instr_length(dcontext, inst);
     }
     return start_shift;

core/arch/x86/mangle.c

@@ -1726,8 +1726,8 @@ mangle_return(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr,
     }
 
     if (TEST(INSTR_CLOBBER_RETADDR, instr->flags)) {
-        /* we put the value in the note field earlier */
-        ptr_uint_t val = (ptr_uint_t)instr->note;
+        /* we put the value in the offset field earlier */
+        ptr_uint_t val = (ptr_uint_t)instr->offset;
         insert_mov_ptr_uint_beyond_TOS(dcontext, ilist, instr, val, retsz);
     }
 

core/emit.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2012-2021 Google, Inc.  All rights reserved.
+ * Copyright (c) 2012-2022 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -438,9 +438,8 @@ emit_fragment_common(dcontext_t *dcontext, app_pc tag, instrlist_t *ilist, uint
         IF_X64(CLIENT_ASSERT(instr_get_isa_mode(inst) == isa_mode,
                              "single fragment cannot mix x86 and x64 modes"));
         if (!PAD_FRAGMENT_JMPS(flags)) {
-            /* we're going to skip the 2nd pass, save this instr's offset in
-             * the note field (used by instr_encode) */
-            instr_set_note(inst, (void *)(ptr_uint_t)offset);
+            /* We're going to skip the 2nd pass; save the offset for instr_encode. */
+            inst->offset = offset;
         }
         if (instr_ok_to_emit(inst))
             offset += instr_length(dcontext, inst);
@@ -505,7 +504,7 @@ emit_fragment_common(dcontext_t *dcontext, app_pc tag, instrlist_t *ilist, uint
             extra_jmp_padding_body += 2;
             instrlist_append(ilist, INSTR_CREATE_nop(dcontext));
             ASSERT(instr_length(dcontext, instrlist_last(ilist)) == 2);
-            instr_set_note(instrlist_last(ilist), (void *)(ptr_uint_t)offset);
+            instrlist_last(ilist)->offset = offset;
         }
         ASSERT(ALIGNED(offset + extra_jmp_padding_body, PC_LOAD_ADDR_ALIGN));
     }

core/heap.c

@@ -112,18 +112,17 @@ static const uint BLOCK_SIZES[] = {
     /* 40 dbg / 36 rel: */
     ALIGN_FORWARD(sizeof(fragment_t) + sizeof(indirect_linkstub_t), HEAP_ALIGNMENT),
 #if defined(X64)
-    sizeof(instr_t), /* 104 x64 */
 #    ifdef DEBUG
     sizeof(fragment_t) + sizeof(direct_linkstub_t) +
         sizeof(cbr_fallthrough_linkstub_t), /* 112 dbg x64 / 104 rel x64 */
 #    else
-/* release == instr_t */
+    sizeof(instr_t), /* 112 x64 */
 #    endif
 #else
     sizeof(fragment_t) + sizeof(direct_linkstub_t) +
         sizeof(cbr_fallthrough_linkstub_t), /* 60 dbg / 56 rel */
 #    ifndef DEBUG
-    sizeof(instr_t),                        /* 68 */
+    sizeof(instr_t),                        /* 72 */
 #    endif
 #endif
     /* we keep this bucket even though only 10% or so of normal bbs

core/ir/aarch64/codec.c

@@ -229,7 +229,7 @@ encode_pc_off(OUT uint *poff, int bits, byte *pc, instr_t *instr, opnd_t opnd,
     if (opnd.kind == PC_kind)
         off = opnd.value.pc - pc;
     else if (opnd.kind == INSTR_kind)
-        off = (byte *)opnd_get_instr(opnd)->note - (byte *)instr->note;
+        off = (byte *)opnd_get_instr(opnd)->offset - (byte *)instr->offset;
     else
         return false;
     range = (ptr_uint_t)1 << bits;
@@ -783,7 +783,8 @@ encode_opnd_adr_page(int scale, byte *pc, opnd_t opnd, OUT uint *enc_out, instr_
         offset = (ptr_int_t)opnd_get_addr(opnd) -
             (ptr_int_t)((ptr_uint_t)pc >> scale << scale);
     } else if (opnd_is_instr(opnd)) {
-        offset = (ptr_int_t)((byte *)opnd_get_instr(opnd)->note - (byte *)instr->note);
+        offset =
+            (ptr_int_t)((byte *)opnd_get_instr(opnd)->offset - (byte *)instr->offset);
     } else
         return false;
 
@@ -3939,9 +3940,8 @@ encode_opnd_instr(int bit_pos, opnd_t opnd, byte *start_pc, instr_t *containing_
     if (!opnd_is_instr(opnd)) {
         return false;
     }
-    ptr_uint_t val =
-        ((ptr_uint_t)instr_get_note(opnd_get_instr(opnd)) -
-         (ptr_uint_t)instr_get_note(containing_instr) + (ptr_uint_t)start_pc) >>
+    ptr_uint_t val = (opnd_get_instr(opnd)->offset - containing_instr->offset +
+                      (ptr_uint_t)start_pc) >>
         opnd_get_shift(opnd);
 
     uint bits = opnd_size_in_bits(opnd_get_size(opnd));

core/ir/arm/decode_private.h

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2014-2015 Google, Inc.  All rights reserved.
+ * Copyright (c) 2014-2022 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -212,7 +212,7 @@ struct _decode_info_t {
     byte *decorated_pc;
 
     /* For instr_t* target encoding */
-    ptr_int_t cur_note;
+    ptr_int_t cur_offs;
     bool has_instr_opnds;
 
     /* For IT block */

core/ir/arm/encode.c

@@ -1034,12 +1034,12 @@ get_immed_val_shared(decode_info_t *di, opnd_t opnd, bool relative, bool selecte
             CLIENT_ASSERT(opnd_get_shift(opnd) == 0,
                           "relative shifted instr not supported");
             /* For A32, "cur PC" is "PC + 8"; "PC + 4" for Thumb, sometimes aligned */
-            return (ptr_int_t)opnd_get_instr(opnd)->note -
-                (di->cur_note +
+            return (ptr_int_t)opnd_get_instr(opnd)->offset -
+                (di->cur_offs +
                  decode_cur_pc(di->final_pc, di->isa_mode, di->opcode, NULL) -
                  di->final_pc);
         } else {
-            ptr_int_t val = (ptr_int_t)opnd_get_instr(opnd)->note - (di->cur_note) +
+            ptr_int_t val = (ptr_int_t)opnd_get_instr(opnd)->offset - (di->cur_offs) +
                 (ptr_int_t)di->final_pc;
             /* Support insert_mov_instr_addr() by truncating to opnd size */
             uint bits = opnd_size_in_bits(opnd_get_size(opnd));
@@ -1053,9 +1053,9 @@ get_immed_val_shared(decode_info_t *di, opnd_t opnd, bool relative, bool selecte
     } else if (opnd_is_near_pc(opnd)) {
         if (relative) {
             /* For A32, "cur PC" is "PC + 8"; "PC + 4" for Thumb, sometimes aligned */
-            return (ptr_int_t)(
-                opnd_get_pc(opnd) -
-                decode_cur_pc(di->final_pc, di->isa_mode, di->opcode, NULL));
+            return (
+                ptr_int_t)(opnd_get_pc(opnd) -
+                           decode_cur_pc(di->final_pc, di->isa_mode, di->opcode, NULL));
         } else {
             return (ptr_int_t)opnd_get_pc(opnd);
         }
@@ -1353,8 +1353,8 @@ get_abspc_delta(decode_info_t *di, opnd_t opnd)
 {
     /* For A32, "cur PC" is really "PC + 8"; "PC + 4" for Thumb, sometimes aligned */
     if (opnd_is_mem_instr(opnd)) {
-        return (ptr_int_t)opnd_get_instr(opnd)->note -
-            (di->cur_note + decode_cur_pc(di->final_pc, di->isa_mode, di->opcode, NULL) -
+        return (ptr_int_t)opnd_get_instr(opnd)->offset -
+            (di->cur_offs + decode_cur_pc(di->final_pc, di->isa_mode, di->opcode, NULL) -
              di->final_pc) +
             opnd_get_mem_instr_disp(opnd);
     } else {
@@ -3036,7 +3036,7 @@ instr_encode_arch(dcontext_t *dcontext, instr_t *instr, byte *copy_pc, byte *fin
     di.check_reachable = check_reachable;
     di.start_pc = copy_pc;
     di.final_pc = final_pc;
-    di.cur_note = (ptr_int_t)instr->note;
+    di.cur_offs = (ptr_int_t)instr->offset;
     di.encode_state = *get_encode_state(dcontext);
 
     /* We need to track the IT block state even for raw-bits-valid instrs.

core/ir/instr.h

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2021 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2022 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -170,9 +170,8 @@ enum {
 #else
     /* Indicates an instruction that's part of the rseq endpoint.  We use this in
      * instrlist_t.flags (sort of the same namespace: INSTR_OUR_MANGLING is used there,
-     * but also EDI_VAL_*) and as a version of DR_NOTE_RSEQ that survives encoding
-     * (seems like we could store notes for labels in another field so they do
-     * in fact survive: a union with instr_t.translation?).
+     * but also EDI_VAL_*); we no longer use it on individual instructions since
+     * the label note field DR_NOTE_RSEQ now survives encoding.
      */
     INSTR_RSEQ_ENDPOINT = 0x01000000,
 #endif