Closed
Bug 1411630
Opened 7 years ago
Closed 7 years ago
-Wclass-memaccess: clearing an object of non-trivial type 'struct mozilla::gfx::VRHMDSensorState'
Categories
(Core :: WebVR, defect)
Core
WebVR
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: Sylvestre, Assigned: andi)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
/root/firefox-gcc-last/gfx/vr/gfxVR.h: In member function 'void mozilla::gfx::VRHMDSensorState::Clear()': /root/firefox-gcc-last/gfx/vr/gfxVR.h:153:45: error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'struct mozilla::gfx::VRHMDSensorState'; use assignment or value-initialization instead [-Werror=class-memaccess] memset(this, 0, sizeof(VRHMDSensorState));
Reporter | ||
Updated•7 years ago
|
Component: Graphics → WebVR
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bpostelnicu
Assignee | ||
Comment 1•7 years ago
|
||
We should init the memebers in the ctor in order to land this: 525063
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
Upcoming work in Bug 1346927 will depend on this being trivially-copyable as a POD type. Would it be sufficient to remove the constructor and call Clear() at the place of initialization? Alternately, could we move the members into a contained POD struct and initialize it with the owner's constructor? Also, do we have an established static assert for enforcing that objects stay POD? ie. Could we do: static_assert(std::is_pod<VRHMDSensorState>::value, "");
Flags: needinfo?(bpostelnicu)
Assignee | ||
Comment 4•7 years ago
|
||
If we can remove the ctor is great! This relieves the bourdon of out static-analysis checker. What do you think that instead of calling Clear after initialization we use the new cpp11 0 initialization like: >> VRHMDSensorState result{}; >> >> ::vr::Compositor_FrameTiming timing; >> timing.m_nSize = sizeof(::vr::Compositor_FrameTiming); and >>VRDisplayPuppet::VRDisplayPuppet() >> : VRDisplayHost(VRDeviceType::Puppet) >> , mIsPresenting(false) >> , mSensorState{} >>{ >> MOZ_COUNT_CTOR_INHERITED(VRDisplayPuppet, VRDisplayHost); and for array init we do the memset: >> mDisplayInfo.mPresentingGroups = 0; >> mDisplayInfo.mGroupMask = kVRGroupContent; >> mDisplayInfo.mFrameId = 0; >> memset( >> &mDisplayInfo.mLastSensorState, 0, sizeof(mDisplayInfo.mLastSensorState)); >>}
Flags: needinfo?(bpostelnicu)
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8922276 [details] Bug 1411630 - make mozilla::gfx::VRHMDSensorState to be trivial typed. https://reviewboard.mozilla.org/r/193334/#review199178 This looks good, but there are a couple other places where VRHMDSensorState is initialized: gfxVROculus.cpp: VRDisplayOculus::GetSensorState() VRDisplayOculus::GetSensorState(double absTime) gfxVR.h: struct VRDisplayInfo's mLastSensorState member is an array of VRHMDSensorState's. VRDisplayInfo is intended to be another POD which we can zero out when initialized. We would need to do this in two places: VRDisplayHost.cpp: VRDisplayHost::mDisplayInfo and VRDisplayHost::mLastUpdateDisplayInfo should be zero'ed out at top of VRDisplayHost constructor VRServiceTest.cpp: VRMockDisplay::mDisplayInfo should be zero'ed out in the VRMockDisplay constructor. r=me with the additional zero'ing out in these places.
Attachment #8922276 -
Flags: review?(kgilbert) → review+
Comment hidden (mozreview-request) |
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8743cbd9049 make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip
Comment 9•7 years ago
|
||
Backed out changeset b8743cbd9049 (bug 1411630) for failing on OS X and Windows 10 reftests in dom/vr/test/reftest/change_size.html r=backout a=backout on a CLOSED TREE https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b8743cbd9049ad5ecf852035cd89b1103bcd5c95&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable https://hg.mozilla.org/integration/autoland/rev/93d170c6bbbc52634bd42a2d5c99e9b402daf0d9
Flags: needinfo?(bpostelnicu)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
I've update the patch and started a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=902e46e171d74ac9ea958e6e0bb1f98258c824f9
Flags: needinfo?(bpostelnicu)
Assignee | ||
Comment 12•7 years ago
|
||
Looking at the results from treeherder, the updated patch fixed the issue.
Comment 13•7 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90a9bac24763 make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip
![]() |
||
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/90a9bac24763
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•