Closed Bug 1119403 Opened 10 years ago Closed 10 years ago

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]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: bkelly, Assigned: cpeterson)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Guessing this is from bug 1118149.
Blocks: 1118149
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.)
Depends on: 1061664
Flags: needinfo?(bkelly)
Yep, mozilla::unused fixes it for me. Really weird.
Flags: needinfo?(bkelly)
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.
Attached patch part-1-fix-gcc-warn_unused_result.patch (obsolete) (deleted) — Splinter Review
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. :)
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Attachment #8546899 - Flags: review?(luke)
Attached patch part-2-use-rand_s-error.patch (obsolete) (deleted) — Splinter Review
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.
Attachment #8546907 - Flags: review?(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 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.
(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.
Attached patch suppress-unused-result-v2.patch (deleted) — Splinter Review
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()
Attachment #8546899 - Attachment is obsolete: true
Attachment #8546907 - Attachment is obsolete: true
Attachment #8546899 - Flags: review?(luke)
Attachment #8546907 - Flags: review?(luke)
Attachment #8547686 - Flags: review?(luke)
Attachment #8547686 - Flags: review?(luke) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1140806
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: