add bitfields to D #13568

WalterBright · 2022-01-25T05:23:47Z

Since @ibuclaw and I already did the work to implement bitfields for ImportC, and they'll be of significant value to add to D, might as well enable them!

dlang-bot · 2022-01-25T05:23:49Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13568"

Geod24 · 2022-01-25T05:33:25Z

Very nice. This needs a few documentation changes, and a changelog entry to properly advertise it though.
Also, what are we going to do with https://dlang.org/library/std/bitmanip/bitfields.html ?

WalterBright · 2022-01-25T05:48:43Z

@Geod24 I agree, and I'll add it after this is working.

As for bitfields.html, it won't go into Phobos2.

changelog/bitfields.dd

src/dmd/parse.d

test/fail_compilation/test17284.d

changelog/bitfields.dd

WalterBright · 2022-01-26T04:36:22Z

Added code example, and link to ddoc PR with documentation.

WalterBright · 2022-01-26T04:39:27Z

Spec PR: dlang/dlang.org#3190

src/dmd/parse.d

ibuclaw · 2022-01-26T07:54:45Z

test/fail_compilation/test17284.d

-fail_compilation/test17284.d(1): Error: no identifier for declarator `TEST_OUTPUT`
-fail_compilation/test17284.d(1): Error: declaration expected, not `:`
-fail_compilation/test17284.d(12): Error: unmatched closing brace
+fail_compilation/test17284.d(3): Error: semicolon needed to end declaration of `__anonBitField1`, instead of `:`


See? You're doing it wrong if users can see internal names.

The Identifier.isAnonymous isn't checked in Identifier.toChars

Which is true of every use of anonymous names in the compiler.

ibuclaw · 2022-01-26T07:59:53Z

test/runnable/bit.d

+    uint x : 2, y : 3, :0;
+    int :0;


Missing:

union tests

class tests

non-pod struct tests (postblit, copy constructors)

static anonymous bit-field test

zero-sized named bit-field test

CTFE-interpreted width tests

non-integral type tests

template struct/class tests (T t : 3;)

reflection tests (can bit-fields be looked up with __traits same as regular fields?)

D enum bit-field tests

alias this bit-field test

fail compilation tests (taking address of bit-field)

property tests (offsetof, sizeof)

more property tests (min, max)

They all use the same code to resolved bit fields, and the same code that ImportC uses. Not even copied code, it's the same code in the same functions.

The whole point of this PR was to use the same code as was tested in ImportC.

That may be the case now but it still needs to be tested since it might not be the case in the future.

The whole point of mentioning classes and non-pod structs is to ensure that it works with druntime GC.

The whole point of CTFE constants is because C doesn't have a CTFE engine. (int ctbfield : ctfeFun();).

The whole point of mentioning __traits is because C doesn't have reflection.

We aren't testing this currently with ImportC.

That is based on a flawed assumption - the type of a bitfield is essentially a lie because the behaviour differs from the base type due to the limited size. This causes misbehaviour e.g. for overflows.

So these claims should be proven by tests instead of handwaving

I wonder if min/max properties would work too, and if value range propagation is aware of any of this. What if I assign an int to a bitfield? Is that a narrowing conversion that requires an explicit cast? If no, i guess so much for that rule. (I hate that rule anyway and would be glad to see it abolished). If yes, how do you explicitly cast to a type you can't represent?

I checked the internal type that gdc generates for bit-fields, and it correctly sets the backend properties of ie: int a:2; to .min = -2, .max = 1. Haven't checked the D front-end though.

Ok, I added these tests, except for the template one where I don't know what you mean.

Ok, I added these tests, except for the template one where I don't know what you mean.

struct S(T) { T t : 3; } S!int good; S!float bad;

kinke · 2022-01-26T15:46:54Z

Dammit, I hoped I would get away without having to touch LDC's glue layer, and hoped never to see native bitfields in D. :/

I wildly guess 99% of the use cases are packing bools to single bits; I don't think that justifies complicating the language.

ibuclaw · 2022-01-26T17:41:36Z

Dammit, I hoped I would get away without having to touch LDC's glue layer, and hoped never to see native bitfields in D. :/

I wildly guess 99% of the use cases are packing bools to single bits; I don't think that justifies complicating the language.

You'll need to implement bit-field support anyway for ImportC.

kinke · 2022-01-26T18:09:55Z

You'll need to implement bit-field support anyway for ImportC.

I don't need to. :P - They're currently not supported by LDC, just like the poor C initializer lists handling.

Edit: Anyway, my argument is the language complication. Just because C has it and now DMD+GDC for ImportC, doesn't mean it's a no-brainer to add this to D too, just because we easily can.

ibuclaw · 2022-01-26T18:24:56Z

I don't need to. :P - They're currently not supported by LDC, just like the poor C initializer lists handling.

You will for sure get away with it until someone attempts (from D code), import Cfile;

I'm currently blessed by that gdc foo.c will call the underlying C compiler/preprocessor, rather than the dmd ImportC implementation. But still not safe from the import case.

With any luck, bit-fields are the same as normal fields, you just set a width flag and llvm will take care of the rest.

