Implement bit field sanity checks

[rikkimax]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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#20848"

[rikkimax]

Good news, all the analysis is doing its job. That makes me happy.

Bad news, I'm not sure what to do about the tests, since this is generating an error not a warning.

@WalterBright I'm ok with a warning over an error, what do you want to do about this?

EDIT: done.

[rikkimax]

Okay, think I got the test suite sorted out.

I've checked with @schveiguy, he is happy with what problems it's finding.

This should solve our concerns.

[WalterBright]

Thank you.

1. turn off checks if isCsymbol()
2. alignment of previous field not considered:

struct S { byte b; int a:3; }
pragma(msg, S.sizeof);

prints 4 on one machine and 8 on another
3. does not consider contributesToAggregateAlignment

The approach I was contemplating is that if a different layout was produced when compiling for Microsoft vs non-Microsoft, then issue the error. Otherwise, I'd expect a number of false positives. The implementation code here does not appear to do that. This will also make it sensitive to align directives, which it should be.

[rikkimax]

Shouldn't all C scopes have the C linkage set?

Yes it won't consider previous alignment, that is ok. It doesn't matter where the start byte offset is.
The reason is due to conditional compilation, its going to be different on every target (potentially). If you try to solve for this without a clear example, there is a good chance of false positives I think.

If you do care about start byte offset, you can pack it with align(1).

[rikkimax]

I don't see a reason to check against contributesToAggregateAlignment. Same problem, its alignment related of initial start offset.

[rikkimax]

Okay looks like the C linkage is set for ImportC:

https://github.com/dlang/dmd/blob/master/compiler/test/runnable/bitfields.c#L120

Never caused an error, and I'm pretty certain it should have.

[WalterBright]

It doesn't matter where the start byte offset is.

It alters the behavior of anonymous fields.

If you try to solve for this without a clear example, there is a good chance of false positives I think.

The solution I was anticipating was if the layout is different between platforms, then and only then flag it. That means emulating the layout algorithm.

If you do care about start byte offset, you can pack it with align(1).

This is the crux of the issue. Why does one not care about field alignment portability, yet care about bitfield layout portability? Nobody in the C world seems to care, why should we? (Numerous google searches turned up nothing significant, other than Linus saying if you do care, use explicit shift and mask.)

[rikkimax]

It doesn't matter where the start byte offset is.

It alters the behavior of anonymous fields.

Yes, I saw them in the test suite.

struct F {
    ubyte :1;
    ubyte v:1;
}

That's an error in extern D.

You can't introspect it.

We did discuss it, and honestly, it's kinda insanity to allow this in extern D.

If you try to solve for this without a clear example, there is a good chance of false positives I think.

The solution I was anticipating was if the layout is different between platforms, then and only then flag it. That means emulating the layout algorithm.

Indeed a very admirable goal that I thought was possible from what you've been saying.
However, while that may work per compiler, it doesn't account for all compilers. There will be differing behaviours, unfortunately.

So to a certain extent, we just gotta go: "use your CI to detect all combinations you care about".

No matter what we do, its gonna fail at some point.

If you do care about start byte offset, you can pack it with align(1).

This is the crux of the issue. Why does one not care about field alignment portability, yet care about bitfield layout portability? Nobody in the C world seems to care, why should we? (Numerous google searches turned up nothing significant, other than Linus saying if you do care, use explicit shift and mask.)

Keep in mind that the C folk don't have this guarantee that this PR adds.

Therefore they have no choice but to go: "don't use language bitfields".

We have it, therefore we can rely on it and use it in a lot more places.

[WalterBright]

However, while that may work per compiler, it doesn't account for all compilers.

Nothing accounts for all compilers. We explicitly only account for the "associated C compiler".

Keep in mind that the C folk don't have this guarantee that this PR adds.

Right, because in the last 35 years C has made no movement towards such a guarantee. They evidently do not care. There are millions of C users and google search turns up next to nothing on the issue. Why is it a monumental problem for D?

