Bug 156015

Summary: Fails to build in Linux / PowerPC due to different ucontext_t definition
Product: WebKit Reporter: Alberto Garcia <berto>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, keith_miller, mark.lam, mcatanzaro, msaboff, sbarati, ysuzuki
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mcatanzaro: review+

Description Alberto Garcia 2016-03-30 01:33:26 PDT
/«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp: In function 'void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void*)':
/«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp:89:37: error: no match for 'operator=' (operand types are 'mcontext_t' and 'ucontext::uc_regs_ptr')
     thread->suspendedMachineContext = userContext->uc_mcontext;
                                     ^
In file included from /usr/include/signal.h:326:0,
                 from /«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.h:36,
                 from /«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp:23:
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note: candidate: mcontext_t& mcontext_t::operator=(const mcontext_t&)
 } mcontext_t;
   ^
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note:   no known conversion for argument 1 from 'ucontext::uc_regs_ptr' to 'const mcontext_t&'
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note: candidate: mcontext_t& mcontext_t::operator=(mcontext_t&&)
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note:   no known conversion for argument 1 from 'ucontext::uc_regs_ptr' to 'mcontext_t&&'
Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/build.make:9801: recipe for target 'Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/heap/MachineStackMarker.cpp.o' failed
make[3]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/heap/MachineStackMarker.cpp.o] Error 1
s
Comment 1 Alberto Garcia 2016-03-30 01:36:44 PDT
[ somehow the previous comment is incomplete, please ignore it ]

I reproduced this with WebKitGTK+ 2.12.0.

It seems that the definition of ucontext_t is different in Linux PowerPC, producing this build error:

/«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp: In function 'void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void*)':
/«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp:89:37: error: no match for 'operator=' (operand types are 'mcontext_t' and 'ucontext::uc_regs_ptr')
     thread->suspendedMachineContext = userContext->uc_mcontext;
                                     ^
In file included from /usr/include/signal.h:326:0,
                 from /«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.h:36,
                 from /«PKGBUILDDIR»/Source/JavaScriptCore/heap/MachineStackMarker.cpp:23:
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note: candidate: mcontext_t& mcontext_t::operator=(const mcontext_t&)
 } mcontext_t;
   ^
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note:   no known conversion for argument 1 from 'ucontext::uc_regs_ptr' to 'const mcontext_t&'
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note: candidate: mcontext_t& mcontext_t::operator=(mcontext_t&&)
/usr/include/powerpc-linux-gnu/sys/ucontext.h:60:3: note:   no known conversion for argument 1 from 'ucontext::uc_regs_ptr' to 'mcontext_t&&'
Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/build.make:9801: recipe for target 'Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/heap/MachineStackMarker.cpp.o' failed
make[3]: *** [Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/heap/MachineStackMarker.cpp.o] Error 1

Here's how it is defined:

    typedef struct ucontext {
        struct ucontext *uc_link;
        sigset_t         uc_sigmask;
        stack_t          uc_stack;
        mcontext_t       uc_mcontext;
        ...
    } ucontext_t;


Here's how it is defined in powerpc:

    typedef struct ucontext {
        struct ucontext *uc_link;
        sigset_t         uc_sigmask;
        stack_t          uc_stack;
        union uc_regs_ptr {
            struct pt_regs *regs;
            mcontext_t *uc_regs;
        } uc_mcontext;
        ...
    } ucontext_t;

Full definition here:

https://sources.debian.net/src/glibc/2.21-9/sysdeps/unix/sysv/linux/powerpc/sys/ucontext.h/?hl=139#L139

According to the setcontext(3) man page, this was removed in POSIX.1-2008 because of portability reasons.
Comment 2 Yusuke Suzuki 2016-03-31 06:09:39 PDT
Created attachment 275275 [details]
Patch
Comment 3 Yusuke Suzuki 2016-03-31 06:10:18 PDT
Could you ensure that the uploaded patch fixes the issue? I don't have any PowerPC machine.
Comment 4 Alberto Garcia 2016-03-31 08:29:51 PDT
(In reply to comment #3)
> Could you ensure that the uploaded patch fixes the issue? I don't have any
> PowerPC machine.

Hey, thanks for the patch!

I'm currently building it and will test it later. I noticed however
that with this patch what suspendedMachineContext stores is not an
mcontext_t but a pointer to an mcontext_t. If I understand it
correctly the actual mcontext_t data in the PowerPC case is stored in
ucontext_t.uc_reg_space.

I wonder if this patch is correct then? If not maybe there's no
portable way to do this...
Comment 5 Yusuke Suzuki 2016-03-31 08:37:27 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Could you ensure that the uploaded patch fixes the issue? I don't have any
> > PowerPC machine.
> 
> Hey, thanks for the patch!
> 
> I'm currently building it and will test it later. I noticed however
> that with this patch what suspendedMachineContext stores is not an
> mcontext_t but a pointer to an mcontext_t. If I understand it
> correctly the actual mcontext_t data in the PowerPC case is stored in
> ucontext_t.uc_reg_space.
> 
> I wonder if this patch is correct then? If not maybe there's no
> portable way to do this...

Oops, that's right. I'll change the patch soon.
Comment 6 Yusuke Suzuki 2016-03-31 08:44:52 PDT
Created attachment 275295 [details]
Patch
Comment 7 Yusuke Suzuki 2016-03-31 08:45:13 PDT
(In reply to comment #6)
> Created attachment 275295 [details]
> Patch

OK, could you try it again?
Comment 8 Alberto Garcia 2016-03-31 10:42:04 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Created attachment 275295 [details]
> > Patch
> 
> OK, could you try it again?

Yeah, that works.
Comment 9 Michael Catanzaro 2016-03-31 11:58:44 PDT
Comment on attachment 275295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275295&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:90
> +    // Since glib in PowerPC does not have mcontext_t in ucontext_t::uc_mcontext, we specially handle this case here.

I would omit this comment; the conditional compilation makes it obvious that the data structure is architecture-specific.
Comment 10 Yusuke Suzuki 2016-03-31 14:48:50 PDT
Comment on attachment 275295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275295&action=review

>> Source/JavaScriptCore/heap/MachineStackMarker.cpp:90
>> +    // Since glib in PowerPC does not have mcontext_t in ucontext_t::uc_mcontext, we specially handle this case here.
> 
> I would omit this comment; the conditional compilation makes it obvious that the data structure is architecture-specific.

OK, thanks. Omitted.
Comment 11 Yusuke Suzuki 2016-03-31 14:52:08 PDT
Committed r198919: <http://trac.webkit.org/changeset/198919>