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

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

/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));

Comment 1

[Andi [:andi]]

We should init the memebers in the ctor in order to land this: 525063

Comment 2

[Andi [:andi]]

Attachment: Bug 1411630 - make mozilla::gfx::VRHMDSensorState to be trivial typed.

Review commit: https://reviewboard.mozilla.org/r/193334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193334/

Comment 3

[:kip (Kearwood Gilbert)]

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, "");

Comment 4

[Andi [:andi]]

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));
>>}

Comment 5

[Andi [:andi]]

Comment on attachment 8922276 [details]
Bug 1411630 - make mozilla::gfx::VRHMDSensorState to be trivial typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193334/diff/1-2/

Comment 6

[:kip (Kearwood Gilbert)]

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.

Comment 7

[Andi [:andi]]

Comment on attachment 8922276 [details]
Bug 1411630 - make mozilla::gfx::VRHMDSensorState to be trivial typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193334/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8743cbd9049
make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip

Comment 9

[Cristina Coroiu [:ccoroiu]]

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

Comment 10

[Andi [:andi]]

Comment on attachment 8922276 [details]
Bug 1411630 - make mozilla::gfx::VRHMDSensorState to be trivial typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193334/diff/3-4/

Comment 11

[Andi [:andi]]

I've update the patch and started a try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=902e46e171d74ac9ea958e6e0bb1f98258c824f9

Comment 12

[Andi [:andi]]

Looking at the results from treeherder, the updated patch fixed the issue.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90a9bac24763
make mozilla::gfx::VRHMDSensorState to be trivial typed. r=kip

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/90a9bac24763