Bug 213223 – WTF signal machinery is guarded by #if USE(PTHREADS) && HAVE(MACHINE_CONTEXT) but does not use pthreads or machine context

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Michael Catanzaro]

WTF signal machinery is guarded by #if USE(PTHREADS) && HAVE(MACHINE_CONTEXT) but does not use pthreads or machine context.

This is a follow-up to bug #213200. Mark's patch there should fix jsc-stress-tests when running cloop on architectures that enable HAVE(MACHINE_CONTEXT). This bug should fix it for other architectures.

History: I found this guard was added in bug #173590 to fix the build on ports where HAVE(MACHINE_CONTEXT) was false. I'm testing a build with this setting disabled right now to make sure it works.

[Michael Catanzaro]

Created attachment 401960 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> I'm testing a build with this setting disabled right now to make sure it works.

All good. I looked over the history of the file, but didn't immediately see which commit removed the dependency on machine context.

Remaining problem is that the code lives under Source/WTF/wtf/threads/ but is not related to threading.

[Mark Lam]

Comment on attachment 401960 [details]
Patch

r=me if the EWS is green.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> Remaining problem is that the code lives under Source/WTF/wtf/threads/ but
> is not related to threading.

Source/WTF/wtf/unix would probably be a better place....

[EWS]

Committed r263074: <https://trac.webkit.org/changeset/263074>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401960 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64390209>

[Michael Catanzaro]

So although this code does not use MACHINE_CONTEXT and builds successfully with !HAVE(MACHINE_CONTEXT), this still failed because it does use ucontext_t in WTF::jscSignalHandler:

    PlatformRegisters& registers = registersFromUContext(reinterpret_cast<ucontext_t*>(ucontext));

    bool didHandle = false;
    bool restoreDefaultHandler = false;
    handlers.forEachHandler(signal, [&] (const SignalHandler& handler) {
        switch (handler(signal, sigInfo, registers)) {
        case SignalAction::Handled:
            didHandle = true;
            break;
        case SignalAction::ForceDefault:
            restoreDefaultHandler = true;
            break;
        default:
            break;
        }
    });

registersFromUContext is not implemented for all supported architectures. I need to think about how to handle this.

[Michael Catanzaro]

Reopening

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> WTF signal machinery is guarded by #if USE(PTHREADS) &&
> HAVE(MACHINE_CONTEXT) but does not use pthreads or machine context.

So although this code compiles fine on x86_64 with HAVE(MACHINE_CONTEXT) enabled, it fails to build on ppc64le and s390x due to lacking registersFromUContext, which is guarded by HAVE(MACHINE_CONTEXT). That's my fault for failing to notice this. jscSignalHandler doesn't directly read the PlatformRegisters, but it does need to pass it on to the installed signal handler.

Anyway, the solution here is also simple: we can just create an empty PlatformRegisters object when HAVE(MACHINE_CONTEXT) is false. Then signal handlers that depend on HAVE(MACHINE_CONTEXT) can then use MachineContext to make use of the PlatformRegisters, while signal handlers that don't depend on HAVE(MACHINE_CONTEXT) are not affected. At first, I was worried that passing an empty PlatformRegisters could be open to misuse, but it's actually kinda hard to misuse it because the only sensible thing you can do with it is pass it to MachineContext (otherwise it's just gobbledygook), and you can't use MachineContext outside HAVE(MACHINE_CONTEXT), so... we should be good. Patch incoming for this solution.

An alternative solution would be to have a separate function signature for our signal handlers that lacks the PlatformRegisters parameter when HAVE(MACHINE_CONTEXT) is disabled, but then we have to define every signal handler twice if the signal handler is installed outside HAVE(MACHINE_CONTEXT) guards, so that's not ideal.

Finally, we could attempt to implement every possible architecture in MachineContext.h, but I don't think it's reasonable. Distros support too many CPU architectures nowadays. Fedora only supports a handful of architectures, but Debian has 27. Not going to work.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9)
> So although this code compiles fine on x86_64 with HAVE(MACHINE_CONTEXT)
> enabled, it fails to build on ppc64le and s390x due to lacking
> registersFromUContext, which is guarded by HAVE(MACHINE_CONTEXT).

I meant to write: "So although this code compiles fine on x86_64 with HAVE(MACHINE_CONTEXT) *disabled*" because that's what I tested yesterday. That said, I must have seriously messed up somehow, because there's no way it should have built without registersFromUContext.

[Michael Catanzaro]

Created attachment 402052 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 402052 [details]
Patch

r=me

[EWS]

Committed r263123: <https://trac.webkit.org/changeset/263123>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402052 [details].