-
Notifications
You must be signed in to change notification settings - Fork 212
[aadwarf64]] Add DW_CFA_AARCH64_set_ra_state stack frame operation #346
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
base: main
Are you sure you want to change the base?
Conversation
The DW_CFA_AARCH64_set_ra_state operation updates the RA_SIGN_STATE pseudo register with the current signing state. If the signing state includes signing is DW_AARCH64_RA_SIGNED_SP_PC, then it also provides the offset to the signing instruction so that the PC value used in the signing can be calculated. The DW_CFA_AARCH64_negate_ra_state_with_pc operation has been marked as deprecated. This is because it has been found that it is not suitable for describing all cases where the PC was used to sign the return address (see ARM-software#327) The contents of the RA_SIGN_STATE pseudo register is also changed from being described in terms of a set of bits to being a series of defined values. Previously the state of the RA_SIGN_STATE pseudo register was changed implicitly by the DW_CFA_AARCH64_negate_ra_state and DW_CFA_AARCH64_set_ra_state operations. This meant that the actual encoding was actually internal to any implementation. Now with the introduction of the DW_CFA_AARCH64_set_ra_state operation the encoding has been made externally visible. So the opportunity has been taken now to change the encoding to a simpler form.
aadwarf64/aadwarf64.rst
Outdated
| | | | - Deprecated | | ||
| | | | DW_CFA_AARCH64_negate_ra_state. | |
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 believe this should be DW_CFA_AARCH64_negate_ra_state_with_pc.
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 spotting that. I'll fix it.
Incorrect change history deprecated state DW_CFA_AARCH64_negate_ra_state corrected to DW_CFA_AARCH64_negate_ra_state_with_pc
rearnsha
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.
A couple of grammatical nits/comments and I think we should add some background to the deprecation of ...negate_ra_state_with_pc
aadwarf64/aadwarf64.rst
Outdated
|
|
||
| 9. Normally, the program counter is restored from the return address, however | ||
| having both LR and PC columns is useful for describing asynchronously | ||
| having both LR and PC diversifiers are useful for describing asynchronously |
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 in this context, 'having both ... is useful' is the correct form. 'having both' here creates a singular subject for the sentence.
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
aadwarf64/aadwarf64.rst
Outdated
| The ``DW_CFA_AARCH64_set_ra_state`` instruction takes two operands; an unsigned | ||
| LEB128 value representing a return address state ra_state and a signed LEB128 | ||
| factored offset. The required action is to set the RA_SIGN_STATE pseudo-register |
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 use '...takes two operands: an unsigned ...; and a signed ...' to
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. The current style is what is used in the base DWARF specification. But the suggested change does make it clearer what are the 2 operands.
aadwarf64/aadwarf64.rst
Outdated
| as the signing/authenticating PAC instruction, otherwise it is has the value 0. | ||
| The code location information can be used for authenticating the return address. | ||
|
|
||
| The ``DW_CFA_AARCH64_negate_ra_state_with_pc`` operation toggles between the |
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.
Shouldn't this re-emphasize that this is deprecated (and perhaps explain why)?
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 added a statement to re-emphasize that this instruction is deprecate and a short explanation in non-normative text (the base DWARF specification does this by using italics for non-normative text so I have used the same convention here).
- Fixes some minor grammatical issues. - Add an explicit deprecation statement for the DW_CFA_AARCH64_negate_ra_state_with_pc instruction in the section describing the instruction. It has already been marked as deprecated in the AArch64 vendor CFA operations table.
The DW_CFA_AARCH64_set_ra_state operation updates the RA_SIGN_STATE pseudo register with the current signing state. If the signing state includes signing is DW_AARCH64_RA_SIGNED_SP_PC, then it also provides the offset to the signing instruction so that the PC value used in the signing can be calculated.
The DW_CFA_AARCH64_negate_ra_state_with_pc operation has been marked as deprecated. This is because it has been found that it is not suitable for describing all cases where the PC was used to sign the return address (see #327)
The contents of the RA_SIGN_STATE pseudo register is also changed from being described in terms of a set of bits to being a series of defined values.
Previously the state of the RA_SIGN_STATE pseudo register was changed implicitly by the DW_CFA_AARCH64_negate_ra_state and DW_CFA_AARCH64_set_ra_state operations. This meant that the actual encoding was actually internal to any implementation.
Now with the introduction of the DW_CFA_AARCH64_set_ra_state operation the encoding has been made externally visible. So the opportunity has been taken now to change the encoding to a simpler form.