Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Forbid creation of non-faulting null-check nodes. #77078
Forbid creation of non-faulting null-check nodes. #77078
Changes from 5 commits
d5e8d6a
0a17d91
37d8f34
6434f20
4c1fddd
98b6b90
d9bb584
6bb0d26
58a9b98
7193ca3
c984a91
e20a756
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 doesn't look like this does anything, since
node
is a local, and is unused after the subsequent return.Does the
Append(node);
need to be moved below, just before thereturn
?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.
Append(node)
sets class variableGenTree* m_result
, andnode
is used either (1) to be directly assigned tom_result
or (2) to makecomma
tree that is subsequently assigned tom_result
.So, I think the code is correct.
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 problem is that
Append(node)
here happens before you setnode = m_compiler->gtNewNothingNode();
. The NothingNode gets created then never used. And for the non-faulting case, the original BLK node is still on the side-effect list.I think you want code more like this:
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.
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
op1
(addr) of a static GT_FIELD can benullptr
, butfgAddrCouldBeNull
doesn't check for that. Could we AV here?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.
As you said, if the op1 address of GT_FIELD can be
nullptr
, we wanttrue
returned fromfgAddrCouldBeNull(op1->gtGetOp1())
. And, it falls through to the default case and returnstrue
. Isn't it enough?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, because
fgAddrColdBeNull()
hasswitch (addr->OperGet())
and in the case I'm worried about,addr
will be nullptr, so that would AV. Now, maybe there's some reason the case I'm worried about doesn't happen.Or, maybe you can rewrite it to protect against that, e.g.:
(presumably static fields don't need null 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.
Done.
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.
This is hiding the other
isContainable
and then going out of scope and the value is lost.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.
Yep and this is the reason for so many diffs.
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.
oh I forgot to remove it when moving it out of if. Thanks for spotting it.
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.
just a nit here - you could write it as
if you wanted to preserve the
const
ness or shorten it. I don't think it's a big deal.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.
Thanks for the code snippet. Will make that change.