Warning treated as error in jemalloc.c. Build busted.

RESOLVED INCOMPLETE

Status

Firefox Build System
General
--
blocker
RESOLVED INCOMPLETE
6 years ago
3 months ago

People

(Reporter: Bengt-Erik Soderstrom, Assigned: fabrice)

Tracking

Trunk
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
gcc -o jemalloc.o -c  -DMOZ_GLUE_IN_PROGRAM  -I/home/bengt-erik/mozilla-central/src/memory/jemalloc -I. -I../../dist/include -I../../dist/include/nsprpub  -I/home/bengt-erik/mozilla-central/src/ff-opt/dist/include/nspr -I/home/bengt-erik/mozilla-central/src/ff-opt/dist/include/nss      -fPIC  -pedantic -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Werror=unused-result -Wno-unused -Wno-overlength-strings -Wcast-align -Wno-long-long -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fomit-frame-pointer   -include ../../mozilla-config.h -DMOZILLA_CLIENT -MD -MF .deps/jemalloc.pp /home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c
/home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c: I funktion "wrtmessage":
/home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c:1530:8: fel: ignorerar returvärdet av "write", deklarerad med attributet warn_unused_result [-Werror=unused-result]
/home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c:1531:8: fel: ignorerar returvärdet av "write", deklarerad med attributet warn_unused_result [-Werror=unused-result]
/home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c:1532:8: fel: ignorerar returvärdet av "write", deklarerad med attributet warn_unused_result [-Werror=unused-result]
/home/bengt-erik/mozilla-central/src/memory/jemalloc/jemalloc.c:1533:8: fel: ignorerar returvärdet av "write", deklarerad med attributet warn_unused_result [-Werror=unused-result]
cc1: some warnings being treated as errors
make[5]: *** [jemalloc.o] Fel 1 (=Error 1)

Short tranlation: jemalloc.c: Ignors return value of "write", declared with attribute warn_unused_result [-Werror=unused-result]

This did not happen one/two days ago. Also Seamonkey is affected.

