- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
Teach x86 unwinder about POP ECX being optional optimization #115660
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
        
          
                src/coreclr/vm/gc_unwind_x86.inl
              
                Outdated
          
        
      | // do nothing before popping the callee-saved registers | ||
| } | ||
| else if (info->rawStkSize == sizeof(void*)) | ||
| else if (info->rawStkSize == sizeof(void*) && epilogBase[0] == X86_INSTR_POP_ECX) | 
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.
offset is always 0 at this point, so epilogBase[0] and epilogBase[offset] is the same thing
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 it would be better to use the offset even though it is zero. All the remaining code uses offset, even in the code inside of this if, so using 0 here might be confusing for the reader.
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 changed both of the places with the pattern. Optimizing compiler should propagate the constant anyway.
| Tagging subscribers to this area: @mangod9 | 
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 looks good modulo the comment.
        
          
                src/coreclr/vm/gc_unwind_x86.inl
              
                Outdated
          
        
      | // do nothing before popping the callee-saved registers | ||
| } | ||
| else if (info->rawStkSize == sizeof(void*)) | ||
| else if (info->rawStkSize == sizeof(void*) && epilogBase[0] == X86_INSTR_POP_ECX) | 
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 it would be better to use the offset even though it is zero. All the remaining code uses offset, even in the code inside of this if, so using 0 here might be confusing for the reader.
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, thank you!
Fixes #115120
The same pattern already exists in the unwinder elsewhere:
runtime/src/coreclr/vm/gc_unwind_x86.inl
Lines 2446 to 2449 in 2f8f6e7