Closed Bug 568782 Opened 12 years ago Closed 12 years ago

Failed build js/src/jscntxt.cpp on Solaris

Categories

(Core :: JavaScript Engine, defect)

All
OpenSolaris
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

/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*.
Attached patch patch (obsolete) — Splinter Review
Attachment #447956 - Flags: review?(jim)
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.
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.
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)
(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 ...
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 :)
Attachment #447956 - Flags: review?(jim) → review?(igor)
Comment on attachment 447956 [details] [diff] [review]
patch

jim is away
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);
Attachment #447956 - Flags: review?(igor)
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
(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.
The old patch was committed to t-m in another bug by Leon Sha (perhaps accidentally)

So, add #ifdef SOLARIS.
Attachment #447956 - Attachment is obsolete: true
Attachment #460484 - Flags: review?(igor)
Attachment #460484 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/26f5eea96492
Whiteboard: fixed-in-tracemonkey
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.