1419637 - PodZero throws lots of warnings on clearing an object of non-trivial type when using gcc 8

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In file included from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/HashTable.h:18:0,
                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/TracingAPI.h:12,
                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/GCPolicyAPI.h:47,
                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/js/GCVector.h:12,
                  from /home/allstars/src/gcc8/js/src/jscntxt.h:15,
                  from /home/allstars/src/gcc8/js/src/jit/JitFrames.h:12,
                  from /home/allstars/src/gcc8/js/src/jit/Bailouts.h:12,
                  from /home/allstars/src/gcc8/js/src/jit/Bailouts.cpp:7,
                  from /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/js/src/Unified_cpp_js_src9.cpp:2:
 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/mozilla/PodOperations.h: In instantiation of ‘void mozilla::PodZero(T*) [with T = js::jit::Address]’:
 /home/allstars/src/gcc8/js/src/jit/shared/Assembler-shared.h:301:38:   required from here
 /home/allstars/src/gcc8/obj-x86_64-pc-linux-gnu/dist/include/mozilla/PodOperations.h:55:9: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct js::jit::Address’; use assignment or value-initialization instead [-Wclass-memaccess]
    memset(aT, 0, sizeof(T));

Comment 1

[Andi [:andi]]

Te problem lies here:

>>struct Address
>>{
>>    Register base;
>>    int32_t offset;
>>
>>    Address(Register base, int32_t offset) : base(base), offset(offset)
>>    { }
>>
>>    Address() { mozilla::PodZero(this); }
>>};

The two ctors are the issue, having them makes the class not being trivial.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks, but there are many warnings, I just listed one of them.
Perhaps I should close this one, and fix each caller site of PodZero individually.

Comment 3

[Andi [:andi]]

I have a patch for this specific issue if you'd like.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> Te problem lies here:
> 
> >>struct Address
> >>{
> >>    Register base;
> >>    int32_t offset;
> >>
> >>    Address(Register base, int32_t offset) : base(base), offset(offset)
> >>    { }
> >>
> >>    Address() { mozilla::PodZero(this); }
> >>};
> 
> The two ctors are the issue, having them makes the class not being trivial.

Well, not really, the issue is that PodZero is a hidden way to do memset() and that the right thing here would be for base and offset to be initialized "manually" like:

Address() : base(), offset(0) {}

(Register has its own constructor with no argument too)

Comment 5

[Andi [:andi]]

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> > Te problem lies here:
> > 
> > >>struct Address
> > >>{
> > >>    Register base;
> > >>    int32_t offset;
> > >>
> > >>    Address(Register base, int32_t offset) : base(base), offset(offset)
> > >>    { }
> > >>
> > >>    Address() { mozilla::PodZero(this); }
> > >>};
> > 
> > The two ctors are the issue, having them makes the class not being trivial.
> 
> Well, not really, the issue is that PodZero is a hidden way to do memset()
> and that the right thing here would be for base and offset to be initialized
> "manually" like:
> 
> Address() : base(), offset(0) {}
> 
> (Register has its own constructor with no argument too)

Yep, without the ctors the objet would have been trivial, and yes again the correct fix is what you wrote. In general doing memset on this is very dangerous, specially in those cases where the class is polymorphic.

Comment 6

[Andi [:andi]]

Attachment: Bug 1419637 - do not call memset on a non-trivial type.

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

Comment 7

[Andi [:andi]]

This should also be added to:
>>struct BaseIndex

Comment 8

[Andi [:andi]]

Comment on attachment 8930862 [details]
Bug 1419637 - do not call memset on a non-trivial type.

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

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8930862 [details]
Bug 1419637 - do not call memset on a non-trivial type.

https://reviewboard.mozilla.org/r/201964/#review207472

Thanks!

Comment 10

[Jan de Mooij [:jandem]]

Actually, Register has this:

    Register() = default;

Doesn't that mean we should now initialize these Register fields explicitly to Register::Invalid() or something?

I wonder if we could do that in the Register constructor - probably not without breaking other stuff...

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8930862 [details]
Bug 1419637 - do not call memset on a non-trivial type.

Clearing r+ per comment 10.

Comment 12

[Andi [:andi]]

I will update the patch!

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #3)
> I have a patch for this specific issue if you'd like.

go ahead

Comment 14

[Andi [:andi]]

Comment on attachment 8930862 [details]
Bug 1419637 - do not call memset on a non-trivial type.

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

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8930862 [details]
Bug 1419637 - do not call memset on a non-trivial type.

https://reviewboard.mozilla.org/r/201964/#review207778

Great, thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da6568fa82df
do not call memset on a non-trivial type. r=jandem

Comment 17

[Tiberius Oros[:tiberius_oros]]

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