Edit: Anyway, my argument is the language complication. Just because C has it and now DMD+GDC for ImportC, doesn't mean it's a no-brainer to add this to D too, just because we easily can.

We've been sitting on this for a month now, so it was was only a matter of time before someone tried to hook the two together.

kinke · 2022-01-26T18:34:48Z

I don't want to derail here too much, but as you know, I'm not convinced ImportC is the right approach.

Wrt. bitfields, we've been fine all this time without them (there's some library thing in Phobos IIRC), so I'm just raising my concerns here wrt. a clean language for the future. Bitfields seem 1960s tech in my eyes, where wasting bytes was presumably
a problem.

you just set a width flag and llvm will take care of the rest

Nope, LLVM has no such thing in their IR, at least AFAIK.

WalterBright · 2022-01-26T18:43:11Z

Rationale for D bitfields:

Impractical to be compatible with C structs without it. In developing ImportC, we found wide variation about how bitfield layouts are done among C compilers. This is mostly under-documented behavior, and was surprising (to me). Expecting D users to get this portably correct is unreasonable.
In dmd source, one often sees:

bool a;
bool b;
bool c;
etc.

that should be replaced with bit flags. But bit flags are a nuisance because they introduce another named scope, and adding/removing flags requires renumbering the rest. It's not self-documenting if the flags are inclusive or exclusive. Bit fields eliminate this problem.

As for wasting a byte, the creator of Zig pointed out how saving a byte in a heavily used struct had a significant effect on runtime speed and memory consumption.

The dmd backend will replace the bit field operations with shifts and masks:
https://github.com/dlang/dmd/blob/master/src/dmd/backend/cgelem.d#L3209
https://github.com/dlang/dmd/blob/master/src/dmd/backend/cgelem.d#L4142

Feel free to reuse in your glue layer.

WalterBright · 2022-01-26T18:47:14Z

there's some library thing in Phobos

The library thing in Phobos https://dlang.org/phobos/std_bitmanip.html is incompatible with the way gcc/clang lay out bit fields.

kinke · 2022-01-26T19:19:26Z

Okay so your #1 rationale seems to be the C interop. I don't think that's of high priority given the rareness of bit fields (right?). People/tools are currently using a dummy field of appropriate type and size when mapping a C bitfield to D.

Wrt. packing bools, e.g. in DMD source, there are surely cases where packing pays off, but I think they aren't many. I can fit 8 scalar bools inbetween 2 word-aligned members for 64-bit code and gain nothing from packing them into a single byte (but uglify in the source in the process). The existing std.bitmanip.bitFields mixin is generic; its usage could be made much simpler for bools only, something like:

struct S {
    mixin(packedBools!("a", "b", "c"));
}
…
S s;
if (s.a) s.b = true;

Feel free to reuse in your glue layer.

Generally speaking, an alternative would be moving this to the frontend. Then the glue layer can stay as it is, and no future D compilers would have to deal with such C ancestry.
Similar I guess for C initializer lists; if they can and were converted to a regular D initializer in the final AST, the glue layers won't need anything.

WalterBright · 2022-01-26T19:35:07Z

C interop

Anything we can do that makes that better will pay off. All it takes is for some user to try using a C library with a bit field, and he's faced with a non-trivial amount of work to deal with it. And much worse is the portability problem with bit field layouts, as it will cause the poor user hidden bugs.

I understand the nuisance this is for you, but the code to solve it is all there for you to use. It's just some shifting and masking. @ibuclaw and I have already solved the tedious problem of duplicating the target layout.

Similar I guess for C initializer lists; if they can and were converted to a regular D initializer in the final AST, the glue layers won't need anything.

That's on my list of things to do.

WalterBright · 2022-01-26T19:40:30Z

People/tools are currently using a dummy field of appropriate type and size when mapping a C bitfield to D.

Right, because they are forced to. But the problem with it is is what is the "appropriate type and size" ? This is a non-trivial and non-portable problem, as I found out when trying to implement that in the compiler. Alignment and padding are also an issue, as this is all implementation-defined. But to support it in LDC, this work is already done for you. The back end is supplied the byte offset, the bit position, the width, and the signedness. Everything needed to shift & mask.

kinke · 2022-01-27T00:42:43Z

Anything we can do that makes that better will pay off.

We're talking about a new language feature, that's not just 'anything' :) - leaving aside the unpleasant implications for current and future compiler backends.

I've incidentally had to map a C++ bitfield to D for interop a while ago, can't remember where, but in the end it was simply a matter of declaring a uint, because all the dirty bit manipulation was a C++ library detail, the fields private. D just had to know that it's some 32-bit value. I suspect this is the regular case for the off-chance one has to deal with bitfields in the first place.

I found no request for native bitfields: https://issues.dlang.org/buglist.cgi?quicksearch=bitfield&list_id=239169 (also searched for bit field)

@ibuclaw and I have already solved the tedious problem of duplicating the target layout

And of course that's not enough and raises further desires: https://issues.dlang.org/show_bug.cgi?id=22248, one of the 5 open issues about bit fields returned by above query

