Bug 5174 – Add support for compiling on Linux (likely to help for other POSIX systems too)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[George Staikos]

This patch (ongoing) adds support for compilation on POSIX platforms as the 
default.  Apple, Win32, Linux, etc specific code is specialized.  Fixes #if 
tests to be more specific and accurate, fixes warnings, compile errors, etc.  
Work remains, but this allows compilation on Linux.  Needs testing on Apple 
and Win32 again as I haven't had time to test on those platforms yet.

[George Staikos]

Created attachment 4085 [details]
Patch for compilation

[George Staikos]

The following snipped makes collector work properly on Linux: 
 
    void *stackBase = 0; 
    pthread_attr_t sattr; 
    int rc; 
    rc = pthread_getattr_np(pthread_self(), &sattr); 
    // Fails on Linux (?) 
    //rc = pthread_attr_getstack(&sattr, &stackBase, &stackSize); 
    rc = pthread_attr_getstackaddr(&sattr, &stackBase); 
    assert(stackBase); 
 
With this applied, the patch now passes everything in the regression suite 
properly on Linux except Date stuff.  Failures: 
        ecma/Date/15.9.3.1-1.js 
        ecma/Date/15.9.3.1-2.js 
        ecma/Date/15.9.3.1-3.js 
        ecma/Date/15.9.3.1-4.js 
        ecma/Date/15.9.3.1-5.js 
        ecma/Date/15.9.3.2-1.js 
        ecma/Date/15.9.3.2-2.js 
        ecma/Date/15.9.3.2-3.js 
        ecma/Date/15.9.3.2-4.js 
        ecma/Date/15.9.3.2-5.js 
        ecma/Date/15.9.3.8-1.js 
        ecma/Date/15.9.3.8-2.js 
        ecma/Date/15.9.3.8-3.js 
        ecma/Date/15.9.3.8-4.js 
        ecma/Date/15.9.3.8-5.js 
        ecma/Date/15.9.4.2.js 
        ecma/Date/15.9.5.10-1.js 
        ecma/Date/15.9.5.10-6.js 
        ecma/Date/15.9.5.10-9.js 
        ecma/Date/15.9.5.12-1.js 
        ecma/Date/15.9.5.12-6.js 
        ecma/Date/15.9.5.14.js 
        ecma/Date/15.9.5.2-1.js 
        ecma/Date/15.9.5.2.js 
        ecma/Date/15.9.5.23-1.js 
        ecma/Date/15.9.5.23-10.js 
        ecma/Date/15.9.5.23-11.js 
        ecma/Date/15.9.5.23-12.js 
        ecma/Date/15.9.5.23-13.js 
        ecma/Date/15.9.5.23-14.js 
        ecma/Date/15.9.5.23-15.js 
        ecma/Date/15.9.5.23-16.js 
        ecma/Date/15.9.5.23-17.js 
        ecma/Date/15.9.5.23-18.js 
        ecma/Date/15.9.5.24-1.js 
        ecma/Date/15.9.5.24-2.js 
        ecma/Date/15.9.5.24-3.js 
        ecma/Date/15.9.5.24-4.js 
        ecma/Date/15.9.5.24-5.js 
        ecma/Date/15.9.5.24-6.js 
        ecma/Date/15.9.5.24-7.js 
        ecma/Date/15.9.5.24-8.js 
        ecma/Date/15.9.5.25-1.js 
        ecma/Date/15.9.5.26-1.js 
        ecma/Date/15.9.5.27-1.js 
        ecma/Date/15.9.5.29-1.js 
        ecma/Date/15.9.5.30-1.js 
        ecma/Date/15.9.5.31-1.js 
        ecma/Date/15.9.5.32-1.js 
        ecma/Date/15.9.5.33-1.js 
        ecma/Date/15.9.5.34-1.js 
        ecma/Date/15.9.5.35-1.js 
        ecma/Date/15.9.5.36-1.js 
        ecma/Date/15.9.5.36-2.js 
        ecma/Date/15.9.5.36-3.js 
        ecma/Date/15.9.5.36-4.js 
        ecma/Date/15.9.5.36-5.js 
        ecma/Date/15.9.5.36-6.js 
        ecma/Date/15.9.5.36-7.js 
        ecma/Date/15.9.5.37-1.js 
        ecma/Date/15.9.5.37-2.js 
        ecma/Date/15.9.5.37-3.js 
        ecma/Date/15.9.5.37-4.js 
        ecma/Date/15.9.5.37-5.js 
        ecma/Date/15.9.5.5.js 
        ecma/Date/15.9.5.6.js 
        ecma/Date/15.9.5.8.js 
        ecma_3/Date/15.9.5.3.js 
        ecma_3/Date/15.9.5.4.js 
        ecma_3/Date/15.9.5.6.js 
 

[George Staikos]

Created attachment 4134 [details]
Updated patch

This fixes almost all regressions and cleans up the code a bit more.  There are
7 regressions remaining in Date and none elsewhere.  They need further
investigation because the actual bug seems to be quite strange.

[George Staikos]

My tests in Mozilla and extended testing of the code here indicates that the 
testcases are wrong.  How are these passing in JSCore on OS X? 

[Darin Adler]

Comment on attachment 4134 [details]
Updated patch

Great direction to go, being portable to POSIX!

