Fix -Wuninitialized warnings about Operand member variables

RESOLVED FIXED in Firefox 40

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla40
All
Linux
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8590087 [details] [diff] [review]
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]
Attachment #8590087 - Flags: review?(jdemooij)
(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 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.
Attachment #8590087 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 3

3 years ago
Thanks. I added the base_ initializers, too.

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

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
Created attachment 8591390 [details] [diff] [review]
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]
Attachment #8590087 - Attachment is obsolete: true
Attachment #8591390 - Flags: review?(jdemooij)

Updated

3 years ago
Attachment #8591390 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/b0ebb6ba8644
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.