Skip to content
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.

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

Pad bit-fields only to the next byte #1058

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

jmr
Copy link
Contributor

@jmr jmr commented Jun 21, 2020

Fixes #1054.

Previously, a bit-field followed by a non-bit field was expanded to two bytes. A bit-field ending the struct was (and continues to be) expanded to only a single byte.

struct s {
  unsigned int x : 4;
};

struct t {
  unsigned int x : 4;
  unsigned int y;
};

Before: sizeof (struct s) == 1, sizeof (struct t) == 4

After: sizeof (struct s) == 1, sizeof (struct t) == 3

Fixes cc65#1054.

Previously, bit-fields followed by another field were aligned
to two bytes.  Bit-fields ending the struct were (and continue
to be) aligned only to a single byte.

```
struct s {
  unsigned int x : 4;
};

struct t {
  unsigned int x : 4;
  unsigned int y;
};
```

Before: `sizeof(struct s) == 1`, sizeof(struct t) == 4`
After: `sizeof(struct s) == 1` sizeof(struct t) == 3`
src/cc65/declare.c Outdated Show resolved Hide resolved
@jmr
Copy link
Contributor Author

jmr commented Jun 22, 2020

I'll also try to add some tests.

@greg-king5
Copy link
Contributor

Bit-field initializers still are stored as two bytes. Do you want to fix that in this PR, or in a separate PR?

@jmr
Copy link
Contributor Author

jmr commented Jun 24, 2020

I'll fix it here. Could you merge #1059 so I can send a follow-up that gets the fields.c test working?

@jmr
Copy link
Contributor Author

jmr commented Jun 27, 2020

The tests I added in #1063 cause CHECK failures. I'll let you know when I have a new version with that fixed.

jmr added a commit to jmr/cc65 that referenced this pull request Jun 28, 2020
This prepares for cc65#1058, which will pad bit-fields only to
the next byte, instead of the next sizeof(int) (two bytes).

OutputBitFieldData now outputs chars instead of ints, and
calls to this function loop until there is less than one byte
to output.  A final partial byte is written out with zero padding
as a final partial int was previously.
@jmr
Copy link
Contributor Author

jmr commented Jun 28, 2020

Bit-field initializers still are stored as two bytes. Do you want to fix that in this PR, or in a separate PR?

This is in #1066 instead of this PR.

greg-king5 pushed a commit that referenced this pull request Jun 29, 2020
This prepares for #1058, which will pad bit-fields only to
the next byte, instead of the next sizeof(int) (two bytes).

OutputBitFieldData now outputs chars instead of ints, and
calls to this function loop until there is less than one byte
to output.  A final partial byte is written out with zero padding
as a final partial int was previously.
@oliverschmidt
Copy link
Contributor

Bit-field initializers still are stored as two bytes. Do you want to fix that in this PR, or in a separate PR?

This is in #1066 instead of this PR.

Sorry, I'm a bit lost here. I understand the statement above as #1066 fixing something regarding this PR (#1058).

But #1066 is already merged while this PR (#1058) is not.

@jmr @greg-king5: Can you please shed some light on the status of this PR?

@jmr
Copy link
Contributor Author

jmr commented Jul 3, 2020

Bit-field initializers still are stored as two bytes. Do you want to fix that in this PR, or in a separate PR?

This is in #1066 instead of this PR.

Sorry, I'm a bit lost here. I understand the statement above as #1066 fixing something regarding this PR (#1058).

#1066 was fixing something related to this PR, but it was also safe to do before this one, so I did it separately.

@jmr @greg-king5: Can you please shed some light on the status of this PR?

This is ready to be merged, but it would be better to merge #1063 first so there's more test coverage. This PR changes some sizes that are checked in #1063 , so I need to change something before the second one is merged.

@oliverschmidt
Copy link
Contributor

it would be better to merge #1063 first so there's more test coverage.

Does that mean that bitfield.c (#1063) is supposed to fail before this PR (#1058) is merged?

@jmr
Copy link
Contributor Author

jmr commented Jul 3, 2020

No. Either can be merged first, then the other will need to be updated.

@oliverschmidt
Copy link
Contributor

Now that #1071 is sorted out we can move forward again.

@oliverschmidt oliverschmidt merged commit 6dc2bf1 into cc65:master Jul 8, 2020
jmr added a commit to jmr/cc65 that referenced this pull request Jul 8, 2020
oliverschmidt pushed a commit that referenced this pull request Jul 8, 2020
src/cc65/declare.c Outdated Show resolved Hide resolved
jmr added a commit to jmr/cc65 that referenced this pull request Jul 8, 2020
MSVC complains about unary negation of unsigned, but it's
intended.  Suppress the warning.

cc65#1058 (comment)

"Tested" with godbolt.org.
@jmr jmr deleted the bit-field-pad-byte branch July 8, 2020 20:34
oliverschmidt pushed a commit that referenced this pull request Jul 9, 2020
MSVC complains about unary negation of unsigned, but it's
intended.  Suppress the warning.

#1058 (comment)

"Tested" with godbolt.org.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bit-fields inconsistently packed
4 participants