260525 - bustage for Solaris gcc box

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Pete Zha]

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?

Comment 1

[Pete Zha]

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

Comment 2

[Roland Mainz]

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 ?

Comment 3

[Greg Onufer]

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).

Comment 4

[Pete Zha]

(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

[Kenneth Herron]

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

[Danek Duvall]

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.

Comment 7

[Pete Zha]

Attachment: patch to fix it

Comment 8

[Danek Duvall]

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.

Comment 9

[Pete Zha]

Attachment: patch2

Comment 10

[Danek Duvall]

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.

Comment 11

[Pete Zha]

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

Comment 12

[Pete Zha]

checked in and the SunOS boxes should be fine soon.

Comment 13

[Pete Zha]

->FIXED