Closed
Bug 1411625
Opened 7 years ago
Closed 7 years ago
-Wclass-memaccess: clearing an object of non-trivial type 'class gfxShapedText::CompressedGlyph'
Categories
(Core :: Graphics, defect, P3)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla58
People
(Reporter: Sylvestre, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 1 obsolete file)
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/gfxFont.h: In constructor 'gfxShapedWord::gfxShapedWord(const char16_t*, uint32_t, gfxShapedWord::Script, int32_t, mozilla::gfx::ShapedTextFlags, gfxFontShaper::RoundingFlags)': /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/gfxFont.h:1376:72: error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'class gfxShapedText::CompressedGlyph'; use assignment or value-initialization instead [-Werror=class-memaccess] memset(mCharGlyphsStorage, 0, aLength * sizeof(CompressedGlyph)); ^ /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/gfxFont.h:762:11: note: 'class gfxShapedText::CompressedGlyph' declared here class CompressedGlyph {
Reporter | ||
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Hmm.... why exactly is this considered an error? The type gfxShapedText::CompressedGlyph has no virtual methods or destructor, and just a single uint32_t data member; is it not safe to clear an array of them by a simple memset()? Will the compiler promise to generate equally efficient code if we rewrite this to explicitly assign or construct them all?
Reporter | ||
Comment 2•7 years ago
|
||
This is just a warning but as we are building with -Werror (and we can build this option), I am reporting this warning. We can always ignore it!
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #1) > Hmm.... why exactly is this considered an error? The type > gfxShapedText::CompressedGlyph has no virtual methods or destructor, and > just a single uint32_t data member; is it not safe to clear an array of them > by a simple memset()? Will the compiler promise to generate equally > efficient code if we rewrite this to explicitly assign or construct them all? It's because of this: >>CompressedGlyph() { mValue = 0; } Having a non-trivial ctor implies that the class is non-trivial -> http://en.cppreference.com/w/cpp/concept/TrivialType
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bpostelnicu
Updated•7 years ago
|
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8923759 [details] Bug 1411625 - avoid using memset on CompressedGlyph. https://reviewboard.mozilla.org/r/194916/#review199974 If this is a concern (TBH, it's still not clear to me whether there's a genuine issue -- e.g. the current code results in strictly undefined behavior -- or just an over-eager compiler warning because it's not sure the code is OK even though we can tell that it is), then shouldn't we also be concerned about the similar behavior in gfxTextRun::Create? There, a block of memory is allocated, and part of it memset() to zero (in AllocateStorageForTextRun); then the object is constructed in that storage (with placement-new), and the zeroed area is reinterpret_cast<>ed to be used as an array of CompressedGlyph records. So fundamentally the same thing is happening: we've zeroed the storage for a block of CompressedGlyph records using memset. ::: gfx/thebes/gfxFont.h:1363 (Diff revision 2) > - memset(mCharGlyphsStorage, 0, aLength * sizeof(CompressedGlyph)); > + for (size_t index = 0; index < aLength; index++) > + mCharGlyphsStorage[index].Reset(); If we do this, it needs braces (per Gecko style guide). What does the compiled code end up like here (for the various compilers we use)? I'd be reluctant to take it unless the compiler successfully optimizes it just as well as the memset() call.
Assignee | ||
Comment 7•7 years ago
|
||
Yes I see your point, the fact is that the code introduced by this patch will never be the same with the original one, despite the optimisation level that we've used. I did a short test on clang 5 with and without memset call, and difference in resulted code was pretty substantial, several extra calls. Looking on this strict particular issue, I think the compiler is a bit to strict. Another option would be to remove the ctor and default init all of the instances of CompressedGlyph with {}, this will assure us that the only member variable is init with 0. If we do this a problem remains when we build an rvalue and we use it: https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h?q=CompressedGlyph%28%29.&redirect_type=single#1088 In that context we're safe since we don't assume that mValue was init with a predefined value, but what if in the future we add functionality thinking that mValue is somewhere 0'ed. If you think this solution is acceptable I'll be more than happy to push a patch.
Comment 8•7 years ago
|
||
That sounds like it should be OK; let's try a patch like that and see how it looks, I think.
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8923759 [details] Bug 1411625 - avoid using memset on CompressedGlyph. https://reviewboard.mozilla.org/r/194916/#review201916 I think you missed an instance at http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/gfx/thebes/gfxFont.cpp#576. However, on looking at this again, I think I'd like to propose an alternative. I notice that in every case where you're initializing a CompressedGlyph variable with {}, we then proceed immediately to call SetComplex() or (in one case) SetSimpleGlyph() on it, and then assign the result to a location in the gfxShapedText's storage. I think this pattern could be simpler if we have a couple of static CompressedGlyph "factory" methods that take the same arguments as SetComplex and SetSimpleGlyph, and return a CompressedGlyph value directly initialized from those arguments. The Set{Complex,SimpleGlyph} methods can then be refactored to use these helpers, combining the result with the additional flags that may be present in the object. I'll put up a patch using this approach for you to see what you think. ::: commit-message-51540:1 (Diff revision 3) > +Bug 1411625 - avoid using memset on CompressedGlyph. r?jfkthame The commit message would need updating, as this is no longer the change that is being made here.
Comment 11•7 years ago
|
||
AIUI, this should make the compiler happy, and seems a tidier solution to me. WDYT?
Attachment #8925618 -
Flags: review?(bpostelnicu)
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8925618 [details] [diff] [review] Remove the constructor from gfxShapedText::CompressedGlyph to make it a trivial class, and provide a couple of convenience "factory" methods to create simple and complex glyph values This looks very good!
Attachment #8925618 -
Flags: review?(bpostelnicu) → review+
Comment 13•7 years ago
|
||
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2dc5d006fc7 Remove the constructor from gfxShapedText::CompressedGlyph to make it a trivial class, and provide a couple of convenience "factory" methods to create simple and complex glyph values. r=andi
Updated•7 years ago
|
Attachment #8923759 -
Attachment is obsolete: true
Attachment #8923759 -
Flags: review?(jfkthame)
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e2dc5d006fc7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•