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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

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

Comment 1

[Andi [:andi]]

Attachment: Bug 1411626 - don't use memset on mozilla::gl::GLContextSymbols since it's not a trivial type.

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

Comment 2

[Andi [:andi]]

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 3

[Andi [:andi]]

Comment on attachment 8922309 [details]
Bug 1411626 - don't use memset on mozilla::gl::GLContextSymbols since it's not a trivial type.

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

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.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Which version of gcc is this from?

Comment 6

[Kelsey Gilbert [:jgilbert]]

This warning is spurious here, since we're within the lifetime of the object. (Both in the ctor or in a member function)

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1411626 - Make GLContextSymbols a pure aggregate POD. -

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

Comment 8

[Kelsey Gilbert [: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.

Comment 9

[Andi [:andi]]

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 10

[Kelsey Gilbert [:jgilbert]]

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 11

[Andi [:andi]]

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

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdf605ea0f4
Make GLContextSymbols a pure aggregate POD. - r=andi

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9cdf605ea0f4