The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla7

Status

()

Core
IPC
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla7
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning][inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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".
(Assignee)

Comment 1

6 years ago
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.
Attachment #541734 - Flags: review?(jones.chris.g)
Comment on attachment 541734 [details] [diff] [review]
fix v1: (void)

Please use |unused| for this (I love saying that!).  E.g. |unused << checkMappingFront;|.
(Assignee)

Comment 3

6 years ago
Created attachment 541778 [details] [diff] [review]
fix v2: unused <<

Nice -- I didn't realize we had that!
Assignee: nobody → dholbert
Attachment #541734 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #541778 - Flags: review?(jones.chris.g)
Attachment #541734 - Flags: review?(jones.chris.g)
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.
Attachment #541778 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad79a2f23f2f
Whiteboard: [build_warning][inbound]
http://hg.mozilla.org/mozilla-central/rev/ad79a2f23f2f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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!
(Assignee)

Comment 8

6 years ago
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.
Changing status to VERIFIED-FIXED based on comment 8
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.