bustage for Solaris gcc box

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Pete Zha, Unassigned)

Tracking

Trunk
Sun
Solaris

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1095683760.20951.gz

I tried gcc 3.4.2 on S8 but it doesn't have problem. Is this a problem of gcc on
that box?
(Reporter)

Comment 1

14 years ago
I suggest to pass char* to the function to see if it can resolve the problem. 

Comment 2

14 years ago
I have zero clue why the two tinderboxen fail. It seems the bustage is
Solaris+gcc only - I cannot reproduce the problem on Suse Linux/gcc nor with
Solaris+Sun Workshop compiler...

pkw: Do you have any idas (see
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports, tinderbox
machines "worms" and "speedracer" - they fail at |munmap()|) what is going wrong
there ?
OS: Windows 2000 → Solaris
Hardware: PC → Sun

Comment 3

14 years ago
A side note: mmap doesn't return NULL on failure, it returns -1, yet the code is
checking for a 0 return.  Was this code reviewed?

How about running c++ -E ... on the file to see what those lines look like after
preprocessing?  In C no casting would be required since the prototypes already
specify (void *) so any valid pointer would work.  I have no idea what the C++
rules are (nor do I want to know).
(Reporter)

Comment 4

14 years ago
(In reply to comment #2)
> I have zero clue why the two tinderboxen fail. It seems the bustage is
> Solaris+gcc only - I cannot reproduce the problem on Suse Linux/gcc 
But gcc3.4.2 on S8 sparc works fine for me. Maybe there are some compile
parameter like CFLAGs or CXXFLAGS will cause this problem? Just guess.

Comment 5

14 years ago
On solaris 8 at least, depending on which conformance macros are set, mmap() can
be declared to return a caddr_t instead of a void*. The following is the
relevant section of <sys/mman.h>:

#if (_POSIX_C_SOURCE > 2) || defined(_XPG4_2)
extern void *mmap(void *, size_t, int, int, int, off_t);
extern int munmap(void *, size_t);
extern int mprotect(void *, size_t, int);
extern int msync(void *, size_t, int);
#if (!defined(_XPG4_2) || (_POSIX_C_SOURCE > 2)) || defined(__EXTENSIONS__)
extern int mlock(const void *, size_t);
extern int munlock(const void *, size_t);
extern int shm_open(const char *, int, mode_t);
extern int shm_unlink(const char *);
#endif	/* (!defined(_XPG4_2) || (_POSIX_C_SOURCE > 2))... */
/* transitional large file interface version */
#if	defined(_LARGEFILE64_SOURCE) && !((_FILE_OFFSET_BITS == 64) && \
            !defined(__PRAGMA_REDEFINE_EXTNAME))
extern void *mmap64(void *, size_t, int, int, int, off64_t);
#endif	/* _LARGEFILE64_SOURCE... */
#else	/* (_POSIX_C_SOURCE > 2) || defined(_XPG4_2) */
extern caddr_t mmap(caddr_t, size_t, int, int, int, off_t);
extern int munmap(caddr_t, size_t);
extern int mprotect(caddr_t, size_t, int);
extern int msync(caddr_t, size_t, int);
extern int mlock(caddr_t, size_t);
extern int munlock(caddr_t, size_t);
extern int mincore(caddr_t, size_t, char *);
extern int memcntl(caddr_t, size_t, int, caddr_t, int, int);
extern int madvise(caddr_t, size_t, int);
/* transitional large file interface version */
#ifdef	_LARGEFILE64_SOURCE
extern caddr_t mmap64(caddr_t, size_t, int, int, int, off64_t);
#endif
#endif	/* (_POSIX_C_SOURCE > 2)  || defined(_XPG4_2) */

At my office, I regularly compile mozilla using gcc 2.95 on solaris 8/sparc, and
this problem bit me this morning. Examining the postprocessed source confirms
the caddr_t declarations are being used in this case. Sprinkling (caddr_t) casts
in all the right places fixed the errors, but I haven't worked out a portable fix.

Comment 6

14 years ago
Seeing the bustage as well with Forte compilers (CC: Sun C++ 5.5 Patch 113817-06
2004/01/29) on S10.  Same problem as worms seems to be having; that is, it dies
in nsRenderingContextPS.cpp.  It's a firefox build, too.
(Reporter)

Comment 7

14 years ago
Created attachment 159548 [details] [diff] [review]
patch to fix it

Comment 8

14 years ago
Why not just make data a non-const char *, and dispense with the casting
entirely?  Does making it const give you anything here?

There's also Greg's point about mmap() not returning 0 on failure, but
MMAP_FAILED ((void *)-1) instead.  That's almost certainly a crash waiting to
happen.

And using fstat() would probably be faster than fseek()/ftell(), but I don't
know if the latter pair has important side-effects.
(Reporter)

Comment 9

14 years ago
Created attachment 159558 [details] [diff] [review]
patch2
Attachment #159558 - Flags: review+

Comment 10

14 years ago
Is it really not possible to compare data against MAP_FAILED?  That's what the
interface specifies; the fact that it happens to equal -1 when cast to an
integer is coincidence.  All this casting and hardcoding of numbers just
produces ugly, unmaintainable code.

That said, I'm not maintaining it, and it would appear to be in a working state,
so I won't squawk no more.
(Reporter)

Comment 11

14 years ago
checked in the second patch(In reply to comment #10)
> Is it really not possible to compare data against MAP_FAILED?  That's what the
> interface specifies; the fact that it happens to equal -1 when cast to an
> integer is coincidence.  All this casting and hardcoding of numbers just
> produces ugly, unmaintainable code.
We will fix it in another patch. This patch is for bustage. Roland should have
more idea about if we should use MAP_FAILED since he made the orignial patch
(Reporter)

Comment 12

14 years ago
checked in and the SunOS boxes should be fine soon.
(Reporter)

Updated

14 years ago
Attachment #159548 - Attachment is obsolete: true
(Reporter)

Comment 13

14 years ago
->FIXED
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.