Bug 63717

Summary: Broken build on QNX
Product: WebKit Reporter: Ademar Reis <ademar>
Component: New BugsAssignee: Ademar Reis <ademar>
Status: RESOLVED FIXED    
Severity: Normal CC: paroga, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Other   
Attachments:
Description Flags
patch
paroga: review-, paroga: commit-queue-
patch v2 none

Description Ademar Reis 2011-06-30 07:38:07 PDT
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 <ritt.ks@gmail.com>. I'll be uploading a patch based on his changes.
Comment 1 Ademar Reis 2011-06-30 07:52:54 PDT
Created attachment 99300 [details]
patch
Comment 2 WebKit Review Bot 2011-06-30 07:55:39 PDT
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.
Comment 3 Ademar Reis 2011-06-30 09:05:01 PDT
(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().
Comment 4 Patrick R. Gansterer 2011-07-09 13:29:05 PDT
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.
Comment 5 Ademar Reis 2011-07-12 13:24:57 PDT
(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.
Comment 6 Ademar Reis 2011-07-12 13:26:09 PDT
Created attachment 100552 [details]
patch v2
Comment 7 WebKit Review Bot 2011-07-12 13:28:18 PDT
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.
Comment 8 Patrick R. Gansterer 2011-07-12 14:56:30 PDT
Comment on attachment 100552 [details]
patch v2

LGTM, but I'm not a reviewer
Comment 9 Andreas Kling 2011-07-13 06:32:27 PDT
Comment on attachment 100552 [details]
patch v2

Cool, r=me
Comment 10 WebKit Review Bot 2011-07-13 07:25:37 PDT
Comment on attachment 100552 [details]
patch v2

Clearing flags on attachment: 100552

Committed r90916: <http://trac.webkit.org/changeset/90916>
Comment 11 WebKit Review Bot 2011-07-13 07:25:41 PDT
All reviewed patches have been landed.  Closing bug.