Last Comment Bug 666989 - Shmem.cpp:357:8: warning: variable ‘checkMappingFront’ set but not used [-Wunused-but-set-variable]
: Shmem.cpp:357:8: warning: variable ‘checkMappingFront’ set but not used [-Wun...
Status: VERIFIED FIXED
[build_warning][inbound]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert]
:
: [PTO to Dec5] Bill McCloskey (:billm)
Mentors:
Depends on: 523174
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 11:23 PDT by Daniel Holbert [:dholbert]
Modified: 2011-08-31 23:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1: (void) (978 bytes, patch)
2011-06-24 11:39 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v2: unused << (1.35 KB, patch)
2011-06-24 13:39 PDT, Daniel Holbert [:dholbert]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-06-24 11:23:28 PDT
Filing bug on this GCC 4.6 warning:

ipc/glue/Shmem.cpp: In member function ‘void mozilla::ipc::Shmem::AssertInvariants() const’:
ipc/glue/Shmem.cpp:357:8: warning: variable ‘checkMappingFront’ set but not used [-Wunused-but-set-variable]

The flagged code is:

349 void
350 Shmem::AssertInvariants() const
351 {
352   NS_ABORT_IF_FALSE(mSegment, "NULL segment");
353   NS_ABORT_IF_FALSE(mData, "NULL data pointer");
354   NS_ABORT_IF_FALSE(mSize > 0, "invalid size");
355   // if the segment isn't owned by the current process, these will
356   // trigger SIGSEGV
357   char checkMappingFront = *reinterpret_cast<char*>(mData);
358   char checkMappingBack = *(reinterpret_cast<char*>(mData) + mSize - 1);
359   checkMappingFront = checkMappingBack; // avoid "unused" warnings
360 }

It looks like we're just assigning these checkMappingXXX vars to see if that triggers a segfault.

Line 359 is a hackaround for "unused" warnings in older GCC versions, but sadly, GCC 4.6 sees through it. :)

Perhaps we want to use a #pragma to selectively disable this warning for this block of code?

Or alternately, we could add:
  checkMappingBack = checkMappingFront;
at the end of this function, so that both variables are both written & read, being fully "used".
Comment 1 Daniel Holbert [:dholbert] 2011-06-24 11:39:39 PDT
Created attachment 541734 [details] [diff] [review]
fix v1: (void)

...or we can cast the variables to (void), which IIRC is the best way to "use" unused variables.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-24 12:47:43 PDT
Comment on attachment 541734 [details] [diff] [review]
fix v1: (void)

Please use |unused| for this (I love saying that!).  E.g. |unused << checkMappingFront;|.
Comment 3 Daniel Holbert [:dholbert] 2011-06-24 13:39:26 PDT
Created attachment 541778 [details] [diff] [review]
fix v2: unused <<

Nice -- I didn't realize we had that!
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-24 13:46:28 PDT
Comment on attachment 541778 [details] [diff] [review]
fix v2: unused <<

Looking back on this code, I see that |mData| actually should have been declared |volatile|, but that's a bigger patch and separate problem.
Comment 5 Daniel Holbert [:dholbert] 2011-06-24 14:08:36 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad79a2f23f2f
Comment 6 Marco Bonardo [::mak] 2011-06-25 03:31:41 PDT
http://hg.mozilla.org/mozilla-central/rev/ad79a2f23f2f
Comment 7 Mihaela Velimiroviciu (:mihaelav) 2011-08-31 07:09:46 PDT
Verified that file ipc/glue/Shmem.cpp was updated in mozilla-central repository.

Is this enough to set the satus to VERIFIED-FIXED or there are other tests that need to be run?

Thanks!
Comment 8 Daniel Holbert [:dholbert] 2011-08-31 08:45:36 PDT
Technically, you could do a Firefox build and verify that the warning's gone (or find the most recent linux "clobber" build on tinderbox and check its build log for the warning), but that's probably more work than a bug like this is worth.

So, I'd say that Comment 7 is sufficient in my book.
Comment 9 Mihaela Velimiroviciu (:mihaelav) 2011-08-31 23:45:18 PDT
Changing status to VERIFIED-FIXED based on comment 8

Note You need to log in before you can comment on or make changes to this bug.