Don't use one-bit bit fields for enums -- they misbehave and are inefficient in VC++

Don't use one-bit bit fields for enums -- they misbehave in VC++, unless you force the enum's type to be unsigned. enum bit fields may also waste space.

The bug is that direct comparisons of STORE_TO_INITIALIZED_ENTRY with store_mode_ would always fail, so this is a behavior-altering change.

One-bit bit fields in VC++ default to being signed so their only possible values are zero
and *negative* one. /analyze warned that such a bit field was being
compared to one which can never succeed.

Also, the bit field in particular was falling afoul of the VC++ bit field layout
rules and the two one-bit bit fields were actually taking up 64-bits.

Fixed by removing some bit field specifiers and specifying the enum's type as being unsigned char. This saves four bytes in both places the enum was used, and the lack of bit fields means that access to the variables will be faster, and the sign problem will be avoided.

In some cases it would be nice to use unsigned-char enums together with bit fields but gcc warns on that. Luckily that combination is not needed for this fix.

Here is the /analyze warning which I confirmed in a test app is a real bug:

v8\src\hydrogen-instructions.h(6845) : warning C6299: Explicitly comparing a bit field to a Boolean type will yield unexpected results.

BUG=427616
LOG=n

[brucedawson, 2014-10-31 18:38:38]

brucedawson@chromium.org changed reviewers:
+ hpayer@chromium.org

[cpu_(ooo_6.6-7.5), 2014-11-03 17:23:36]

cpu@chromium.org changed reviewers:
+ cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2014-11-03 17:23:37]

there are quite a few more of those in the file...  not compiled for windows?

[brucedawson, 2014-11-03 22:14:42]

On 2014/11/03 17:23:37, cpu wrote:
> there are quite a few more of those in the file...  not compiled for windows?

Were the other ones references to the same enum? If so then they should all be
fixed by this change.

One of the other references to store_mode_ is through a store_mode() accessor
which is probably sufficient to hide the problem from /analyze, although I
believe the problem still exists (the -1 value will be sign extended to 32-bits
and will compare false against 1). I'm not sure why /analyze doesn't complain
about the seemingly identical instance of this problem at line 7,034.

There are definitely other places in v8 where the bit field layout is not
working well. I've noted down a few of these but I found them based on a very
crude find-in-files. It may be worth running /d1reportAllClassLayout and parsing
the output to look for other inefficiently laid out bit fields so that we get
the intended memory savings. As for other places where sign extension is causing
incorrect results I'm not sure of any easy way to find them since I'm not
confident that /analyze identifies them all.

I've tried other fixes for this and they are problematic. Specifying the
underlying type for the enum as unsigned char or bool seems like it should work
but gcc misbehaves with unsigned char and VC++ misbehaves with bool. Probably
the best solution is to specify the underlying type as unsigned char (to save
space) and not use a bit field (to avoid compatibility complexity).

The unsigned char typed enums do work with clang, so if we could ditch gcc we'd
have a sane solution.

It's a mess.

[brucedawson, 2014-11-03 22:21:31]

I have an update that further improves things with VC++ by squishing the enum
into an unsigned char, but I am having trouble uploading it. "git cl upload"
gives me this error and I don't know what it means or where LOG=Y is supposed to
go.

** Presubmit ERRORS **
An issue reference (BUG=) requires a change log flag (LOG=). Use LOG=Y for
including this commit message in the change log. Use LOG=N or leave blank
otherwise.

Subsequent tries sometimes succeed but then they say:

No changes in files detected. Skipping cpplint check.

Since no changes are detected no changes are uploaded, and that's not much use.

Any advice? Apparently in v8 I cannot amend my changes.

[Jakob Kummerow, 2014-11-04 09:12:14]

On 2014/11/03 22:21:31, brucedawson wrote:
> I have an update that further improves things with VC++ by squishing the enum
> into an unsigned char, but I am having trouble uploading it. "git cl upload"
> gives me this error and I don't know what it means or where LOG=Y is supposed
to
> go.

It goes on the CL description, like "BUG=...". I've fixed it for you. Uploading
should work now.

[Hannes Payer (out of office), 2014-11-04 09:14:08]

On 2014/11/04 09:12:14, Jakob wrote:
> On 2014/11/03 22:21:31, brucedawson wrote:
> > I have an update that further improves things with VC++ by squishing the
enum
> > into an unsigned char, but I am having trouble uploading it. "git cl upload"
> > gives me this error and I don't know what it means or where LOG=Y is
supposed
> to
> > go.
> 
> It goes on the CL description, like "BUG=...". I've fixed it for you.
Uploading
> should work now.

[Hannes Payer (out of office), 2014-11-04 09:44:14]

On 2014/11/04 09:14:08, Hannes Payer (OOO) wrote:
> On 2014/11/04 09:12:14, Jakob wrote:
> > On 2014/11/03 22:21:31, brucedawson wrote:
> > > I have an update that further improves things with VC++ by squishing the
> enum
> > > into an unsigned char, but I am having trouble uploading it. "git cl
upload"
> > > gives me this error and I don't know what it means or where LOG=Y is
> supposed
> > to
> > > go.
> > 
> > It goes on the CL description, like "BUG=...". I've fixed it for you.
> Uploading
> > should work now.

Thanks for looking into this. As you pointed out in your cl description, I would
prefer the C++ 11 typed enum solution which is more general.

[brucedawson, 2014-11-04 18:48:02]

Okay, updates made. It's a shame that gcc and VC++ both make combining of enums
and bit fields so messy. In this case eschewing the bit field helps.

The behavioral change from this fix concerns me, but it should just make VC++
behave the same as the other platforms.

[cpu_(ooo_6.6-7.5), 2014-11-04 21:26:20]

cpu@chromium.org changed reviewers:
- cpu@chromium.org

[cpu_(ooo_6.6-7.5), 2014-11-04 21:26:20]

removing myself from review.

[Jakob Kummerow, 2014-11-04 21:37:33]

Triggered by this, and by the fact that we have many more places where we pack
member fields, I've created a patch that migrates all of them to V8's own
BitField. That's more verbose to write, but allows us full control over every
bit, and is compiler independent: https://codereview.chromium.org/700963002/