The switches to using __APPLE__ for some things that are currently WIN32 seem
great, although I'm not certain __APPLE__ is the best way to check for the Mac
OS X platform. But we can easily switch to something else with a global
replace, so it seems fine to use it for now.

On the other hand, I'm not at all happy with all the changes to use #if defined
X rather than #if X. Is there really a compiler in use where you get warnings
for using #if X when X is not defined? If so, then I suppose we can switch
uniformly to use #if defined X, but we've intentionally not used that style up
until now, and I don't want to change unless there's a good reason. Is this
some feature in a new version of gcc?

I'm also not clear on why the seemingly not-entirely-portable code is the
"else" case. Is this final case truly a "POSIX" case, or is it simply Linux
being treated as the default? I don't understand why pthread_getattr_np is more
portable than pthread_get_stack_addr_np. I sense Linux bias here, and similarly
in the config.h header. Maybe we should seek an ifdef about the presence of
some non-portable pthreads extensions, or invent our own and put it in
config.h.

I see some left-in fprintf and fflush calls. We don't want to land that, I
don't think.

Must the tm_year/tm_mon call go inside an __APPLE__ ifdef? I can see how it's
not needed on all platforms, but is it harmful on any?

[George Staikos]

On Monday 03 October 2005 00:03, you wrote: 
> The switches to using __APPLE__ for some things that are currently WIN32 
> seem great, although I'm not certain __APPLE__ is the best way to check for 
> the Mac OS X platform. But we can easily switch to something else with a 
> global replace, so it seems fine to use it for now. 
 
  Just tell me which one you want and I'm perfectly happy to switch to it.  
There should be a system-level one already defined, right? 
 
> On the other hand, I'm not at all happy with all the changes to use #if 
> defined X rather than #if X. Is there really a compiler in use where you 
> get warnings for using #if X when X is not defined? If so, then I suppose 
> we can switch uniformly to use #if defined X, but we've intentionally not 
> used that style up until now, and I don't want to change unless there's a 
> good reason. Is this some feature in a new version of gcc? 
 
  Older gcc versions definitely warn (at best) for #if X when X is not 
defined.  There is also the case of the #defines not being set causing 
somewhat unpredictable behavior.  I had at least one case where code was being 
compiled that really shouldn't have been. 
 
> I'm also not clear on why the seemingly not-entirely-portable code is the 
> "else" case. Is this final case truly a "POSIX" case, or is it simply Linux 
> being treated as the default? 
 
   Which case do you see this?  I'm checking the manpages for calls in there 
to make sure they're portable.  I was trying to put the "fallback" or "least 
common denominator" case into the "else".  Win32 and OSX specific code surely 
don't fall into that category. 
 
> I don't understand why pthread_getattr_np is  
> more portable than pthread_get_stack_addr_np.  
 
   pthread_attr_getstack is completely broken on my Linux system and 
pthread_get_stackaddr_np isn't available.  I get a warning that 
pthread_attr_getstackaddr is deprecated, but the alternative is simply not 
useful.  Hence the comment...  I haven't had a chance to check OS X, but is 
pthread_attr_getstackaddr available and functional?  It's apparently in IEEE 
1003.1, but does have issues. 
 
> I sense Linux bias here, and  
> similarly in the config.h header. Maybe we should seek an ifdef about the 
> presence of some non-portable pthreads extensions, or invent our own and 
> put it in config.h. 
 
   That config.h header is quite the hack and needs to be cleaned up IMHO.  
Anyway there is no Linux bias.  I was trying to restructure so that anything 
that isn't very basic POSIX-compliant is considered the "else" case.  In most 
cases Mac OS X should also take this case, and WIN32 should be most often the 
"special" case.  My goal is to make this code compile on every platform I can 
get access to. 
 
> I see some left-in fprintf and fflush calls. We don't want to land that, I 
> don't think. 
 
  Ooops, unclean patch.  Sorry.  Please remove those entirely.  They're a 
relic of further debugging work I was doing. 
 
> Must the tm_year/tm_mon call go inside an __APPLE__ ifdef? I can see how 
> it's not needed on all platforms, but is it harmful on any? 
 
  Yes, testcases fail with it in there.  At least on Linux they do.  That code 
seems entirely related to CF*. 

[George Staikos]

Created attachment 4185 [details]
Updated patch

This addresses the printfs, reduces the amount of code that is APPLE_CHANGES,
fixes almost all testcases that pass on OS X and didn't on Linux, and cleans it
up a bit more.	I think it addresses the major issues Darin mentioned.

[George Staikos]

FYI, here is a run of JSCore on Linux with the latest patch: 
 
** Danger, Will Robinson! Danger! The following failures have been introduced: 
        ecma_3/Date/15.9.5.6.js 
 
You fixed the following test: 
        ecma_3/Date/15.9.5.7.js 
 
1 regression found. 
1 test fixed. 
 
 
The regression is toLocale* which is still using the old Linux implementation, 
known broken, and also an issue in KJS.  I guess there is a bug in the JSCore 
Apple toLocale* code though, since we somehow also "fixed" one of those 
bugs. :-) 
 
It would be nice to get this patch in soon so that I can move on to fixing 
bugs and creating a build system.  I'm building patches upon patches at this 
point. 

[Maciej Stachowiak]

I intergrated this with a few tweaks. Going to land shortly. I got rid of the use of defined() for the postive 
case, got rid of /*exec*/ entirely, and clearly flagged the nonportable function used in the portable posix 
case of the thread code.