1119403 - js/src/jsmath.cpp:740:63: error: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

I updated mozilla-central to d480b3542cc2 today and can no longer build.  I get this error:

  js/src/jsmath.cpp:740:63: error: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]

I am using this compiler:

  gcc (Ubuntu/Linaro 4.7.3-2ubuntu1~12.04) 4.7.3

From what I understand tinderbox uses an old version of glibc which prevents it from catch unused-result errors.  This is pretty terrible for end developers, though, since the build keeps breaking.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Guessing this is from bug 1118149.

Comment 2

[Daniel Holbert [:dholbert]]

Line 740 is:
> 740         (void)read(fd, seed.u8, mozilla::ArrayLength(seed.u8));
https://mxr.mozilla.org/mozilla-central/source/js/src/jsmath.cpp?rev=65e82280a4be#740

I'd expect the "(void)" there should already be "using" the value.  That was added a few months ago in bug 1061664, presumably to fix this exact build warning.  Strange that it's not working in your case.

Ben, does the warning go away if you replace the "(void)" with "mozilla::unused <<"?  (You'll need to add #include "mozilla/unused.h" to this file, too.)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Yep, mozilla::unused fixes it for me.  Really weird.

Comment 4

[Chris Peterson [:cpeterson]]

bkelly: I will post a patch later (unless someone beats me to it :). You should be able to build by adding "ac_add_options --disable-warnings-as-errors" to your mozconfig.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-1-fix-gcc-warn_unused_result.patch

Part 1: Use read()'s return value to fix -Wunused-result warning in jsmath.cpp's PRNG.

Luke: I'm sending this review to you because you reviewed this block of code for me last time. :)

gcc is complaining that read()'s return value, marked with the warn_unused_result attribute in *some* versions of glibc, is unused. gcc takes its warn_unused_result attribute very seriously and won't allow a (void) cast suppress the warning.

This patch "uses" the return value by mixing it into the random seed bits. 99+% of the time the return value will 8. If we are unable to read /dev/urandom or get a short read, then the return value will be some other value -1 <= x < 8, providing some desperate scrap of entropy. :)

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-2-use-rand_s-error.patch

Part 2: Use rand_s()'s error return value in jsmath.cpp's PRNG.

If we're overengineering random_generateSeed() with an error check for read(), we might as well add an error check to the Windows code path. Microsoft's rand_s() function returns 0 on success or EINVAL or an undocumented error code on failure.

Also, PRMJ_Now() returns 64-bit time in microseconds. There's no need to throw away the high bits by stuffing the 64-bit time into seed.u32[1] instead of seed.u64.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8546899 [details] [diff] [review]
part-1-fix-gcc-warn_unused_result.patch

Review of attachment 8546899 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsmath.cpp
@@ +738,5 @@
>      MOZ_ASSERT(fd >= 0, "Can't open /dev/urandom");
>      if (fd >= 0) {
> +        ssize_t nread = read(fd, seed.u8, mozilla::ArrayLength(seed.u8));
> +        MOZ_ASSERT(nread == 8, "Can't read /dev/urandom");
> +        seed.u32[1] ^= nread;

I think 'unused << read(...);' would be the more standard Mozilla way to do this.  (#include "mozilla/unused.h")

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8546907 [details] [diff] [review]
part-2-use-rand_s-error.patch

Review of attachment 8546907 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsmath.cpp
@@ +719,5 @@
> +{
> +    uint32_t u32;
> +    errno_t error = rand_s(&u32);
> +    MOZ_ASSERT(error == 0, "rand_s() error");
> +    return u32 ^ error;

Again, it seems a bit strange to mix in the return value of the random number generator.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Luke Wagner [:luke] from comment #8)
> Again, it seems a bit strange to mix in the return value of the random
> number generator.

The idea was that if read() or rand_s() fail to return random bits, mixing in the error return value provides a small amount of randomness.

But I can suppress the warning with 'unused << read()' if you prefer.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: suppress-unused-result-v2.patch

patch v2:
* add 'unused << read(...)'
* add assertions for "can't happen" read() and rand_s() errors
* call rand_s() twice to fill entire 64-bit buffer
* remove mixing of error code bits from open(), read(), and rand_s()

Comment 11

[Chris Peterson [:cpeterson]]

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/422367d84ec6

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like: 
https://treeherder.mozilla.org/logviewer.html#?job_id=5388488&repo=mozilla-inbound

Comment 13

[Chris Peterson [:cpeterson]]

Landing again with build fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c63c749f2ca

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c63c749f2ca