1411625 - -Wclass-memaccess: clearing an object of non-trivial type 'class gfxShapedText::CompressedGlyph'

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

/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 {

Comment 1

[Jonathan Kew [:jfkthame]]

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?

Comment 2

[Sylvestre Ledru [:Sylvestre]]

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!

Comment 3

[Andi [:andi]]

(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

Comment 4

[Andi [:andi]]

Attachment: Bug 1411625 - avoid using memset on CompressedGlyph.

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

Comment 5

[Andi [:andi]]

Comment on attachment 8923759 [details]
Bug 1411625 - avoid using memset on CompressedGlyph.

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

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

[Andi [:andi]]

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

[Jonathan Kew [:jfkthame]]

That sounds like it should be OK; let's try a patch like that and see how it looks, I think.

Comment 9

[Andi [:andi]]

Comment on attachment 8923759 [details]
Bug 1411625 - avoid using memset on CompressedGlyph.

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

Comment 10

[Jonathan Kew [:jfkthame]]

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

[Jonathan Kew [:jfkthame]]

Attachment: 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

AIUI, this should make the compiler happy, and seems a tidier solution to me. WDYT?

Comment 12

[Andi [:andi]]

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!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e2dc5d006fc7