[PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP

[Michael Ellerman]

Matt Evans <matt at ozlabs.org> writes:
>> On 28 Mar 2018, at 11:36, Matt Evans <matt at ozlabs.org> wrote:
>>> On 28 Mar 2018, at 06:54, Michael Ellerman <mpe at ellerman.id.au> wrote:
>>> Matt Evans <matt at ozlabs.org> writes:
>>>> When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
>>>> user context when single_step_exception() prepares the SIGTRAP
>>>> delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
>>>> isn't healthy.
>>>> 
>>>> Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
>>>> replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
>>>> to clear_single_step() which only clears MSR_SE.
>>>> 
>>>> This patch adds a new helper, clear_br_trace(), which clears the
>>>> debug trap before invoking the signal handler.  This helper is a
>>>> NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.
>>>> 
>>>> Signed-off-by: Matt Evans <matt at ozlabs.org>
>>> 
>>> Hi Matt!
>>> 
>>> It seems we might not be regularly testing this code :}
>> 
>> I know, rite? ;-)
>> 
>>> How did you hit/find the bug? And do you have a test case by any chance?
>>> 
>>> I found the test code at the bottom of:
>>> https://lwn.net/Articles/114587/
>>> 
>>> But it didn't immediately work.
>> 
>> I'm using this feature as part of a debug harness I wrote to log a
>> program’s control flow (to create a “known good” pattern to compare a
>> PPC interpreter against). So at least the feature has /one/ user. ;-)

One is enough for us :)
 
>> The symptoms of the bug are that if you use single-stepping you get a
>> sequence of SIGTRAPs representing each instruction completion (good),
>> but if you use branch tracing the process just dies with SIGTRAP
>> (looks like it’s never caught by the signal handler). What’s really
>> happening is that there /is/ a signal delivered to the handler, but
>> (because branch tracing is left on) that then causes a second debug
>> exception from the handler itself, i.e. whilst SIGTRAP’s masked.
>> 
>> OK, let me have a dig to reduce my program to something very basic
>> and I’ll post something — sorry, I should’ve got a PoC ready before.
>> (I did start out inspired by that post you linked to, but IIRC I
>> don’t think it worked out of the box for me either.)
>
> I’ve put a simple SIG_DBG_BRANCH_TRACING test program here:
>
> 	http://ozlabs.org/~matt/files/sig_dbg_brtrace_test.c

Thanks.

> It’s commented regarding expected output. I’ve only tested this on a
> G4 — it should work on PPC64 too but the ISA says support for branch
> tracing is optional for an implementation. I’d be interested in what
> POWERx does. :)

I get no traps on any 64-bit machine I tried, including 970FX, Power6,
Power7, Power8, Power9.

So I guess it's never been implemented on "server" CPUs.

I'd be happy to turn your test program into a selftest, though it won't
actually catch bugs unless someone runs it on actual 32-bit systems :)

cheers