Closed Bug 1226907 Opened 4 years ago Closed 4 years ago

Fix warnings in mozjemalloc and remove ALLOW_COMPILER_WARNINGS

Categories

(Core :: Memory Allocator, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

1 clang warning on OS X:

> memory/mozjemalloc/jemalloc.c:438:9: error: '__DECONST' macro redefined [-Werror,-Wmacro-redefined]
> #define __DECONST(type, var) ((type)(uintptr_t)(const void *)(var))
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/sys/cdefs.h:327:9: note: previous definition is here
> #define __DECONST(type, var)    __CAST_AWAY_QUALIFIER(var, const, type)


1 gcc warning on Android:

> memory/mozjemalloc/jemalloc.c:6047:2: error: implicit declaration of function 'pthread_atfork' [-Werror=implicit-function-declaration]
> pthread_atfork(_malloc_prefork, _malloc_postfork, _malloc_postfork);

I would have liked to limit the forward declaration of pthread_atfork() to #if ANDROID_VERSION < 21 but the ANDROID_VERSION macro is not #defined in mozglue. There is a precedent for forward declaring pthread_atfork() in LogAlloc.cpp to suppress another -Wimplicit-function-declaration warning: 

https://hg.mozilla.org/mozilla-central/annotate/tip/memory/replace/logalloc/LogAlloc.cpp#l48


Fix 1 MSVC warning on Win64. reg_size is 64-bit size_t but 1U is 32-bit unsigned int:

> Warning: C4334 in memory\mozjemalloc\jemalloc.c: '<<' : result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)


Suppress 1 MSVC warning (originally reported in bug 558163):

> memory/mozjemalloc/jemalloc.c(6782) : warning C4273: '_recalloc' : inconsistent dll linkage
> memory/mozjemalloc/jemalloc.c(6809) : warning C4273: '_expand' : inconsistent dll linkage
> memory/mozjemalloc/jemalloc.c(6818) : warning C4273: '_msize' : inconsistent dll linkage
Attachment #8690454 - Flags: review?(mh+mozilla)
Have you tried this on try server? When I looked at this directory on Linux I saw warnings about unchecked return values from _write() and strerror_r()...
(In reply to Nicholas Nethercote [:njn] from comment #1)
> Have you tried this on try server? When I looked at this directory on Linux
> I saw warnings about unchecked return values from _write() and
> strerror_r()...

Yes. The Linux try builders successfully built this change [1]. The problem you see is that some newer Linux distros have system headers with warn_unused_result annotations, e.g. bug 1113669 and bug 1119403.

I don't have a Linux machine. If you send me a list of the mozjemalloc warnings you see (mach warnings-list | grep mozjemalloc), I can fix or suppress them.

Or I could exclude gcc from my moz.build change like:

  if CONFIG['GCC_CXX'] and not CONFIG['CLANG_CXX']:
    ALLOW_COMPILER_WARNINGS = True

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdcc1b7dfd1e
If it's passing on try server that's good enough for me. Thanks.
I think those warnings should be addressed somehow, either fixing them or allowing warnings for gcc. I don't want developers to have any excuse for building without --enable-warnings-as-errors locally just because they have newer distro version than the try servers. :)
Blocks: buildwarning
Attachment #8690454 - Flags: review?(mh+mozilla) → review+
Part 2: To breaking the builds of people who are building with newer glibc headers with the warn_unused_result annotations, check the return values from _write and strerror_r.
Attachment #8693150 - Flags: review?(n.nethercote)
Attachment #8693150 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/5ab8e51f00ce
https://hg.mozilla.org/mozilla-central/rev/dec5fee37b12
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Duplicate of this bug: 502904
Duplicate of this bug: 942833
You need to log in before you can comment on or make changes to this bug.