568782 - Failed build js/src/jscntxt.cpp on Solaris

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ginn Chen]

/export/home/moz/tools/ff_slave/mozilla-central-solaris-sparc/build/js/src/jscntxt.cpp", line 157: Error: Formal argument 1 of type char* in call to munmap(char*, unsigned) is being passed int*.

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Wesley W. Garland]

Ginn: IIRC caddr_t isn't universally available (although I know it's been in Solaris since at least 2.5.1).

How about a cast to void * instead?  That would match the SUSv2 spec.

Comment 3

[Ginn Chen]

caddr_t is available on Mac OS X, Linux, Solaris and BSD.

The point here is we cannot use (void *) because munmap is using caddr_t (char *) on Solaris unless _POSIX_C_SOURCE > 2 or _XPG4_2 is defined.

Comment 4

[Wesley W. Garland]

Thanks Ginn -- I missed caddr_t on Mac because it's tucked away in /X11/Xw32defs.h and my test program only included sys/mman.h; it should have also included sys/types.h. I didn't test Linux.

I was surprised to see this issue biting you, though -- perhaps it "goes away" for me because I'm running g++ and hence modified system headers. 

Carry on, forget I said anything! :)

(except, does this need patching in jsgcchunk.cpp:228 too then?:
  JS_ALWAYS_TRUE(munmap(p, size) == 0);
You might not spot it until you do a DEBUG build)

Comment 5

[Ginn Chen]

(In reply to comment #4)
> (except, does this need patching in jsgcchunk.cpp:228 too then?:
>   JS_ALWAYS_TRUE(munmap(p, size) == 0);
> You might not spot it until you do a DEBUG build)

it is in #else /* XP_UNIX */
so ...

Comment 6

[Wesley W. Garland]

Whoops, it's actually unreached because it's not in JS_GC_HAS_MAP_ALIGN and we're on Solaris.

Thanks, Ginn - that's doubly ironic because I'm the original author of the  JS_GC_HAS_MAP_ALIGN code.  

I think I should just stop talking now :)

Comment 7

[Ginn Chen]

Comment on attachment 447956 [details] [diff] [review]
patch

jim is away

Comment 8

[Igor Bukanov]

Comment on attachment 447956 [details] [diff] [review]
patch

>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -149,17 +149,17 @@ StackSpace::init()
> 
> void
> StackSpace::finish()
> {
> #ifdef XP_WIN
>     VirtualFree(base, (commitEnd - base) * sizeof(jsval), MEM_DECOMMIT);
>     VirtualFree(base, 0, MEM_RELEASE);
> #else
>-    munmap(base, CAPACITY_BYTES);
>+    munmap((caddr_t)base, CAPACITY_BYTES);

I would prefer to have explicit ifdef SOLARIS as AFAIK only Solaris has this non void* munmap.

> #endif
> }
> 
> #ifdef XP_WIN
> JS_FRIEND_API(bool)
> StackSpace::bumpCommit(jsval *from, ptrdiff_t nvals) const
> {
>     JS_ASSERT(end - from >= nvals);

Comment 9

[Ginn Chen]

Igor, we didn't use ifdef in elsewhere, do you think I should change them?

e.g.
http://mxr.mozilla.org/mozilla-central/source/js/src/jsgcchunk.cpp#238

It was suggested at Bug 529764 comment 2

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor, we didn't use ifdef in elsewhere, do you think I should change them?

Yes, please change them. Otherwise it is hard for a reader to figure out what exactly caddr_t is doing instead of void * from the documentation.

Comment 11

[Ginn Chen]

Attachment: patch for tracemonkey

The old patch was committed to t-m in another bug by Leon Sha (perhaps accidentally)

So, add #ifdef SOLARIS.

Comment 12

[Ginn Chen]

http://hg.mozilla.org/tracemonkey/rev/26f5eea96492