abi-aa
abi-aa copied to clipboard
Add initial draft of MemtagABI.
MemtagABI is an AArch64 ELF ABI extension that allows for tagging of stack, heap, and global variables. The ABI includes:
- Dynamic array entries for instructing the dynamic loader to control the MTE mode, heap, stack, and global tagging.
- A compressed table of metadata that's used to tag global variables at runtime.
- Extensions to existing relocations to allow tagging of global variables at runtime, while still allowing correct behaviour when run on non-MTE hardware or by non-MemtagABI aware loaders.
@smithp35 @MaskRay @pcc @eugenis @fmayer @enh
This is related to https://reviews.llvm.org/D128958.
Not hugely familiar with Github's review process - whether inline comments are published on-demand or only on this comment is unclear to me.
I've addressed the outstanding reviewer comments, leaving open anywhere that I'm still investigating or where it would be great to get an ack that I've done what you asked.
@hctim could you add this document to
- tools/common/check-rst-syntax.sh
- tools/rst2pdf/generate-pdfs.sh
- toplevel README.md
Adding the document to generate-pdfs.sh
will have the added benefit that it will be picked up by CI, which will include the PDF in the PDFs.zip zipfile of the CI run, which you can access if you click on the Details link for the CI run on this page, and scroll to the bottom part of the summary page: https://github.com/ARM-software/abi-aa/actions/runs/2959474522
@hctim could you add this document to
- tools/common/check-rst-syntax.sh
- tools/rst2pdf/generate-pdfs.sh
- toplevel README.md
Adding the document to
generate-pdfs.sh
will have the added benefit that it will be picked up by CI, which will include the PDF in the PDFs.zip zipfile of the CI run, which you can access if you click on the Details link for the CI run on this page, and scroll to the bottom part of the summary page: https://github.com/ARM-software/abi-aa/actions/runs/2959474522
Thanks, done all.
Folks who have left comments that are still open (and that I've responded to) - would you mind taking a quick look and making sure that your comments were sufficiently addressed?
I think the only outstanding thing here is to address the pauth+memtag pointers.
Who'd be the right person to approve/merge this document? Have they had an opportunity to review it?
@hctim could you add this document to
- tools/common/check-rst-syntax.sh
- tools/rst2pdf/generate-pdfs.sh
- toplevel README.md
Adding the document to
generate-pdfs.sh
will have the added benefit that it will be picked up by CI, which will include the PDF in the PDFs.zip zipfile of the CI run, which you can access if you click on the Details link for the CI run on this page, and scroll to the bottom part of the summary page: https://github.com/ARM-software/abi-aa/actions/runs/2959474522Thanks, done all.
Folks who have left comments that are still open (and that I've responded to) - would you mind taking a quick look and making sure that your comments were sufficiently addressed?
I think the only outstanding thing here is to address the pauth+memtag pointers.
Who'd be the right person to approve/merge this document? Have they had an opportunity to review it?
We're working through the process at the moment, I think the major things we need are:
- The copyright according to the recently updated CONTRIBUTION guide. I think this has been done.
- Assign a document owner to the table in https://github.com/ARM-software/abi-aa/blob/main/CONTRIBUTING.md . Would you be willing to be the owner, or one of the owners? I think this modification could be done in a follow up patch, possibly by someone from Arm, so no need to update the PR yet.
- Make sure we have the document status correct as we are expecting this document to evolve over time. This should be good already (Alpha).
- Ensure that the document has been reviewed, which I think we're getting to the end of that process now.
Will hopefully make some progress on these tomorrow. We also need to work out a process for how to get an Alpha merged in to the main ABI once it has stabilised.
We're working through the process at the moment, I think the major things we need are:
- The copyright according to the recently updated CONTRIBUTION guide. I think this has been done.
- Assign a document owner to the table in https://github.com/ARM-software/abi-aa/blob/main/CONTRIBUTING.md . Would you be willing to be the owner, or one of the owners? I think this modification could be done in a follow up patch, possibly by someone from Arm, so no need to update the PR yet.
Wasn't sure on first glance that the owner could be non-ARM, but I've added myself there.
- Make sure we have the document status correct as we are expecting this document to evolve over time. This should be good already (Alpha).
- Ensure that the document has been reviewed, which I think we're getting to the end of that process now.
Will hopefully make some progress on these tomorrow. We also need to work out a process for how to get an Alpha merged in to the main ABI once it has stabilised.
Great, thanks very much.
I plan to check over these changes over the next couple of days, although please do give us a ping if this drags on too long. My plan is to:
- Check to find if there are any comments that are unanswered and summarise them. This may take a bit of time as I'm not that familiar with the github UI.
- Check over the most recent changes.
- With luck give a +1 to merging, from there we'll give some time for any other reviewers to comment.
At some point after approval but prior to merging, we'll ask you to squash the commits into one or more logical commits with a good commit message. We've recently found that the ABI history has been cluttered up with "address review comments" commits.
I've given this my approval. What I suggest is to leave this open till next week for any further comments/objections. Once that has been covered the changes can be squashed (may be possible to do that now if it doesn't lose any of the conversations) and merged.
Thanks. AIUI, only the owners (or people with merge permissions) of the repo can merge the change. Even with the changes approved, I can't merge the branch, and it'd need to be done by you or someone from your team.
I was under the impression at that point, the default option was "squash and merge". I think a variant of the original commit message from the first version of the pull request might be a good one:
Add the Alpha version of MemtagABI.
MemtagABI is an AArch64 ELF ABI extension that allows for tagging of
stack, heap, and global variables. The ABI includes:
1. Dynamic array entries for instructing the dynamic loader to control
the MTE mode, heap, stack, and global tagging.
2. A compressed table of metadata that's used to tag global variables
at runtime.
3. Extensions to existing relocations to allow tagging of global
variables at runtime, while still allowing correct behaviour when
run on non-MTE hardware or by non-MemtagABI aware loaders.
I've given this my approval. What I suggest is to leave this open till next week for any further comments/objections. Once that has been covered the changes can be squashed (may be possible to do that now if it doesn't lose any of the conversations) and merged.
Thanks. AIUI, only the owners (or people with merge permissions) of the repo can merge the change. Even with the changes approved, I can't merge the branch, and it'd need to be done by you or someone from your team.
I was under the impression at that point, the default option was "squash and merge". I think a variant of the original commit message from the first version of the pull request might be a good one:
Add the Alpha version of MemtagABI. MemtagABI is an AArch64 ELF ABI extension that allows for tagging of stack, heap, and global variables. The ABI includes: 1. Dynamic array entries for instructing the dynamic loader to control the MTE mode, heap, stack, and global tagging. 2. A compressed table of metadata that's used to tag global variables at runtime. 3. Extensions to existing relocations to allow tagging of global variables at runtime, while still allowing correct behaviour when run on non-MTE hardware or by non-MemtagABI aware loaders.
Thanks for the update. It looks like the only option I have is Rebase and Merge. If you are able to squash the commits using your suggest commit message then I'll try and merge tomorrow since it has been a week since my "any last comments" message.
Thanks for the update. It looks like the only option I have is Rebase and Merge. If you are able to squash the commits using your suggest commit message then I'll try and merge tomorrow since it has been a week since my "any last comments" message.
Alright, I force-pushed to my own branch to reset the history. Github UI now only shows a single commit in the pull request, which seems right to me.
I think the only outstanding question I had was with respect to the MEMTAG_GLOBALS_STATIC section vs. st_other
: https://github.com/ARM-software/abi-aa/pull/166/files#diff-b74373b2ee6de4c5ae65712c0be8f666b7f192e3772f1a7b82126c00e320153c