Closed Bug 1411630 Opened 5 years ago Closed 5 years ago

-Wclass-memaccess: clearing an object of non-trivial type 'struct mozilla::gfx::VRHMDSensorState'

Categories

(Core :: WebVR, defect)

defect
Not set
normal

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));
Component: Graphics → WebVR
Assignee: nobody → bpostelnicu
We should init the memebers in the ctor in order to land this: 525063
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)
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 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+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8743cbd9049
make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip
I've update the patch and started a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=902e46e171d74ac9ea958e6e0bb1f98258c824f9
Flags: needinfo?(bpostelnicu)
Looking at the results from treeherder, the updated patch fixed the issue.
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90a9bac24763
make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip
https://hg.mozilla.org/mozilla-central/rev/90a9bac24763
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.