1152661 - Fix -Wuninitialized warnings about Operand member variables

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wuninitialized_Operand.patch

These -Wuninitialized warnings about Operand's (implicit) copy constructor copying uninitialized member variables are only reproducible in Linux gcc PGO builds, but they are real bugs:

js/src/jit/x86/MacroAssembler-x86.h:209:10: error: 'src.js::jit::Operand::index_' is used uninitialized in this function [-Werror=uninitialized]
js/src/jit/x86-shared/MoveEmitter-x86-shared.cpp:203:16: error: '<anonymous>.js::jit::Operand::disp_' may be used uninitialized in this function [-Werror=uninitialized]

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> These -Wuninitialized warnings about Operand's (implicit) copy constructor
> copying uninitialized member variables are only reproducible in Linux gcc
> PGO builds, but they are real bugs:

I doubt these are real bugs, the fields are initialized based on the kind. For instance, if kind_ == REG, the scale_/index_/disp_ fields shouldn't be used and can be safely left uninitialized.

Landing this patch to silence the gcc PGO warnings is probably fine though...

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8590087 [details] [diff] [review]
Wuninitialized_Operand.patch

Review of attachment 8590087 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/x86-shared/Assembler-x86-shared.h
@@ +78,5 @@
>      { }
>      explicit Operand(AbsoluteAddress address)
>        : kind_(MEM_ADDRESS32),
> +        scale_(TimesOne),
> +        index_(0),

We should initialize base_ too, I think base_(Registers::Invalid) should work.

@@ +84,5 @@
>      { }
>      explicit Operand(PatchedAbsoluteAddress address)
>        : kind_(MEM_ADDRESS32),
> +        scale_(TimesOne),
> +        index_(0),

And here.

Comment 3

[Chris Peterson [:cpeterson]]

Thanks. I added the base_ initializers, too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/469f64dc67e2

Comment 4

[Chris Peterson [:cpeterson]]

I had to back this out for -Wbitfield-constant-conversion warnings-as-errors on OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b83fb4bb908

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Wuninitialized_Operand_v2.patch

gcc didn't mind, but clang did not like assigning (unsigned) enum Registers::Invalid (16) to a signed int bitfield (of 5 bits). I changed Operand's base_ and index_ from int32_t to uint32_t because they are only used as (unsigned) Code enum values. I wanted to get a re-review to make sure there is no special reason for base_ or index_ to be signed.

js/src/jit/x86-shared/Assembler-x86-shared.h:81:9: error: implicit truncation from 'const Code' (aka 'const js::jit::X86Encoding::RegisterID') to bitfield changes value from 16 to -16 [-Werror,-Wbitfield-constant-conversion]

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ebb6ba8644

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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