-Wclass-memaccess: clearing an object of non-trivial type 'struct mozilla::gl::GLContextSymbols'

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: sylvestre, Assigned: jgilbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

In file included from /root/firefox-gcc-last/gfx/gl/GLContext.h:42:0,
                  from /root/firefox-gcc-last/gfx/gl/GLContextGLX.h:10,
                  from /root/firefox-gcc-last/gfx/gl/GLContextProviderGLX.cpp:33:
 /root/firefox-gcc-last/gfx/gl/GLContextSymbols.h: In member function 'void mozilla::gl::GLContextSymbols::Zero()':
 /root/firefox-gcc-last/gfx/gl/GLContextSymbols.h:35:49: error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'struct mozilla::gl::GLContextSymbols'; use assignment or value-initialization instead [-Werror=class-memaccess]
          memset(this, 0, sizeof(GLContextSymbols));
                                                  ^
 /root/firefox-gcc-last/gfx/gl/GLContextSymbols.h:28:8: note: 'struct mozilla::gl::GLContextSymbols' declared here
  struct GLContextSymbols
         ^~~~~~~~~~~~~~~~
 cc1plus: all warnings being treated as errors
 /root/firefox-gcc-last/config/rules.mk:1072: recipe for target 'GLContex
Assignee: nobody → bpostelnicu
I'm wondering if it would also be OK to initialize all of the members in ctor without memset and to leave memset only in the reset. I'm asking this since we really want to integrate this: 525063
Comment on attachment 8922309 [details]
Bug 1411626 - don't use memset on mozilla::gl::GLContextSymbols since it's not a trivial type.

https://reviewboard.mozilla.org/r/193366/#review198820

Not like this.
Attachment #8922309 - Flags: review?(jgilbert) → review-
Which version of gcc is this from?
Flags: needinfo?(sledru)
This warning is spurious here, since we're within the lifetime of the object. (Both in the ctor or in a member function)
Assignee: bpostelnicu → jgilbert
I'm inferring that this is from GCC8-dev, which I don't trivially have available to test with.
I'm going to discourage you from trying to fix warnings from pre-release compilers, but I'm fine with at least this change.
Flags: needinfo?(sledru)
Attachment #8922309 - Attachment is obsolete: true
Comment on attachment 8922593 [details]
Bug 1411626 - Make GLContextSymbols a pure aggregate POD. -

https://reviewboard.mozilla.org/r/193706/#review199508

::: gfx/gl/GLContext.cpp:352
(Diff revision 1)
>  
>      ScopedGfxFeatureReporter reporter("GL Context");
>  
>      if (!InitWithPrefixImpl(prefix, trygl)) {
>          // If initialization fails, zero the symbols to avoid hard-to-understand bugs.
> -        mSymbols.Zero();
> +        mSymbols = {};

Aren't we doing here a byte copy of an rvalue object o mSymbols?
Why don't we keet the Zero function and call it here?
Since we've deleted the ctor this is a trivial class hence we will no longer have the issue with memset and gcc8.

::: gfx/gl/GLContext.cpp:2144
(Diff revision 1)
>  
>      if (!MakeCurrent()) {
>          NS_WARNING("MakeCurrent() failed during MarkDestroyed! Skipping GL object teardown.");
>      }
>  
> -    mSymbols.Zero();
> +    mSymbols = {};

Same here as above.
Comment on attachment 8922593 [details]
Bug 1411626 - Make GLContextSymbols a pure aggregate POD. -

https://reviewboard.mozilla.org/r/193706/#review199508

> Aren't we doing here a byte copy of an rvalue object o mSymbols?
> Why don't we keet the Zero function and call it here?
> Since we've deleted the ctor this is a trivial class hence we will no longer have the issue with memset and gcc8.

The optimizer should pick up on this, and this isn't perf-sensitive anyway.
Comment on attachment 8922593 [details]
Bug 1411626 - Make GLContextSymbols a pure aggregate POD. -

https://reviewboard.mozilla.org/r/193706/#review199720
Attachment #8922593 - Flags: review?(bpostelnicu) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdf605ea0f4
Make GLContextSymbols a pure aggregate POD. - r=andi
https://hg.mozilla.org/mozilla-central/rev/9cdf605ea0f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1495892
You need to log in before you can comment on or make changes to this bug.