-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: check that constant handles are not OK2_CONST_LONGs #1831
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
Follow-up from dotnet#1788. Also remove an arm-only handle block that should now be redundant.
|
@briansull PTAL |
|
The newly added assert is firing. Will dig in and see why. |
|
There is a strange code sequence here, that may be relevant: Assertionprop.cpp line 1341: |
|
Hmm, I should have checked the assertion unions more carefully -- the iconFlags overlay the top part of the long value (on a 32 bit host), so once an assertion is created, there's no easy way to check if somebody has inadvertently set the icon flags. I suppose in a debug build we could keep some extra state? |
Or perhaps just add padding to |
|
These flags should not overlap. I would suggest that we fix that. |
|
/AZP run runtime-coreclr outerloop |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
|
After adding padding, no hits from the new assert in inner or outer loop testing. The padding should not increase the size of the assertion records. The one remaining failure is in the flaky-seeming trace event test. |
briansull
left a comment
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.
Looks Good
| break; | ||
|
|
||
| case O2K_CONST_LONG: | ||
| { |
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.
Don't really need the braces { } 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.
I would add a break.
| // so no handle bits should be set here. | ||
| assert((assertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK) == 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.
Should have a break here
briansull
left a comment
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.
LGTM
|
Remaining failures are in the tracevent tests. |
Follow-up from #1788.
Also remove an arm-only handle block that should now be redundant.