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

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

clang-cl (built from recent svn HEAD) warns about initializing Operand::index_ with Registers::Invalid.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: Bug 1305816 - explicitly specify underlying type of js::jit::X86Encoding::RegisterID;

Recent clang-cl warns when building for x86-64 Windows about
initializing Assembler-x86-shared.h's Operand::index_ with
Registers::Invalid.  This warning stems from a couple of
implementation-defined behaviors in the C++ standard:

- The only constraints on an (non-fixed-width) enum's type are that
  the type is at least as large as `int`, and that the chosen type can
  accomodate all the values of the enum.  MSVC (and clang-cl) default
  to int wherever possible.

- Bitfields declared with `int` (resp. `char`, `short`, `long`) may be
  either signed or unsigned at the implementation's discretion.  It is
  therefore encouraged that you always declare bitfields with unsigned
  types.

Operand::index_ is a 5-bit field, with a declared type of
Register::Encoding; on x86, that boils down to the enum
js::jit::X86Encoding::RegisterID.  The compiler defaults the
underlying type of RegisterID to `int` (so the bitfield is signed on
some implementations), and Registers::Invalid is 16 on x86-64, large
enough to silently change sign when stored into the bitfield.

Declaring the bitfield as a fixed-width unsigned 8-bit enum is
sufficient to silence the warning.

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

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8795444 [details]
Bug 1305816 - explicitly specify underlying type of js::jit::X86Encoding::RegisterID;

https://reviewboard.mozilla.org/r/81482/#review80230

Comment 3

[Hannes Verschore [:h4writer]]

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?

Comment 4

[Nathan Froyd [:froydnj]]

(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?

Comment 5

[Jan de Mooij [:jandem]]

(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).

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: explicitly specify underlying type of js::jit::X86Encoding::RegisterID

Might as well re-review, in case you have comments.

Comment 7

[Jan de Mooij [:jandem]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cff35583fe
explicitly specify underlying type of js::jit::X86Encoding::RegisterID; r=jandem

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37cff35583fe