Sylvestre Ledru [:Sylvestre] Reporter
	Description • 7 years ago

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

	Andi [:andi]
	Comment 2 • 7 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 4 • 7 years ago
mozreview-review

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 5 • 7 years ago

Which version of gcc is this from?

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 6 • 7 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 8 • 7 years ago

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.

	Andi [:andi]
	Comment 9 • 7 years ago
mozreview-review

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

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.

	Andi [:andi]
	Comment 11 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 12 • 7 years ago

Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdf605ea0f4
Make GLContextSymbols a pure aggregate POD. - r=andi

	Eliza Balazs [:ebalazs_]
	Comment 13 • 7 years ago
bugherder

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