Closed Bug 1305816 Opened 5 years ago Closed 5 years ago

fix clang bitfield warnings in Assembler-x86-shared.h

Categories

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

x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 1 obsolete file)

clang-cl (built from recent svn HEAD) warns about initializing Operand::index_ with Registers::Invalid.
Comment on attachment 8795444 [details]
Bug 1305816 - explicitly specify underlying type of js::jit::X86Encoding::RegisterID;

https://reviewboard.mozilla.org/r/81482/#review80230
Attachment #8795444 - Flags: review?(jdemooij) → review+
Priority: -- → P1
Try build failed:
jit/x86-shared/Assembler-x86-shared.h:62:33: error: 'js::jit::Operand::index_' is too small to hold all values of 'js::jit::Register::Encoding {aka enum js::jit::X86Encoding::RegisterID}

Seems like js::jit::Operand::index_ takes only 5 bits, instead of the 8 RegisterID can be with this patch.
Can you take a look Nathan?
Flags: needinfo?(nfroyd)
(In reply to Hannes Verschore [:h4writer] from comment #3)
> Try build failed:
> jit/x86-shared/Assembler-x86-shared.h:62:33: error:
> 'js::jit::Operand::index_' is too small to hold all values of
> 'js::jit::Register::Encoding {aka enum js::jit::X86Encoding::RegisterID}
> 
> Seems like js::jit::Operand::index_ takes only 5 bits, instead of the 8
> RegisterID can be with this patch.

It's nonsensical that the compiler will warn about this for an 8-bit enum, but for a 32-bit enum (the default), it won't. :(  This is because GCC actually computes tight bounds for the enum if it is a non-fixed enum, but doesn't do that for fixed enums.  Sigh.

I don't have good ideas, other than making the underlying type conditional on MSVC (or maybe clang-cl).  Jan, WDYT?
Flags: needinfo?(nfroyd) → needinfo?(jdemooij)
(In reply to Nathan Froyd [:froydnj] from comment #4)
> I don't have good ideas, other than making the underlying type conditional
> on MSVC (or maybe clang-cl).  Jan, WDYT?

If this is the only case, I'd be fine with changing Operand::index_ from 5 bits to 8 to silence GCC. It seems like we have 15 bits of padding there, so it shouldn't affect sizeof(Operand).
Flags: needinfo?(jdemooij)
Might as well re-review, in case you have comments.
Attachment #8803022 - Flags: review?(jdemooij)
Attachment #8795444 - Attachment is obsolete: true
Comment on attachment 8803022 [details] [diff] [review]
explicitly specify underlying type of js::jit::X86Encoding::RegisterID

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

LGTM. If only we had uint5_t :)
Attachment #8803022 - Flags: review?(jdemooij) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cff35583fe
explicitly specify underlying type of js::jit::X86Encoding::RegisterID; r=jandem
https://hg.mozilla.org/mozilla-central/rev/37cff35583fe
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.