Skip to content

[NFC] Address more bit-field storage sizes #140493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[NFC] Address more bit-field storage sizes #140493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lld/wasm/InputChunks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,15 @@ class InputChunk {
// the beginning of the output section this chunk was assigned to.
int32_t outSecOff = 0;

uint8_t sectionKind : 3;
uint32_t sectionKind : 3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why uint32_t is better than uint8_t here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Perhaps some background in the PR description would be useful).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I can :(

The MS ABI only packs bit-fields if they have the same storage size, e.g.

struct S {
   bool b: 1;
   int    i: 1
};

is 8 bytes.

It's a large part of why we use preferred_type and unsigned bit-fields instead of just enum bit-fields.

This PR is the continuing series of fixing the existing cases where the storage types are wrong (per MS :D), so we can then enable the warning for this behavior, so that we can then just start using enums without risking silently terrible padding with MSVC

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By MS ABI I assume you mean mircrosoft?

I guess this is not actually a regression on non-MS compilers since the bit width means that type does not actually impact the size in the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let me check whether I'm meant to be using Microsoft or MS (I may have gotten conflated with when I should use MSVC).

This does not change behavior with anything other than the MS abi. When building with the MS abi this changes the field padding to match clang/gcc/every other ABI.

The long term reason for the change is so we can enable -Wms-bitfield-padding without immediately overwhelming stage2 or trunk builders with the new padding warning.

Once that's enabled we can start moving to correctly typed enum bit-fields with some confidence we aren't going to regress layout under the ms abi.


// Signals that the section is part of the output. The garbage collector,
// and COMDAT handling can set a sections' Live bit.
// If GC is disabled, all sections start out as live by default.
unsigned live : 1;
uint32_t live : 1;

// Signals the chunk was discarded by COMDAT handling.
unsigned discarded : 1;
uint32_t discarded : 1;

protected:
InputChunk(ObjFile *f, Kind k, StringRef name, uint32_t alignment = 0,
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Language/ObjC/NSArray.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ namespace Foundation1010 {
uint32_t _used;
uint32_t _offset;
uint32_t _size : 28;
uint64_t _priv1 : 4;
uint32_t _priv1 : 4;
uint32_t _priv2;
uint32_t _data;
};
Expand Down
6 changes: 3 additions & 3 deletions lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class NSDictionaryISyntheticFrontEnd : public SyntheticChildrenFrontEnd {

struct DataDescriptor_64 {
uint64_t _used : 58;
uint32_t _szidx : 6;
uint64_t _szidx : 6;
};

struct DictionaryItemDescriptor {
Expand Down Expand Up @@ -273,7 +273,7 @@ namespace Foundation1100 {

struct DataDescriptor_64 {
uint64_t _used : 58;
uint32_t _kvo : 1;
uint64_t _kvo : 1;
uint64_t _size;
uint64_t _mutations;
uint64_t _objs_addr;
Expand Down Expand Up @@ -308,7 +308,7 @@ namespace Foundation1428 {

struct DataDescriptor_64 {
uint64_t _used : 58;
uint32_t _kvo : 1;
uint64_t _kvo : 1;
uint64_t _size;
uint64_t _buffer;
uint64_t GetSize() { return _size; }
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Language/ObjC/NSSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class NSSetISyntheticFrontEnd : public SyntheticChildrenFrontEnd {

struct DataDescriptor_64 {
uint64_t _used : 58;
uint32_t _szidx : 6;
uint64_t _szidx : 6;
};

struct SetItemDescriptor {
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class DWARFDebugInfoEntry {
// If it is zero, then the DIE doesn't have children,
// or the DWARF claimed it had children but the DIE
// only contained a single NULL terminating child.
uint32_t m_sibling_idx : 31, m_has_children : 1;
uint64_t m_sibling_idx : 31, m_has_children : 1;
uint16_t m_abbr_idx = 0;
/// A copy of the DW_TAG value so we don't have to go through the compile
/// unit abbrev table
Expand Down