Stop clearing PSL_T on the trace trap.

[kib]

Summary

We certainly should clear PSL_T when calling the SIGTRAP signal handler, which is already done my all x86 sendsig(9) ABI code. On the other hand, it is not clear why PSL_T needs to be cleared when returning from the signal handler.

Test Plan

Right now this is opened for the discussion.

[jhb]

So I know that debuggers expect that after a PT_STEP stepping is disabled when the resulting SIGTRAP is reported. I suspect this will break gdb at least. That is, the debugger expects to do something like:

• PT_STEP
• <SIGTRAP with TRAP_TRACE reported>
• PT_CONTINUE
• <run until some other non-trace event occurs>

PT_STEP in particular is documented as only stepping a single instruction. It is less clear what should happen if a debugger does a second PT_CONTINUE after PT_SETSTEP, that is:

• PT_SETSTEP
• PT_CONTINUE
• <SIGTRAP with TRAP_TRACE>
• PT_CONTINUE
• <currently runs without stepping, but possibly should continue stepping until PT_CLEARSTEP>

[kib]

Do clear PSL_T after SIGTRAP for PT_STEP.

[kib]

In D15054#317299, @jhb wrote:

So I know that debuggers expect that after a PT_STEP stepping is disabled when the resulting SIGTRAP is reported. I suspect this will break gdb at least. That is, the debugger expects to do something like:

• PT_STEP
• <SIGTRAP with TRAP_TRACE reported>
• PT_CONTINUE
• <run until some other non-trace event occurs>

PT_STEP in particular is documented as only stepping a single instruction. It is less clear what should happen if a debugger does a second PT_CONTINUE after PT_SETSTEP, that is:

• PT_SETSTEP
• PT_CONTINUE
• <SIGTRAP with TRAP_TRACE>
• PT_CONTINUE
• <currently runs without stepping, but possibly should continue stepping until PT_CLEARSTEP>

Both notes are reasonable, the first one is exactly what I suspected but other discussion went more radical way.

I updated the patch to only clear PSL_T when PT_STEP was used. I believe (but not tested yet) that both your cases should have the described outcome, and that if the thread set PSL_T manually, it should stay set for the thread execution, except for the signal handlers code.

Interaction between manual PSL_T/PT_STEP/PT_SETSTEP is of course muddy and I do not expect this to be important.

[Uff. Apparently I did not pressed 'submit' for the text above. Also, I run ptrace_test after the r332908 on the patched kernel, not sure how good is the exercise]

[jhb]

Honestly, I'm not really sure how useful it is to try to let user processes set PSL_T themselves in user space. It's never worked in any version of FreeBSD and it is requiring several changes to accomodate. I would probably just go back to not preserving PSL_T on exec() and leave the existing handling of PSL_T for debug traps alone under the assumption that the only really sane way to use PSL_T is via a debugger using ptrace(). Are you aware of anything that wants to use PSL_T on itself without using ptrace()?

[kib]

In D15054#319974, @jhb wrote:

Honestly, I'm not really sure how useful it is to try to let user processes set PSL_T themselves in user space. It's never worked in any version of FreeBSD and it is requiring several changes to accomodate. I would probably just go back to not preserving PSL_T on exec() and leave the existing handling of PSL_T for debug traps alone under the assumption that the only really sane way to use PSL_T is via a debugger using ptrace(). Are you aware of anything that wants to use PSL_T on itself without using ptrace()?

I just cite something from the sbcl sources (it is an implementation of the Common List compiler+runtime):
src/runtime/bsd-os.h:#define CANNOT_GET_TO_SINGLE_STEP_FLAG

void
arch_do_displaced_inst(os_context_t *context, unsigned int orig_inst)
{
    unsigned int *pc = (unsigned int*)(*os_context_pc_addr(context));

    /* Put the original instruction back. */
    arch_remove_breakpoint(pc, orig_inst);

 #ifdef CANNOT_GET_TO_SINGLE_STEP_FLAG
    /* Install helper instructions for the single step:
     * pushf; or [esp],0x100; popf. */
    single_step_save1 = *(pc-3);
    single_step_save2 = *(pc-2);
    single_step_save3 = *(pc-1);
    *(pc-3) = 0x9c909090;
    *(pc-2) = 0x00240c81;
    *(pc-1) = 0x9d000001;
 #else
    *context_eflags_addr(context) |= 0x100;
 #endif

    single_stepping = pc;

 #ifdef CANNOT_GET_TO_SINGLE_STEP_FLAG
    *os_context_pc_addr(context) = (os_context_register_t)((char *)pc - 9);
 #endif
}

I am quite sure that there is more similar code around.

[jhb]

Hmm, if this code works now then it might be relying on the existing semantics that stepping is auto-disabled and has to be re-enabled in the SIGTRAP handler to keep stepping? It's not clear. Linux does seem to jump through extra hoops to permit userland to set PSL_T. Linux also permits PSL_T to be set explicitly via PT_SETREGS which FreeBSD does not (and for which sbcl has an explicit hack). We should perhaps permit PT_SETREGS to change PSL_T in addition to these hoops. (In Linux there is a TIF_FORCED_TF flag that seems to function like TDB_STEP and Linux goes further out of its way to "hide" PSL_T from registers returned by PT_GETREGS if PSL_T is only set due to TDB_STEP and not explicitly by userland as well)

sys/kern/sys_process.c

954 ↗

