<failed to translate> on signal

[egrimley-arm]

I'm not sure if this is a known bug, but this program seems to reliably crash the DEBUG build of DynamoRIO (f892973) on Graviton3:

#include <signal.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

/* Repeatedly call "try", a maximum of "tries" times. If "stop_on_hit"
 * is true, stop when "try" returns true. Return the number of times
 * that "try" returned true. The value of "param" is automatically
 * adjusted according to how "try" sets *adj: negative means increase
 * "param"; positive means decrease it; but "param" is never zero.
 */
int
keep_trying(bool (*try)(int *adj, unsigned long long param, void *arg),
            int tries, unsigned long long param, void *arg, bool stop_on_hit)
{
    int hits = 0;
    /* Typically we halve the step each time, which results in a
     * binary search, but if we have had the same non-zero result
     * several times in succession then we suspect that something has
     * changed so we start doubling the step instead.
     */
    unsigned long long step = 1; /* This will always be power of two. */
    int max_unchanged_results = 4;
    int unchanged_results = max_unchanged_results - 1;
    int previous_result = 0;
    for (int i = 0; i < tries; i++) {
        /* Call try. */
        int adj = 0;
        bool hit = try(&adj, param, arg);

        /* Handle hit. */
        if (hit) {
            ++hits;
            if (stop_on_hit)
                break;
        }

        /* Convert result. */
        int result = adj < 0 ? -1 : adj > 0 ? 1 : 0;

        /* Update unchanged_results. */
        if (result && (i == 0 || result == previous_result))
            ++unchanged_results;
        else
            unchanged_results = 0;
        previous_result = result;

        /* Update step. */
        if (unchanged_results <= max_unchanged_results)
            step = step / 2 > 0 ? step / 2 : step;
        else
            step = step * 2 > 0 ? step * 2 : step;

        /* Adjust param for next try. */
        if (result < 0) {
            param = param + step > param ? param + step : -1ULL;
        } if (result > 0) {
            if (param > step)
                param = param - step;
            else {
                param = 1;
                step = 1;
            }
        }
    }

    return hits;
}

static volatile int volatile_src;
static volatile int volatile_dst;

void
fail(const char *s)
{
    perror(s);
    exit(1);
}

void
handler(int signum, siginfo_t *info, void *ucontext)
{
    volatile_dst = volatile_src;
}

bool try(int *adj, unsigned long long param, void *arg)
{
    timer_t timer = *(timer_t *)arg;

    volatile_dst = 1;
    volatile_src = -1;

    /* Set timer. */
    struct itimerspec spec = {
        { 0, 0 }, { param / 1000000000, param % 1000000000 }
    };
    if (timer_settime(timer, 0, &spec, NULL))
        fail("timer_settime");

    *adj = volatile_dst;

    /* Cancel timer. */
    struct itimerspec spec0 = { { 0, 0 }, { 0, 0 } };
    struct itimerspec old_spec;
    if (timer_settime(timer, 0, &spec0, &old_spec))
        fail("timer_settime");

    if (0)
        printf("%llu %d\n", param, *adj);

    return false;
}

int main()
{
    int signum = SIGPIPE;

    /* Set up signal handler. */
    struct sigaction act = { 0 };
    act.sa_flags = SA_SIGINFO;
    act.sa_sigaction = handler;
    if (sigaction(signum, &act, 0) != 0)
        fail("sigaction");

    /* Create timer. */
    struct sigevent sevp = { 0 };
    sevp.sigev_notify = SIGEV_SIGNAL;
    sevp.sigev_signo = signum;
    timer_t timer;
    if (timer_create(CLOCK_MONOTONIC, &sevp, &timer))
        fail("timer_create");

    keep_trying(try, 100000, 10000, &timer, true);

    printf("all done\n");
    return 0;
}

The error is usually

<failed to translate>
<Application /tmp/bug (647800).  Internal Error: DynamoRIO debug check failure: /.../core/unix/signal.c:3187 false

but sometimes:

<Application /tmp/bug (647810).  Internal Error: DynamoRIO debug check failure: /.../core/translate.c:1116 tdcontext != get_thread_private_dcontext() || INTERNAL_OPTION(stress_recreate_pc) || tdcontext->client_data->is_translating || TEST(FRAG_SELFMOD_SANDBOXED, flags) || TEST(FRAG_WAS_DELETED, flags)