Therefore they have no choice but to go: "don't use language bitfields".

They do have a choice. It's trivial to write portable bitfields in C. In D it is, too, or one can use std.bitmanip. This whole thing is an exercise in waste of time.

The -preview=bitfields switch has been there for years, and not a single complaint about this that I'm aware of.

Frankly, I suspect I am the only D user who even wants to use bitfields, or people would be asking "why don't we merge it?" This has gone on for several years now. So why not merge the bitfields, I will use them, and everyone else can go on doing what they do, and everyone will be happy!

P.S. I'm sorry to unload on you, this is not remotely your fault. I apprecate the efforts you have made here.

P.P.S. Bitfields would make the DMD front end source code significanly easier to read and maintain. And it will mesh perfectly with the gdc and ldc backends. It's just sad that we can't use it.

[schveiguy]

The approach I was contemplating is that if a different layout was produced when compiling for Microsoft vs non-Microsoft, then issue the error. Otherwise, I'd expect a number of false positives.

If these are the only 2 C layouts we know of, this seems like a reasonable mechanism.

[schveiguy]

Frankly, I suspect I am the only D user who even wants to use bitfields, or people would be asking "why don't we merge it?" This has gone on for several years now. So why not merge the bitfields, I will use them, and everyone else can go on doing what they do, and everyone will be happy!

I would like to use bitfields for the new GC. But they have to be precisely laid out. Having a guarantee they stay as expected would be very beneficial. (Note, the new GC currently only compiles with SDC, so some work needs to happen to get access to bitfields).

[WalterBright]

I would like to use bitfields for the new GC. But they have to be precisely laid out.

Send me the layout you want, and I will send back the precise bitfield declaration for it.

[rikkimax]

I know you're feeling frustrated about this, and yes I do want them too.

But it does add a level of unknown movability that the compiler can do without a way for me to just slap on align(1) and get what I want.

And yes this is 100% on the C designer's todo list to improve C's bitfields. I know it's on JeanHeyd Meneide I asked a while back. They have a rather long to do list though!

[rikkimax]

Here is what I suggest:

We merge this and turn on bitfields.

In a couple of years time, we'll know if it is doing its job or not.
It won't break any code to remove it later.

If not, we can swap it for something more layout engine-aware, which is something you are having trouble with anyway.

I'll even do the PR to turn it on!

[WalterBright]

@rikkimax I'm curious what the C proposal is. (There are a lot of C proposals, very few make it into the Standard. A proposal doesn't mean the wider community is interested in it.)

[rikkimax]

No proposals currently that I am aware of and I did check their documents listing.

It's certainly not at the top of anyone's todo list.

But regardless, I don't want to be debugging someone's code and it turns out that a simple bit width count wasn't enough to understand a packed struct. The prospect of that does not bring joy!

[WalterBright]

I don't want to be debugging someone's code and it turns out that a simple bit width count wasn't enough to understand a packed struct

I understand your point. It's great to have a mechanical check that finds errors so one doesn't have to debug. I've used linters and very pedantic error checkers. I soon abandoned them all because the errors given were false positives and the recommended fixes were unattractive. There's a sweet spot in there somewhere, where the real errors are found without being an annoying nag. It's hard to know where that spot is; the best we can do is rely on experience as a guide.

(I've also abandoned entire languages (i.e. Pascal) because of the overbearing error checking.)

[schveiguy]

Send me the layout you want, and I will send back the precise bitfield declaration for it.

I know how to do it correctly. I'm saying, it would be nice if the compiler is also helping me by making sure I don't make a mistake (or anyone else working on the code).

"Send it to Walter if you want it to work" doesn't scale.

Note, we wouldn't be having this discussion at all if some C compilers hadn't decided to do clever tricks to pack bits. Requiring the specification of every bit alleviates any of these problems.