-
-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| An optional check for null dereference is added | ||
|
|
||
| A new check has been implemented that injects code to check a pointer for null before it is dereferenced. | ||
|
|
||
| It will be typically be used if you need a backtrace generated (when one is not automatically done), or if you want to catch and handle the Error as part of a scheduler. | ||
|
|
||
| This can be enabled by using ``-check=nullderef=on`` by default it is off. | ||
| 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does that mean?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do the intrinsics have to do with null pointer checking?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ enum RTLSYM | |
| DARRAYP, | ||
| DARRAY_SLICEP, | ||
| DARRAY_INDEXP, | ||
| DNULLP, | ||
| DINVARIANT, | ||
| MEMCPY, | ||
| MEMSET8, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1187,6 +1187,7 @@ struct CLIUsage | |
| =invariant[=[on|off]] Class/struct invariants | ||
| =out[=[on|off]] Out contracts | ||
| =switch[=[on|off]] Final switch failure checking | ||
| =nullderef[=[on|off]] Null dereference error | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. =safeonly ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of the others support it. I'm matching them. |
||
| =on Enable all assertion checking | ||
| (default for non-release builds) | ||
| =off Disable all assertion checking | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4282,16 +4282,43 @@ bool arrayBoundsCheck(FuncDeclaration fd) | |||
| case CHECKENABLE.on: | ||||
| return true; | ||||
| case CHECKENABLE.safeonly: | ||||
| if (fd) | ||||
| { | ||||
| Type t = fd.type; | ||||
| if (t.isTypeFunction() && t.isTypeFunction().trust == TRUST.safe) | ||||
| return true; | ||||
| } | ||||
| return false; | ||||
| case CHECKENABLE._default: | ||||
| assert(0); | ||||
| } | ||||
| } | ||||
|
|
||||
| /********************** | ||||
| * Check to see if null dereference checking code has to be generated | ||||
| * | ||||
| * Params: | ||||
| * fd = function for which code is to be generated | ||||
| * Returns: | ||||
| * true if do null dereference checking for the given function | ||||
| */ | ||||
| bool nullDerefCheck(FuncDeclaration fd) | ||||
| { | ||||
| final switch (global.params.useNullCheck) | ||||
| { | ||||
| case CHECKENABLE.off: | ||||
| return false; | ||||
| case CHECKENABLE.on: | ||||
| return true; | ||||
| case CHECKENABLE.safeonly: | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. Seems to be an unused enum member for all the checks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair 'nuff |
||||
| if (fd) | ||||
| { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary { }
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer adding the extra scope to make it clearer. Removing. |
||||
| Type t = fd.type; | ||||
| if (t.ty == Tfunction && (cast(TypeFunction)t).trust == TRUST.safe) | ||||
| if (t.isTypeFunction() && t.isTypeFunction().trust == TRUST.safe) | ||||
| return true; | ||||
| } | ||||
| return false; | ||||
| } | ||||
| case CHECKENABLE._default: | ||||
| assert(0); | ||||
| case CHECKENABLE._default: | ||||
| assert(0); | ||||
| } | ||||
| } | ||||
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.