When I enable the printf I can see that the value of param that crashes DynamoRIO is about 1750. It seems that the crash is probably caused by the signal arriving as we come back from the timer_settime system call.

[egrimley-arm]

Not the only way, I think, but one of the ways the test above fails is that we arrive at

dynamorio/core/translate.c

Line 1027 in 5b6d519

res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */

mc->pc is pointing at the LDP instruction at the start of a fragment which was inserted by insert_fragment_prefix:

   0xffffb3fca3a8:      ldp     x0, x1, [x28]
   0xffffb3fca3ac:      ldrsw   x0, [x0, #4]
   0xffffb3fca3b0:      sxtw    x1, w1
   0xffffb3fca3b4:      mov     x8, #0x6e                       // #110
   0xffffb3fca3b8:      b       0xffffb3fca3c0

It probably doesn't matter but the corresponding app code is:

0000000000003fa8 <timer_settime@@GLIBC_2.17>:
    3fa8:       b9800400        ldrsw   x0, [x0, #4]
    3fac:       93407c21        sxtw    x1, w1
    3fb0:       d2800dc8        mov     x8, #0x6e                       // #110
    3fb4:       d4000001        svc     #0x0
    3fb8:       b140041f        cmn     x0, #0x1, lsl #12
    3fbc:       54000048        b.hi    3fc4 <timer_settime@@GLIBC_2.17+0x1c>  // b.pmore
    3fc0:       d65f03c0        ret

So we were about to enter the timer_settime system call rather than coming back from it, as I wrote above.

I can probably do an ugly hack to solve this particular problem, and I might do that temporarily so that I can look for other problems, but what is the planned or recommended way of handling this situation (@derekbruening or anyone)?

[egrimley-arm]

Another way the test can fail is by arriving at

dynamorio/core/translate.c

Line 1114 in 5b6d519

res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */

In this case the app code is:

   0xfffff3f8ab80:      adrp    x16, 0xfffff3f9b000
   0xfffff3f8ab84:      ldr     x17, [x16, #3976]
   0xfffff3f8ab88:      add     x16, x16, #0xf88
   0xfffff3f8ab8c:      br      x17

The fragment looks like:

   0xffffb3fed780:      ldp     x0, x1, [x28]
   0xffffb3fed784:      mov     x16, #0xb000                    // #45056
   0xffffb3fed788:      movk    x16, #0xf3f9, lsl #16
   0xffffb3fed78c:      movk    x16, #0xffff, lsl #32
   0xffffb3fed790:      ldr     x17, [x16, #3976]
   0xffffb3fed794:      add     x16, x16, #0xf88
   0xffffb3fed798:      str     x2, [x28, #16]
   0xffffb3fed79c:      mov     x2, x17
   0xffffb3fed7a0:      b       0xffffb3fed7a4
   0xffffb3fed7a4:      stp     x0, x1, [x28]
   0xffffb3fed7a8:      mov     x0, #0xc368                     // #50024
   0xffffb3fed7ac:      movk    x0, #0xb40b, lsl #16
   0xffffb3fed7b0:      movk    x0, #0xfffd, lsl #32
   0xffffb3fed7b4:      ldr     x1, [x28, #152]
   0xffffb3fed7b8:      br      x1
   0xffffb3fed7bc:      nop
   0xffffb3fed7c0:      cbz     x8, 0xffffb4005030

And mc->pc is pointing at the branch instruction at 0xffffb3fed7a0.

start_cache is 0xffffb3fed784 and end_cache is 0xffffb3fed7c0 so when we were struck by the signal we were in the middle of the fragment on the last of the three instructions (STR, MOV, B) corresponding to the app's BR. How is the translation code expected to handle this?

[egrimley-arm]

A third type of failure has us arriving at:

dynamorio/core/translate.c

Line 1510 in 5b6d519

res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */

A preceding comment says: target is inside an exit stub!

It seems like a normal place to be. Why hasn't it been a problem before, and on other architectures?

[derekbruening]

mc->pc is pointing at the LDP instruction at the start of a fragment which was inserted by insert_fragment_prefix:

If the translate code doesn't handle prefixes (probably because they are not used much on x86) then we just need logic to restore the spilled registers that the prefix is about to restore, which would then get us to the full state instead of just the PC -- right?

[derekbruening]

res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */
A preceding comment says: target is inside an exit stub!

It seems like a normal place to be. Why hasn't it been a problem before, and on other architectures?

On x86 exit stubs are only used temporarily for unlinked code: once linked they are never used, so in steady state you never see one. It's on aarchxx with limited reach branches that we have to always go through stubs in some cases.

[derekbruening]

Why is the full state needed? Normally in a stub or prefix you can get away w/o the full state by failing the translation and requiring the synchall to retry; asynch signals are delayed; so the only issue is the performance cost of the retry. Ideally they'd all have full state to reduce retries (#4232) but it should not be catastrophic to not have the full state in these places.

[egrimley-arm]

mc->pc is pointing at the LDP instruction at the start of a fragment which was inserted by insert_fragment_prefix:

If the translate code doesn't handle prefixes (probably because they are not used much on x86) then we just need logic to restore the spilled registers that the prefix is about to restore, which would then get us to the full state instead of just the PC -- right?

The following seems to do the job:

         if (cpc >= target_cache) {
             if (cpc > target_cache) {
                 if (cpc == start_cache) {
+#ifdef AARCH64
+                    ASSERT(cpc == target_cache + 4 &&
+                           *(uint *)target_cache == 0xa9400780);
+                    reg_t addr = reg_get_value_priv(DR_REG_X28, mc);
+                    reg_t values[2];
+                    memcpy(values, (void *)addr, sizeof(values));
+                    reg_set_value_priv(DR_REG_X0, mc, values[0]);
+                    reg_set_value_priv(DR_REG_X1, mc, values[1]);
+                    mc->pc = instr_get_translation(inst);
+                    return res;
+#endif

Would a patch like that, but referring to helper functions so that it would be easier to port to other architectures, be acceptable?

[derekbruening]

Would a patch like that, but referring to helper functions so that it would be easier to port to other architectures, be acceptable?

A key consideration is whether it would be better to use the general register movement tracking code that lives in translate.c, but inverted: could that then handle all architectures without needing any hardcoded assumptions or new complex code on x86? Walk backward from the end of the prefix (or forward to the end of the prefix) and identify all the registers (incl flags) restored, and then restore those.

If all prefix restore code were for a single instruction, then the suggested code doesn't seem unreasonable, if it lived next to the prefix insertion code so it was clear it handled all the varieties there. But if prefixes are multi-instruction and more complex (x86 trends in that direction), at first glance it seems a general load/store observational walk as just suggested is nicer.

[egrimley-arm]

Why is the full state needed?

Partly because of #133, perhaps: certain timer-generated signals are unnecessarily being treated as synchronous. I'm hitting these problems by using a timer to send SIGPIPE. The problem does not occur with all signal numbers. The frequency of the failures could perhaps be greatly reduced by fixing #133, but if these situations have to be handled anyway (for detach or whatever) then it might be better not to rush to fix #133 so that we can test more easily.

[egrimley-arm]

A key consideration is whether it would be better to use the general register movement tracking code that lives in translate.c

The register movement tracking code is rather difficult to understand, compared to emulating instructions (see #7682), and I'm not sure it covers all AArch64 cases, such as the value in the stolen register being moved to another register temporarily. The stolen register is irrelevant to x86, as I understand it, and there are probably x86 things (xsp?) that are irrelevant to AArch64, so register movement tracking code that handles both architectures might be difficult to modify for someone who isn't familiar with both architectures and how DynamoRIO handles both architectures.

[derekbruening]

A key consideration is whether it would be better to use the general register movement tracking code that lives in translate.c

The register movement tracking code is rather difficult to understand, compared to emulating instructions (see #7682), and I'm not sure it covers all AArch64 cases, such as the value in the stolen register being moved to another register temporarily.

If it doesn't cover a case that occurs in mangling that sounds like something the needs to be addressed.

[egrimley-arm]

If it doesn't cover a case that occurs in mangling that sounds like something the needs to be addressed.

Yes, I'll try to look into that. Emulation is a nice way to move forwards through the fragment instructions for getting to the start of the next app instruction, but if you ever want to move backwards through the fragment instructions for getting to the start of the current app instruction...

[derekbruening]

Yes, I'll try to look into that. Emulation is a nice way to move forwards through the fragment instructions for getting to the start of the next app instruction, but if you ever want to move backwards through the fragment instructions for getting to the start of the current app instruction...

Often there are paired spills and restores which cancel each other out and there's no need to undo each one when both occurred: hence the current walk with restoring not happening until the end, instead of emulating each instruction without any larger view.