(On Diff #41696)

I'm not sure it's really worth treating SETSTEP differently from PT_STEP. I suspect it is more likely that code expects PT_SETSTEP to be single-shot as well. For platforms that don't support hardware stepping I think PT_SETSTEP is also implicitly single-shot anyway (arm and mips).

[kib]

In D15054#320004, @jhb wrote:

Hmm, if this code works now then it might be relying on the existing semantics that stepping is auto-disabled and has to be re-enabled in the SIGTRAP handler to keep stepping? It's not clear. Linux does seem to jump through extra hoops to permit userland to set PSL_T. Linux also permits PSL_T to be set explicitly via PT_SETREGS which FreeBSD does not (and for which sbcl has an explicit hack). We should perhaps permit PT_SETREGS to change PSL_T in addition to these hoops. (In Linux there is a TIF_FORCED_TF flag that seems to function like TDB_STEP and Linux goes further out of its way to "hide" PSL_T from registers returned by PT_GETREGS if PSL_T is only set due to TDB_STEP and not explicitly by userland as well)

Ok, I will enable PSL_T for setregs. [I looked at the set_regs() and I do not see what could disallow setting of PSL_T. The PSL_USERCHANGE mask includes PSLT_T].

From what I see in the sbcl code, they explicitly clear PSL_T after doing required execution advance.

[ali_mashtizadeh.com]

In D15054#319974, @jhb wrote:

Honestly, I'm not really sure how useful it is to try to let user processes set PSL_T themselves in user space. It's never worked in any version of FreeBSD and it is requiring several changes to accomodate. I would probably just go back to not preserving PSL_T on exec() and leave the existing handling of PSL_T for debug traps alone under the assumption that the only really sane way to use PSL_T is via a debugger using ptrace(). Are you aware of anything that wants to use PSL_T on itself without using ptrace()?

From the related bugs that are already fixed. This particular feature (not the forking one) was a regression from FreeBSD 2.1 when someone fixed some issues around ptrace() behavior. It seemed they ignored that processes can trace themselves.

[ali_mashtizadeh.com]

In D15054#320004, @jhb wrote:

Hmm, if this code works now then it might be relying on the existing semantics that stepping is auto-disabled and has to be re-enabled in the SIGTRAP handler to keep stepping? It's not clear. Linux does seem to jump through extra hoops to permit userland to set PSL_T. Linux also permits PSL_T to be set explicitly via PT_SETREGS which FreeBSD does not (and for which sbcl has an explicit hack). We should perhaps permit PT_SETREGS to change PSL_T in addition to these hoops. (In Linux there is a TIF_FORCED_TF flag that seems to function like TDB_STEP and Linux goes further out of its way to "hide" PSL_T from registers returned by PT_GETREGS if PSL_T is only set due to TDB_STEP and not explicitly by userland as well)

It's probably worth supporting this behavior.

kib: I also wonder if we can push the logic into the ptrace code rather than making the trap code more complex. It seems other BSDs have pushed the behavior into the trap code. IIRC, they call into the arch specific code from sys_process.c to set/clear the trace flag. Noticed this when I was looking for the original patches.

We should probably check in a few regression tests to verify the trace flag and fork+tracing behavior on x86. I can send you the ones I have if you want to use those.

[kib]

In D15054#320737, @ali_mashtizadeh.com wrote:

kib: I also wonder if we can push the logic into the ptrace code rather than making the trap code more complex. It seems other BSDs have pushed the behavior into the trap code. IIRC, they call into the arch specific code from sys_process.c to set/clear the trace flag. Noticed this when I was looking for the original patches.

I do not think that trying to move that into MI layer is feasible. The methods to manage single stepping are too arch-depended. IMO it is fine to have that logic in trap/machdep sendsig(). And, we do have ptrace_single_step()/ptrace_clear_single_step(), more, they are patched in the revision.

We should probably check in a few regression tests to verify the trace flag and fork+tracing behavior on x86. I can send you the ones I have if you want to use those.

It would be best if you create ATF tests and put them into review.

[ali_mashtizadeh.com]

In D15054#320744, @kib wrote:

In D15054#320737, @ali_mashtizadeh.com wrote:

kib: I also wonder if we can push the logic into the ptrace code rather than making the trap code more complex. It seems other BSDs have pushed the behavior into the trap code. IIRC, they call into the arch specific code from sys_process.c to set/clear the trace flag. Noticed this when I was looking for the original patches.

I do not think that trying to move that into MI layer is feasible. The methods to manage single stepping are too arch-depended. IMO it is fine to have that logic in trap/machdep sendsig(). And, we do have ptrace_single_step()/ptrace_clear_single_step(), more, they are patched in the revision.

We should probably check in a few regression tests to verify the trace flag and fork+tracing behavior on x86. I can send you the ones I have if you want to use those.

It would be best if you create ATF tests and put them into review.

Sounds good, I'll take care of it after the libc/libsys stuff is up.

[kib]

Rebase after r334009.

[jhb]

sys/i386/i386/trap.c

360 ↗

(On Diff #42832)

Oops, this should probably just be committed as a followup fix on its own.

sys/kern/sys_process.c

954 ↗

(On Diff #41696)

I still am not sure it's worth treating PT_SETSTEP differently and feel like we should just document that PT_SETSTEP is single-shot instead.

[kib]

sys/i386/i386/trap.c

360 ↗

(On Diff #42832)

I do not think this is critical, it just improves the later diagnostic for the rarely used feature.

[kib]

Do not change the mode of PT_SETSTEP, instead document that it is auto-cleaned.

[jhb]

lib/libc/sys/ptrace.2

610 ↗

(On Diff #42853)

Nits: s/steeping/stepping/, s/catched/caught/

Perhaps tweak it a bit like so:

"Stepping is automatically disabled when a single step trap is caught."

[kib]

Rebase after the man commit.