-
-
Notifications
You must be signed in to change notification settings - Fork 668
Implement null dereference check #22040
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
Conversation
|
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 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. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#22040" |
1ca48e7 to
c0d2c45
Compare
|
Fun fact, lazy parameters can have a null object. Oh well we just can't check for null on the object of a delegate prior to a call. |
86f1e3b to
05957b2
Compare
|
So a |
It is an Error. Something has to die in response, like all Error's. |
05957b2 to
7f74a82
Compare
|
Is the plan to have |
|
There's no technical reason why you can't recover from a NullPointerError in general, though you may choose not to as it can indicate you don't understand program state. |
Possibly. I intend to wire up the fast DFA engine to elide the checks. But can't do it until what does exist is proven to work. |
|
The performance impact is very small. The cpu's branch predictor handles it well and ldc's code generator is decent at removing dead checks and arranging the code blocks in a cache-friendly manner. (dmd's isn't so good but ldc is known to optimize better than dmd in many ways) |
|
So a single unittest in libdparse at https://buildkite.com/dlang/dmd/builds/43787/steps/canvas?sid=019a2b1c-9d02-4993-b9df-dfb58a18783d fails buildkite/dmd. The unittest
{
version (53056)[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[
union {}
Klse
gosh ÿ� 'A+ SPL : tape];
}
}
}. Could the CI failure be caused by the compiler existing with error code other than what the tests expects? I also wonder how these seemingly random test cases where deduced... |
|
I'm aware, but the problem will be in the compiled libdparse, and without compiling it locally I won't be figuring out which emitted hook did it. As far as I know this could be related to XMM intrinsics, and since those are more of a known problem they're up first. |
7f74a82 to
0686734
Compare
47116b7 to
8db71ae
Compare
|
CI has done the correct, thing. Anything else you need @WalterBright before I turn it off and take it out of draft? |
| This can be enabled by using ``-check=nullderef=on`` | ||
| What happens may be customized by the ``-checkaction`` switch and by setting a new handler in ``core.exception``. | ||
|
|
||
| Due to issues in dmd's backend, not all pointer dereferences are guaranteed to get a check. |
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.
What does that mean?
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.
Disabling of intrinsics and their arguments.
I don't particularly want to explain this in a changelog as it won't impact everyone, but it does need acknowledgment that dmd specifically can't check all pointer dereferences.
I can disable the intrinsic check in a follow-up PR for you to look into.
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.
What do the intrinsics have to do with null pointer checking?
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.
Ideally nothing.
If I remove the counter check the backend errors when intrinsics are used.
| A new check is implemented for when a null dereference occurs. | ||
|
|
||
| This can be enabled by using ``-check=nullderef=on`` | ||
| What happens may be customized by the ``-checkaction`` switch and by setting a new handler in ``core.exception``. |
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.
No mention of what the default behavior is.
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.
Could add a brief discussion of why one would want this feature.
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.
Done.
7e31fd7 to
773864e
Compare
|
I've turned this off now, will need confirmation that @WalterBright doesn't want any more changes. |
|
@rikkimax can you please resolve the conflicts? Thanks! |
Can do later on, is your feedback handled? |
773864e to
cec2f8d
Compare
What is the relationship between this PR and the opening comment? |
Remember the error handling meeting we had for exceptions? This came up and was approved work.
|
| =invariant[=[on|off]] Class/struct invariants | ||
| =out[=[on|off]] Out contracts | ||
| =switch[=[on|off]] Final switch failure checking | ||
| =nullderef[=[on|off]] Null dereference error |
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.
=safeonly ?
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.
None of the others support it. I'm matching them.
compiler/src/dmd/dfa/fast/report.d
Outdated
| * when the scope exits. | ||
| */ | ||
| void onEndOfScope(FuncDeclaration fd, ref Loc loc) | ||
| void onEndOfScope(FuncDeclaration fd, ref const(Loc) 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.
ref const Loc loc will suffice
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.
Applying to elsewhere in module where I've done it.
| case CHECKENABLE.on: | ||
| return true; | ||
| case CHECKENABLE.safeonly: | ||
| { |
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.
unnecessary { }
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 prefer adding the extra scope to make it clearer. Removing.
| ec = el_same(ethis); | ||
| ethis = el_una(target.isX86_64 || target.isAArch64 ? OP128_64 : OP64_32, TYnptr, ethis); // get this | ||
| ec = array_toPtr(t, ec); // get funcptr | ||
|
|
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.
why removed?
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.
Not removed, slightly moved around.
This was the cleanest I could come up with, which resulted in the least memory allocations.
| params.useInvariants = CHECKENABLE.off; | ||
| params.useOut = CHECKENABLE.off; | ||
| params.useSwitchError = CHECKENABLE.off; | ||
| params.useNullCheck = CHECKENABLE.off; |
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.
what happened to =safeonly ?
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 have no idea, I don't remove such things.
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.
the nullcheck has three settings - on|off|safeonly
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.
There is nothing to enable safeonly for -check.
Line 746 in 72eac87
| static bool check(const(char)[] checkarg, string name, ref CHECKENABLE ce) |
I haven't got an opinion about it being added. But right now, it doesn't exist for any of the checks.
cec2f8d to
6c8cefc
Compare
| return false; | ||
| case CHECKENABLE.on: | ||
| return true; | ||
| case CHECKENABLE.safeonly: |
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.
Here is where the null dereference check is enabled for safeonly. But there is no way to set it to safeonly.
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.
Indeed. Seems to be an unused enum member for all the checks.
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.
Do you think it would be reasonable to add safeonly for the null check?
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 safeonly should be readded for all of the check's in one go.
A separate PR.
It's probably just
Line 763 in 72eac87
| } |
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.
fair 'nuff
6c8cefc to
7d3b300
Compare
Approved work: https://forum.dlang.org/post/mwffryejcmqyolmweaps@forum.dlang.org
Currently turned on to see what the CI does.
Will request Walter to review once I'm happy with CI.
Heavily inspired by OpenD with thanks to @adamdruppe.
EDIT: Special thanks to @limepoutine for helping to get both function pointers and delegate function pointers to be null checked!