Closed Bug 1411626 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Sylvestre, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.