666989 - Shmem.cpp:357:8: warning: variable ‘checkMappingFront’ set but not used [-Wunused-but-set-variable]

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Daniel Holbert [:dholbert]]

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]]

Attachment: 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]

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]]

Attachment: fix v2: unused <<

Nice -- I didn't realize we had that!

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ad79a2f23f2f

Comment 6

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ad79a2f23f2f

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

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]]

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)]

Changing status to VERIFIED-FIXED based on comment 8