Closed Bug 1411030 Opened 7 years ago Closed 7 years ago

Fails with -Werror=class-memaccess on dom/BindingUtils.h

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: Sylvestre, Assigned: andi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1411030 - memset only the underlying vector from the Array container.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
0:47.63 In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ToJSValue.h:12:0,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/WindowBinding.h:14,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/ScrollbarStyles.h:12,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsPresContext.h:41,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/Element.h:28,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/PseudoElementHashEntry.h:10,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/EffectCompositor.h:13,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ServoBindings.h:18,
 0:47.63                  from /root/firefox-gcc-last/intl/locale/nsLanguageAtomService.cpp:14,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/intl/locale/Unified_cpp_intl_locale0.cpp:47:
 0:47.63 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h: In constructor 'mozilla::dom::ProtoAndIfaceCache::PageTableCache::PageTableCache()':
 0:47.63 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:482:40: error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'class mozilla::Array<mozilla::Array<JS::Heap<JSObject*>, 16>*, 97>'; use assignment or value-initialization instead [-Werror=class-memaccess]
 0:47.63        memset(&mPages, 0, sizeof(mPages));
 0:47.63                                         ^
 0:47.63 In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Span.h:23:0,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsTSubstring.h:16,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsAString.h:22,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsSubstring.h:10,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsString.h:14,
 0:47.63                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nsStringGlue.h:21,
 0:47.64                  from /root/firefox-gcc-last/intl/locale/DateTimeFormat.h:12,
 0:47.64                  from /root/firefox-gcc-last/intl/locale/DateTimeFormat.cpp:7,
 0:47.64                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/intl/locale/Unified_cpp_intl_locale0.cpp:2:
 0:47.64 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Array.h:22:7: note: 'class mozilla::Array<mozilla::Array<JS::Heap<JSObject*>, 16>*, 97>' declared here
 0:47.64  class Array
 0:47.64        ^~~~~
Here I don't think we should modify: Array() { memset(...) }, because if we do so we might end up in a similar scenario where we pass to Array first template argument as of a non TriviallyCopyable type that has undefined behavior for memset, memcpy etc.

In this particular case I think the best approach would be to do a loop-initialization something similar with the destructor of the class.
Comment on attachment 8921390 [details]
Bug 1411030 - memset only the underlying vector from the Array container.

https://reviewboard.mozilla.org/r/192422/#review197634

::: commit-message-ce1a8:1
(Diff revision 1)
> +Bug 1411030 - do not use memset, on a non trivially type. r?froydnj

"non trivially type" is not really a thing.  Please correct the commit message.

::: dom/bindings/BindingUtils.h:482
(Diff revision 1)
> -      memset(&mPages, 0, sizeof(mPages));
> +      for (size_t i = 0; i < ArrayLength(mPages); ++i) {
> +        mPages[i] = nullptr;
> +      }

I am not super-excited about this change; it's more code, possibly slower, and the compiler (in some cases) will just turn it back into a memset anyway!

Does the warning go away if you do:

`memset(mPages.begin(), 0, sizeof(mPages));`

instead?
Attachment #8921390 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8921390 [details]
> Bug 1411030 - do not use memset, on a non trivially type.
> 
> https://reviewboard.mozilla.org/r/192422/#review197634
> 
> ::: commit-message-ce1a8:1
> (Diff revision 1)
> > +Bug 1411030 - do not use memset, on a non trivially type. r?froydnj
> 
> "non trivially type" is not really a thing.  Please correct the commit
> message.
> 
> ::: dom/bindings/BindingUtils.h:482
> (Diff revision 1)
> > -      memset(&mPages, 0, sizeof(mPages));
> > +      for (size_t i = 0; i < ArrayLength(mPages); ++i) {
> > +        mPages[i] = nullptr;
> > +      }
> 
> I am not super-excited about this change; it's more code, possibly slower,
> and the compiler (in some cases) will just turn it back into a memset anyway!
> 
> Does the warning go away if you do:
> 
> `memset(mPages.begin(), 0, sizeof(mPages));`
> 
> instead?
It should work, since the underlying type is a T*.
Comment on attachment 8921390 [details]
Bug 1411030 - memset only the underlying vector from the Array container.

https://reviewboard.mozilla.org/r/192422/#review197654

Thanks!
Attachment #8921390 - Flags: review?(nfroyd) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a08737444fc3
memset only the underlying vector from the Array container. r=froydnj
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
https://hg.mozilla.org/mozilla-central/rev/a08737444fc3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee: nobody → bpostelnicu
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: