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
Fix 20148 - void initializated bool can be both true and false #15362
Conversation
Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
|
@@ -960,6 +960,9 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor | |||
else if (dsym.type.hasSystemFields()) | |||
sc.setUnsafePreview(global.params.systemVariables, false, dsym.loc, | |||
"`void` initializers for `@system` variables not allowed in safe functions"); | |||
else if (dsym.type.toBasetype().ty == Tbool) | |||
sc.setUnsafePreview(global.params.systemVariables, false, dsym.loc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this just be a deprecation, independent of the system variables preview?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, would you prefer that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because then the diagnostic will get used (much) more widely and sooner. And otherwise it would potentially make it harder to stabilise the system variables preview switch, as it would likely catch more code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're under the impression that system variables don't emit deprecations yet, but they do. The only difference is that it becomes an error already with -preview=systemVariables
.
Why not just deprecate all void initialization in @safe functions? |
That is a lot more controversial |
I don't see how randomly choosing a different behavior for This is what Simen Kjaeraas is complaining about in the bug report: So instead of closing the obvious hole of @safe functions using void initialization we're just poking at symptoms here and there? |
It's not random, it's about whether the type has unsafe values. See https://dlang.org/spec/function.html#safe-values Considering bool as a type with unsafe values was originally included in my system variables DIP: https://github.com/dkorpel/DIPs/blob/c2078c52ae40f7ee713dc74776322aacabeb7877/DIPs/DIP1NNN-DK.md
It was later removed to make the DIP more focused, but the idea of considering |
The spec would need to be changed: - For basic data types, all possible bit patterns are safe.
+ For basic data types, all possible bit patterns are safe, except for `bool`, for which only `0` and `1` are safe. |
I'm not even sure the bug is valid and maybe the best is to disallow "dereferencing" the variable if it hasn't been assigned to yet. It's clearly not un- |
That depends on what you want from your boolean type. If you want to allow |
I don't see how a "quantum boolean" can violate memory safety, it's a scalar. It'll produce wrong results, and almost definitely be a bug, but not unsafe memory-wise. |
A pointer is also just an integer. The thing that matters is whether there are invariants on the bit pattern that must hold to ensure memory safety, which is not exclusive to pointer values. dip1035 has examples of this in the rationale section. For int f() @safe
{
int[2] arr;
bool i = void;
return arr[i]; // no bounds check inserted by compiler
} The elision of the bounds check relies on the invariant that a |
In general, any form of undefined behavior following from invalidated type invariants can lead to memory corruption. Memory corruption, undefined behavior, violations of type safety, wrong optimizer assumptions etc. are all basically the same thing. |
I don't see memory corruption per say. Rather there's two states that any variable could be in, set, and read. If a read occurs before a set, that is grounds for adding an error to avoid clear and obvious bugs. |
I still think that making it an error to read from void-initialised variables that haven't been written to makes more sense. It's always going to be a bug, after all (unless someone is implementing a very bad RNG). |
That's impossible to check in the general case. Example: void fill(bool[] b);
bool f()
{
bool[64] buf = void;
fill(buf[]);
return buf[50]; // has it been written to?
} What will you do here? |
I think it should error at If you intend for fill to initialise it, make it an In an attempt to address some further rebuttals... What about What about |
Why allow void-initializing a
Still needs to be addressed:
|
Not with |
Saying that, I don't think it would be untenable to have a simple per-function variable/parameter state tracker, such that But it will take some time and devotion for someone to implement it. |
I don't think write before read is relevant here. Modifying the example above slightly:
This generates this obviously memory corrupting code with -O on run.dlang.io:
|
I guess what Iain refers to is writing to the bool first, so my argument is beside the point. Sorry for the noise ;-) |
This is very similar, if not the same, to checking whether an immutable is being assigned or initialized inside a constructor. Pro: there should be some existing logic in the compiler to implement this. If this were to be implemented, I would argue it should be extended for all void initialized types. |
A very important use case for void initializations is the stack allocation of a buffer, of which only a portion of it is used. This is a major optimization. I looked at all the various cases here and in the bugzilla where the random bits can cause trouble. Eliminating void initializations for bool still leaves holes - the bool problem with unions which is there even with no void initializations. I looked at data flow analysis to prove initialization before use. Unfortunately, the front end is poorly equipped to do DFA. DFA in the optimizer pass cannot prove write-before-read, either (it can only prove read-before-write). The only thing I can think of that works 100% is to, whenever a bool is read, add |
You can still void initialize a bool array in
I know, I opened the PR with "Still need to prevent array cast and union overlap". Pointers and system variables also need this. It's not hard to add The real proposal here is in the spec PR (dlang/dlang.org#3649) not this prototype implementation PR. |
Spec PR was merged. Time to revisit this? |
Yes, I need to generalize the implementation so any aggregate with a bool (or system variable) becomes unsafe to void-initialize. |
a360f07
to
d26b849
Compare
@RazvanN7 This is ready now |
Still need to prevent array cast and union overlap though, I'll open a follow up issue if this gets merged