1387018 - MSVC does not pack Label correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

I noticed this a few months ago when I was debugging a crashdump on Windows but forgot to file it.

MSVC does not pack the bitfields in Label correctly, so sizeof(Label) > 4 on Windows. This actually matters because the Baseline compiler for instance has a Vector of Labels that can get pretty big.

This patch changes the bool field to int32_t and adds a sizeof(Label) static_assert. I confirmed the static_assert fails on Windows on Try without the patch.

Comment 1

[Jan de Mooij [:jandem]]

Ehsan, would it be feasible to detect this kind of thing in our Clang static analysis plugin?

In most cases the sizeof difference probably doesn't matter too much, but I bet there are more classes like this one where we're currently wasting space on Windows :/

Comment 2

[Jan de Mooij [:jandem]]

Hm I just realized a 1-bit *signed* int is weird (Stack Overflow suggests it stores 0 and -1). I'll see if MSVC is happy with uint32_t : 1, if not I'll probably just add uint32_t bits_; and implement our own packing. That might even be more efficient.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe6f10263b9
Change type of Label bitfield to ensure sizeof(Label) is 4 even with MSVC. r=bhackett

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Hm I just realized a 1-bit *signed* int is weird (Stack Overflow suggests it
> stores 0 and -1). I'll see if MSVC is happy with uint32_t : 1, if not I'll
> probably just add uint32_t bits_; and implement our own packing. That might
> even be more efficient.

uint32_t works fine according to Try.

Comment 5

[(no longer active)]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Ehsan, would it be feasible to detect this kind of thing in our Clang static
> analysis plugin?

Yes absolutely!  But I can't dissect what was going wrong here from the bug.  Do you mind elaborating, please?

Comment 6

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

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

Comment 7

[Jan de Mooij [:jandem]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #5)
> Yes absolutely!  But I can't dissect what was going wrong here from the bug.
> Do you mind elaborating, please?

Well we had a class like this:

struct Foo {
    int32_t i : 31;
    bool b : 1;
};

And MSVC does not pack Foo in 4 bytes (it probably uses 8 bytes or so). Using an integer type like uint32_t instead of bool fixes it.

Unfortunately I don't know what the spec says or which exact cases behave differently vs other compilers, but I just thought it would be nice if we could detect this so we don't have data structures that use more space than necessary on Windows only.

Comment 8

[(no longer active)]

Makes sense, filed bug 1389531.