bbbuf.c incorrect wrapping on ARM with uqadd16

[toshipiazza]

I wanted to analyze the buffer provided by bbbuf.c which is supposed to wrap when the low 2 bytes of the buffer pointer overflows, so I made this simple patch:

diff --git a/api/samples/bbbuf.c b/api/samples/bbbuf.c
index c2daffd..5352288 100644
--- a/api/samples/bbbuf.c
+++ b/api/samples/bbbuf.c
@@ -71,6 +71,16 @@ typedef struct _per_thread_t {
     void *buf_base;
 } per_thread_t;

+static void
+test(void)
+{
+    void *drcontext = dr_get_current_drcontext();
+    per_thread_t *data = drmgr_get_tls_field(drcontext, tls_idx);
+#define TLS_SLOT(tls_base, enum_val) (void **)((byte *)(tls_base)+tls_offs+(enum_val))
+#define BUF_PTR(tls_base) *(app_pc **)TLS_SLOT(tls_base, 0)
+    dr_printf("Got buf_ptr = %p\n", BUF_PTR(data->seg_base));
+}
+
 static dr_emit_flags_t
 event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst,
                       bool for_trace, bool translating, void *user_data)
@@ -160,6 +170,10 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t *inst
     dr_insert_write_raw_tls(drcontext, bb, inst, tls_seg, tls_offs, reg);
 #endif

+    if (IF_ARM_ELSE(!instr_is_predicated(inst), true)
+        IF_AARCHXX(&& !instr_is_exclusive_store(inst)))
+        dr_insert_clean_call(drcontext, bb, inst, test, false, 0);
+
     if (drreg_unreserve_register(drcontext, bb, inst, reg) != DRREG_SUCCESS)
         DR_ASSERT(false);
 #ifdef ARM

However, I'm seeing some weird behavior at the point where we should be wrapping on ARM (x86 is fine):

Got buf_ptr = 0x6ed7fffc # correct
Got buf_ptr = 0x6ed7ffff # wrong! We should now be at 0x6ed70000
Got buf_ptr = 0x6ed7ffff

I'm wondering why this is happening, since uqadd16 seems to just not be doing the right thing. 0xfffc + 0x0004 should indeed wrap to 0x0000 . If I try qadd16 the problem is still there but we instead wrap at 0x7ffff, which makes sense considering the above. The docs for uqadd16 use the interesting term "saturates," so perhaps this is correct behavior of uqadd16?

[toshipiazza]

Edit: replacing uqadd16 with uadd16 in bbbuf.c seems to work as far as I can tell. uadd16 doesn't modify any flags, maybe we should be using this instruction instead?

[egrimley]

Yes, Q means saturation when it appears in an ARM mnemonic, and saturation means not wrapping. However, SADD16 and QADD16 set the GE bits (or they'd be nothing to distinguish them). That might be why UQADD16 was chosen. Does it definitely have to wrap?

[derekbruening]

uadd16 does modify flags (GE). I have this in my notes:

TODO instead of uqadd16 (which does wrong thing b/c saturation doesn't wrap
around?  and wrapping add instrs write flags?):
  mov r1, r0
  add r0, r0, 4
  pkhtb r0, r1, r0
ALSO: is this any better than regular add + check for end?
  sub, cbz instead of cmp
  or check for sentinel: load

I'm not sure why uqadd16 was left there when we knew it was wrong...

[egrimley]

BFI would be another possibility, I think, something like:

        add      r1, r0, #4
        bfi      r0, r1, #0, #16

[toshipiazza]

@derekbruening Whoops, I missed the fact that the GE flags were written.

@egrimley Doing the bfi sounds like it could work, I'll give it a try

As a bit of backstory/motivation I'm working on #513 which aims to replace the buffer-filling functionality in bbbuf.c and other clients, so potentially when I replace bbbuf.c's own buffer management with drx_buf we can close this issue.

[toshipiazza]

Also just curious but is there a reason not to just perform an add and then simply store the low 2 bytes of r0? This potentially invalidates r0 but bbbuf.c doesn't use the buffer pointer after the update occurs anyway.