MSVC does not pack Label correctly

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
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.
Attachment #8893351 - Flags: review?(bhackett1024)
(Assignee)

Comment 1

2 years ago
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 :/
Flags: needinfo?(ehsan)
Attachment #8893351 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 2

2 years ago
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

2 years ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Comment 4

2 years ago
(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

2 years ago
(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?
Flags: needinfo?(ehsan)

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbe6f10263b9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

2 years ago
Flags: needinfo?(jdemooij)
Depends on: 1387535
(Assignee)

Comment 7

2 years ago
(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.
Flags: needinfo?(jdemooij)

Comment 8

2 years ago
Makes sense, filed bug 1389531.
You need to log in before you can comment on or make changes to this bug.