ibuclaw · 2022-04-07T17:50:45Z

What value does a preview switch add for this feature? As I've mentioned, I see none at all.

For the same reason that other features are behind preview flags (i.e: -preview=shortenedMethods), give it time to brew before a decision is made to commit to having them in the language proper. At the moment, I can't say whether a rejection is more likely if it was to go to vote.

added preview switch

addressed the issues

zachthemystic · 2022-04-09T15:54:52Z

https://en.wikipedia.org/wiki/Rogerian_argument#Rapoport's_rules

ibuclaw · 2022-04-10T07:10:48Z

test/runnable/dbitfieldsdm.d

+     * DM is 32 bit only
+     */
+    printf("                DM |   MS |  P32 |  P64\n");
+    printf("T0  = %2d %d ||  1 1 |  1 1 |  1 1 |  1 1\n", cast(int)T0.sizeof, cast(int)T0.alignof);


Wouldn't it be better to just use asserts here?

It's just easier to do it this way. The text representations are easier to edit, compare and understand.

It doesn't really help my end because the testsuite framework I use doesn't support comparing ad-hoc text, that can be worked around.

WalterBright · 2022-04-13T20:22:38Z

ping @ibuclaw

added the requested tests

WalterBright force-pushed the Dbitfields branch from 79a960c to 9c0e97f Compare January 25, 2022 05:28

Geod24 added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jan 25, 2022

WalterBright force-pushed the Dbitfields branch from 9c0e97f to 1dbbed6 Compare January 25, 2022 05:47

WalterBright force-pushed the Dbitfields branch from 1dbbed6 to 4001fe3 Compare January 25, 2022 06:03

RazvanN7 approved these changes Jan 25, 2022

View reviewed changes

changelog/bitfields.dd Show resolved Hide resolved

RazvanN7 removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Jan 25, 2022

dkorpel previously requested changes Jan 25, 2022

View reviewed changes

src/dmd/parse.d Outdated Show resolved Hide resolved

test/fail_compilation/test17284.d Outdated Show resolved Hide resolved

changelog/bitfields.dd Show resolved Hide resolved

WalterBright mentioned this pull request Jan 26, 2022

add bitfields documentation dlang/dlang.org#3190

Open

WalterBright force-pushed the Dbitfields branch from 4001fe3 to a768a8c Compare January 26, 2022 04:36

WalterBright added Severity:Enhancement and removed Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Jan 26, 2022

WalterBright force-pushed the Dbitfields branch from a768a8c to 54f6bb9 Compare January 26, 2022 08:19

ibuclaw previously requested changes Jan 26, 2022

View reviewed changes

WalterBright force-pushed the Dbitfields branch 3 times, most recently from 1721c41 to 0c6d79f Compare April 8, 2022 09:21

dkorpel mentioned this pull request Apr 8, 2022

Fix broken test 17284 #13967

Merged

WalterBright force-pushed the Dbitfields branch 3 times, most recently from 0a35857 to 19cf464 Compare April 9, 2022 07:22

WalterBright force-pushed the Dbitfields branch from 19cf464 to d2bc6ad Compare April 10, 2022 06:07

ibuclaw reviewed Apr 10, 2022

View reviewed changes

WalterBright force-pushed the Dbitfields branch 7 times, most recently from fba1618 to 1c046b2 Compare April 13, 2022 07:38

dlang-bot added the Review:Needs Rebase label Apr 18, 2022

add bitfields to D

a593261

WalterBright force-pushed the Dbitfields branch from 1c046b2 to a593261 Compare April 19, 2022 03:42

dlang-bot removed the Review:Needs Rebase label Apr 19, 2022

WalterBright merged commit 585b737 into dlang:master Apr 25, 2022

WalterBright deleted the Dbitfields branch April 25, 2022 21:59

ibuclaw pushed a commit to WalterBright/dmd that referenced this pull request May 1, 2022

add bitfields to D (dlang#13568)

aaed4af

WalterBright added a commit to WalterBright/dmd that referenced this pull request May 9, 2022

add bitfields to D (dlang#13568)

170903a

Uh oh!

add bitfields to D #13568

add bitfields to D #13568

Uh oh!

Conversation

WalterBright commented Jan 25, 2022

Uh oh!

dlang-bot commented Jan 25, 2022

Bugzilla references

Testing this PR locally

Uh oh!

Geod24 commented Jan 25, 2022

Uh oh!

WalterBright commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke commented Jan 26, 2022

Uh oh!

ibuclaw commented Jan 26, 2022

Uh oh!

kinke commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Jan 26, 2022

Uh oh!

kinke commented Jan 26, 2022

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

kinke commented Jan 26, 2022

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

WalterBright commented Jan 26, 2022

Uh oh!

kinke commented Jan 27, 2022

Uh oh!

ibuclaw commented Apr 7, 2022

ibuclaw Jan 26, 2022 •

edited

Loading

ibuclaw Jan 26, 2022 •

edited

Loading

MoonlightSentinel Jan 28, 2022 •

edited

Loading

kinke commented Jan 26, 2022 •

edited

Loading