Bug 63717 – Broken build on QNX

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ademar Reis]

When building QtWebKit on QNX, we get plenty of compilation failures, most of them related to pthreads in JSC. Even after fixing these compilation failures, we still have a broken webkit in the end because JIT is not working, so it'll have to be disabled.

Problem reported by Ritt Konstantin <[e-mail]>. I'll be uploading a patch based on his changes.

[Ademar Reis]

Created attachment 99300 [details]
patch

[WebKit Review Bot]

Attachment 99300 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/StackBounds.cpp:44:  "pthread.h" already included at Source/JavaScriptCore/wtf/StackBounds.cpp:28  [build/include] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ademar Reis]

(In reply to comment #2)
> Source/JavaScriptCore/wtf/StackBounds.cpp:44:  "pthread.h" already included at Source/JavaScriptCore/wtf/StackBounds.cpp:28  [build/include] [4]
> Total errors found: 1 in 5 files

This looks like a false positive, as the includes are guarded with #if OS().

[Patrick R. Gansterer]

Comment on attachment 99300 [details]
patch

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

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:83
> +#if !OS(QNX)
> +/* QNX doesn't support SA_RESTART yet. */

Are you sure you want ENABLE(JSC_MULTIPLE_THREADS)?
Why not disable JSC_MULTIPLE_THREADS if you can't support it correctly?

> Source/JavaScriptCore/wtf/Platform.h:663
> +#if OS(QNX)
> +#define ENABLE_JIT 0
> +#endif

Why do you put it in it's own #if?
IMHO http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Platform.h?rev=90167#L1027 is the correct place for this.

> Source/JavaScriptCore/wtf/StackBounds.cpp:49
> +#include <pthread.h>
>  #include <fcntl.h>
>  #include <sys/procfs.h>
>  #include <stdio.h>
>  #include <errno.h>
> +#include <string.h>

I agree with the false positive, but please sort the headers.

[Ademar Reis]

(In reply to comment #4)
> (From update of attachment 99300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99300&action=review
> 

Thanks for the review, the patch is much more clean now. :-)

> > Source/JavaScriptCore/heap/MachineStackMarker.cpp:83
> > +#if !OS(QNX)
> > +/* QNX doesn't support SA_RESTART yet. */
> 
> Are you sure you want ENABLE(JSC_MULTIPLE_THREADS)?
> Why not disable JSC_MULTIPLE_THREADS if you can't support it correctly?

Perfect, done.

> 
> > Source/JavaScriptCore/wtf/Platform.h:663
> > +#if OS(QNX)
> > +#define ENABLE_JIT 0
> > +#endif
> 
> Why do you put it in it's own #if?
> IMHO http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Platform.h?rev=90167#L1027 is the correct place for this.

Done.

> 
> > Source/JavaScriptCore/wtf/StackBounds.cpp:49
> > +#include <pthread.h>
> >  #include <fcntl.h>
> >  #include <sys/procfs.h>
> >  #include <stdio.h>
> >  #include <errno.h>
> > +#include <string.h>
> 
> I agree with the false positive, but please sort the headers.

Done.

[Ademar Reis]

Created attachment 100552 [details]
patch v2

[WebKit Review Bot]

Attachment 100552 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/StackBounds.cpp:46:  "pthread.h" already included at Source/JavaScriptCore/wtf/StackBounds.cpp:28  [build/include] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Comment on attachment 100552 [details]
patch v2

LGTM, but I'm not a reviewer

[Andreas Kling]

Comment on attachment 100552 [details]
patch v2

Cool, r=me

[WebKit Review Bot]

Comment on attachment 100552 [details]
patch v2

Clearing flags on attachment: 100552

Committed r90916: <http://trac.webkit.org/changeset/90916>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.