User Details
- User Since
- Jun 4 2020, 7:24 AM (183 w, 6 h)
Oct 11 2023
Thank you for the review. I read through the comments - they are already addressed except maybe for a few FPAC-related notes, so I added a FIXME to checkAuthenticatedLR function and left a note in the discussion of D156784.
Added FIXME mentioning FEAT_FPAC to AArch64PointerAuth::checkAuthenticatedLR.
As discussed in D156716, it is not clear if I have to add FeatureFPAC to every relevant CPU. Maybe it is worth conservatively assuming that this feature should only be enabled manually by the user as a precaution against "I have CPU core X but it is not listed, so let's use cpu=Y because X supports all the instructions supported by Y (but not FEAT_FPAC)" - that would not cause any obvious run-time crashes under normal operation, but would make the code less secure.
Oct 9 2023
Moved AddressCheckPseudoSourceValue to AArch64SubtargetInfo.
Oct 6 2023
Address the review comments.
Oct 2 2023
Addressed the comments so far.
Updated the patch, thank you.
Sep 29 2023
Updated the patch:
Sep 26 2023
Here is a summary of the contents of this patch:
- implemented a standalone llvm::AArch64PAuth::checkAuthenticatedRegister utility function to emit one of a number of checks in case a pointer is AUT'ed and not immediately used for memory access
- placed this function into a sub-namespace instead of making it a static class member, so I don't have to put otherwise irrelevant AArch64PointerAuth class definition to header file
- note that the checks that are inserted by checkAuthenticatedRegister function are not specific to tail calls (but some of the checks may have restrictions - such as requiring AuthenticatedReg == LR because XPACLRI is encoded as HINT while generic XPAC* instructions require FEAT_PAUTH)
- hooked it to AArch64PointerAuth class via checkAuthenticatedLR method dedicated to hardening tail calls
- in machine outliner, update the costs computed by AArch64InstrInfo::getOutliningCandidateInfo method on a best-effort basis:
- in MachineOutlinerTailCall outlining mode, we need to insert checks in each caller of OUTLINED_FUNCTION
- in MachineOutlinerThunk mode, at most a single extra check is inserted in OUTLINED_FUNCTION itself
- other modes do not introduce new tail calls, so let's just try to account for the checks that would possibly be inserted later into the original candidates by the AArch64PointerAuth pass (including in the two aboves modes)
- factored out isTailCallReturnInst and needsShadowCallStackPrologueEpilogue utility functions
Rebased and updated the patch a bit more:
- in getOutliningCandidateInfo, adjusted SequenceSize variable before its first use
- updated code comments
- skipped checking LR if Scadow Call Stack is enabled: the LR value just before TCRETURN* instruction is anyway not the one produced by AUT* (and we cannot check right after AUT* because we cannot be sure which register is usable as a temporary)
Sep 22 2023
Sep 19 2023
Updated after the D159357 was changed.
Addressed the review comments, rebased.
Sep 18 2023
Sep 1 2023
- Updated the patch based on the changes from D159357
- Made the check opt-in for subtargets, "none" by default, with command line option to override
- Fixed use after free in createCheckMemOperand function (the same way it is done in Hexagon)
Aug 15 2023
Reworked the patch
- replaced "use a fast checker or not" boolean argument with multiple choices
- created a separate source file and put checkAuthenticatedRegister() function there as it is likely to be later used by other parts of the codegen
- exposed a tail-call-specific AArch64FrameLowering::checkAuthenticatedLR() similarly to signLR() and authenticateLR() so it can be used by outliner callbacks later
- marked a dummy load instruction as volatile similar to Hexagon::PS_crash from HexagonInstrInfo.cpp
- added the third implementation of authenticated address checker, similar to this code. Of course, more checkers can be implemented, but I think it would be better to leave this patch as a common implementation + a few "proofs of concept" and add more checkers via later patches, if needed.
Refactoring.
Aug 14 2023
Looks like adding FeatureFPAC may require changes in more places - need to investigate whether I have to add something like AEK_FPAC constant, etc.
Aug 11 2023
Rebased onto current main branch, will upload a few fixes shortly.
Aug 10 2023
Rebased onto current main branch (after D156428). Further simplified the code by introducing the mergeOutliningCandidateAttributes() callback.
Aug 8 2023
Need to think a bit more if it is correct to just copy the PAuth-related function attributes in mergeOutliningCandidateAttributes - this would make it possible to pass less boolean arguments to signLR / authenticateLR.
Aug 7 2023
Aug 4 2023
Updated the patch.
Address the review comments.
Aug 1 2023
Jul 31 2023
This patch is inspired by the commit https://github.com/ahmedbougacha/llvm-project/commit/58cf59b84ca4e7930a640480fd5ad1ea194864f5 (and uses the same immediate operand for BRK instruction) but adds the checks during epilogue insertion instead of asm printing.
https://github.com/ahmedbougacha/llvm-project/commit/7924c7d75ae0015a9fd9786a580b10b2190bccc6
@atrosinenko do you want to try out that patch, see if it helps?
Jul 29 2023
Jul 28 2023
After debugging a bit more, I suspect that the actual reason for emitting XPACI LR is not the instruction selection phase but register allocation.
Jul 27 2023
Agree, updated the patch. Added comments in a few places where CHECK-NOT were more descriptive (spicifically, not emitting BTI before PACI(A|B)SP and not using RETAA with shadow call stack).
Jul 26 2023
While most features of https://github.com/apple/llvm-project/commit/7924c7d75ae0015a9fd9786a580b10b2190bccc6 are already implemented in mainline, @llvm.returnaddress still tends to be lowered like
xpaci x30 mov dest, x30
instead of
mov dest, x30 xpaci dest
This patch forces the return address to be copied out of LR first, if FEAT_PAuth is avaialble (at the instruction selection phase).
Jul 25 2023
Sorry for a hung patch. After getting the approval I had doubts that I interpret the corner cases of LLVM IR correctly. Thus, I postponed the patch these days to not subtly break the stable targets while fixing one issue on MSP430.
Jun 15 2023
Here is the rebased branch: https://github.com/access-softek/llvm-project/tree/rebased-ptrauth. Some tests might fail as it is work-in-progress.
Jun 10 2023
Jun 8 2023
I am planning to land this patch the next week, if there will be no objections.
Jun 1 2023
Could this patch be landed? (I can commit it on your behalf if appropriate)
Jan 30 2022
Apr 11 2021
The idea of introducing SizeOfInt to TargetLibraryInfo looks quite reasonable to me, although I am not too familiar with this part of target description.
Apr 9 2021
Sep 22 2020
Sep 11 2020
See rG553833958fd as an example.
Sep 9 2020
Ping.
Ping.
Sep 1 2020
Aug 31 2020
Thank you very much for review!
Update after the latest comments.
Re-upload after amending parent diff + add minor clarification.
Clarify rounding-related part of function.
Aug 30 2020
Re-upload after amending parent diff.
Add more clarifications, fix explanation for "why it is enough to adjust only once in case of overflow".
Aug 28 2020
Re-upload after changing parent diff.
Add some other explanations.
This update is expected to be completely NFC w.r.t. code behavior and significantly clarify the proof up to the end of half-width iterations.
Aug 27 2020
No-change re-upload: rebase onto current master branch.
No-change re-upload: rebase onto current master branch.
Pinging @echristo in case there is some trivial way to allocate a calling convention ID (but probably there is not, unless GCC allocated one already). As I understand, without this ID the debugger may show some misleading output for a limited number of LibCalls. On the other hand, it will not help either, unless the debugger knows this ID.
- Rebase onto current master branch
- Add a dummy switch case with FIXME, as suggested by @aaron.ballman
- Applied a couple of style fixes proposed by linter
Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.
Uploaded because it was already approved and no other review comments received since then. Anyway, please feel free to request followup changes if needed.
Aug 25 2020
Replaced this by D86546: [compiler-rt][builtins] Use explicitly-sized integer types for LibCalls and D86547: [compiler-rt][builtins] Use c[tl]zsi macro instead of __builtin_c[tl]z. These two patches include everything except for factoring out src_rep_t_clz() and rep_clz() to clzdi() for now.
Uploaded, thank you!