Bug 213223 - WTF signal machinery is guarded by #if USE(PTHREADS) && HAVE(MACHINE_CONTEXT) but does not use pthreads or machine context
Summary: WTF signal machinery is guarded by #if USE(PTHREADS) && HAVE(MACHINE_CONTEXT)...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-15 17:08 PDT by Michael Catanzaro
Modified: 2020-06-16 16:23 PDT (History)
13 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2020-06-15 17:10 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.22 KB, patch)
2020-06-16 15:55 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-06-15 17:08:19 PDT
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.
Comment 1 Michael Catanzaro 2020-06-15 17:10:53 PDT
Created attachment 401960 [details]
Patch
Comment 2 Michael Catanzaro 2020-06-15 17:17:03 PDT
(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.
Comment 3 Mark Lam 2020-06-15 17:24:46 PDT
Comment on attachment 401960 [details]
Patch

r=me if the EWS is green.
Comment 4 Michael Catanzaro 2020-06-15 19:45:12 PDT
(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....
Comment 5 EWS 2020-06-15 20:03:07 PDT
Committed r263074: <https://trac.webkit.org/changeset/263074>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401960 [details].
Comment 6 Radar WebKit Bug Importer 2020-06-15 20:04:20 PDT
<rdar://problem/64390209>
Comment 7 Michael Catanzaro 2020-06-16 05:11:14 PDT
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.
Comment 8 Michael Catanzaro 2020-06-16 07:14:47 PDT
Reopening
Comment 9 Michael Catanzaro 2020-06-16 15:41:13 PDT
(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.
Comment 10 Michael Catanzaro 2020-06-16 15:52:53 PDT
(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.
Comment 11 Michael Catanzaro 2020-06-16 15:55:54 PDT
Created attachment 402052 [details]
Patch
Comment 12 Yusuke Suzuki 2020-06-16 15:58:34 PDT
Comment on attachment 402052 [details]
Patch

r=me
Comment 13 EWS 2020-06-16 16:23:03 PDT
Committed r263123: <https://trac.webkit.org/changeset/263123>

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