Using Ubuntu 12.04 and gcc-4.6.real (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3
(Reporter)

Updated

6 years ago
Version: unspecified → Trunk

Updated

6 years ago
Blocks: 736501
(Assignee)

Comment 1

6 years ago
Created attachment 623083 [details] [diff] [review]
patch

I'm not sure who should review this!
Assignee: nobody → fabrice
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 years ago
Comment on attachment 623083 [details] [diff] [review]
patch

According to file revisions for memory/jemalloc/jemalloc.c, jlebar can review this patch.
Attachment #623083 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 3

6 years ago
Created attachment 623089 [details] [diff] [review]
better patch

This patch also has fixes for bug 754160 and bug 754171
Attachment #623083 - Attachment is obsolete: true
Attachment #623083 - Flags: review?(justin.lebar+bug)
Attachment #623089 - Flags: review?(benjamin)

Updated

6 years ago
Component: Build Config → Build Config
Product: Firefox → Core
QA Contact: build.config → build-config
(Assignee)

Comment 4

6 years ago
Created attachment 623093 [details] [diff] [review]
patch v3

Updated with all the fixes needed to compile on Ubuntu.
Attachment #623089 - Attachment is obsolete: true
Attachment #623089 - Flags: review?(benjamin)
Attachment #623093 - Flags: review?(justin.lebar+bug)
Attachment #623093 - Flags: review?(benjamin)

Comment 5

6 years ago
Comment on attachment 623093 [details] [diff] [review]
patch v3

In several cases here we should actually be handling the error instead of ignoring it. Also in C++ code we have settled on the mozilla::unused pattern.

>diff --git a/config/elf-dynstr-gc.c b/config/elf-dynstr-gc.c
>--- a/config/elf-dynstr-gc.c
>+++ b/config/elf-dynstr-gc.c
>@@ -1234,7 +1234,7 @@ main(int argc, char *argv[])
>   
>   munmap(mapping, size);
> 
>-  ftruncate(fd, size - hole_len);
>+  int unused = ftruncate(fd, size - hole_len);

We should definitely "return 1" if this fails instead of ignoring it.

>diff --git a/dom/plugins/test/testplugin/nptest.cpp b/dom/plugins/test/testplugin/nptest.cpp
>--- a/dom/plugins/test/testplugin/nptest.cpp
>+++ b/dom/plugins/test/testplugin/nptest.cpp
>@@ -1359,7 +1359,7 @@ NPP_StreamAsFile(NPP instance, NPStream*
>     instanceData->fileBuf = malloc((int32_t)size + 1);
>     char* buf = reinterpret_cast<char *>(instanceData->fileBuf);
>     fseek(file, 0, SEEK_SET);
>-    fread(instanceData->fileBuf, 1, size, file);
>+    size_t unused = fread(instanceData->fileBuf, 1, size, file);
>     fclose(file);
>     buf[size] = '\0';
>     instanceData->fileBufSize = (int32_t)size;

I'm torn on this one, the error handling in the testplugin is pretty poor in general and this isn't likely to fail, so I suppose unused << is ok. But please add it to fseek also.

>diff --git a/modules/libmar/tool/mar.c b/modules/libmar/tool/mar.c
>--- a/modules/libmar/tool/mar.c
>+++ b/modules/libmar/tool/mar.c
>@@ -126,7 +126,7 @@ int main(int argc, char **argv) {
>       break;
>     /* -C workingdirectory */
>     } else if (argv[1][0] == '-' && argv[1][1] == 'C') {
>-      chdir(argv[2]);
>+      int unused = chdir(argv[2]);

Definitely return -1 if this fails.

>diff --git a/xpcom/base/nsCycleCollector.cpp b/xpcom/base/nsCycleCollector.cpp
>--- a/xpcom/base/nsCycleCollector.cpp
>+++ b/xpcom/base/nsCycleCollector.cpp
>@@ -1366,7 +1366,7 @@ public:
>         // Therefore we need to call the APIs directly.
>         GetTempPathA(mozilla::ArrayLength(basename), basename);
> #else
>-        tmpnam(basename);
>+        char *tmp = tmpnam(basename);

It definitely seems like we ought to be error-checking this. tempnam can fail.

In most of the other cases unused << is correct.
Attachment #623093 - Flags: review?(benjamin) → review-
Can you use |(void)_write(foo)| instead of |int res|? r=me on the jemalloc changes with that change.

Once we land jemalloc as a third-party library (bug 580408), we'll want to use its build flags -- if there are warnings upstream, I don't want to patch jemalloc just to fix the warnings.
Attachment #623093 - Flags: review?(justin.lebar+bug) → review+

Comment 7

6 years ago
No, you cannot use (void) _write because that doesn't suppress the warning.
Bummer.  Whatever we need to do to fix it is fine, since we're throwing this out soon anyway.
(Assignee)

Comment 9

6 years ago
Created attachment 623214 [details] [diff] [review]
patch v4

Addressed comments except for nptest.cpp where including mozilla/unused.h lead to linking errors related to moz_malloc and friends. I can dig deeper if we really want to, but Benjamin comment lead me to think the current solution is acceptable.
Attachment #623093 - Attachment is obsolete: true
Attachment #623214 - Flags: review?(benjamin)

Updated

6 years ago
Attachment #623214 - Flags: review?(benjamin) → review+
Sorry, I backed this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04f69f8d207c

because it broke the build on Win64:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11684318&tree=Mozilla-Inbound
e:/builds/moz2_slave/m-in-w64/build/dom/plugins/test/testplugin/nptest.cpp(1357) : error C2065: 'ssize_t' : undeclared identifier
ssize_t is defined in BaseTsd.h IIRC.

Updated

6 years ago
Severity: normal → blocker
The correct course of action here is to back out Bug 736501, and reland it when this issue is fixed. r=me to do that.
Please also fix tools/trace-malloc/ as reported in bug 754160.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #14)
> The correct course of action here is to back out Bug 736501, and reland it
> when this issue is fixed. r=me to do that.

Done.
(Reporter)

Comment 17

6 years ago
Thank you Matt, it certainly worked well for Firefox Nightly. Unfortunately I still get a build error in Seamonkey. I am not sure it is related to this at all, I filed a new bug 754642 about the Seamonkey build error.
Like I said in bug 736501, I thought our plan was to make these warnings fatal only on the tinderboxen (unless the developer opts in in their mozconfig).  That way, nobody is blocked by this kind of thing.
Bug on old, unsupported toolchain. Likely not still relevant.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INCOMPLETE

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.