User Details
- User Since
- Jun 18 2014, 12:59 AM (457 w, 4 d)
Tue, Mar 7
Mon, Mar 6
It seems to me that this patch would also need:
- Some regression test to test it actually changes code generation (only?) when targeting Arm.
- As is, this patch will not result in changed code generation IIUC?
Adding Snehasish as a reviewer as git blame suggests snehasish has designed and written most of the MachineFunctionSplitter.
Feb 22 2023
Feb 14 2023
LGTM, thank you!
Jan 20 2023
Jan 17 2023
LGTM, thank you!
Jan 9 2023
I just have a few more very minor nit-picky comments. Feel free to disagree with them.
I'm happy for this to go in.
I haven't looked in detail into the comments @DavidSpickett mad , so please also explicitly get his approval before committing.
Dec 7 2022
My apologies for missing this outstanding review for so long.
Nov 29 2022
Nov 22 2022
Oct 28 2022
Oct 27 2022
Oct 19 2022
Looks great. Thank you for becoming an office hours host!
Oct 10 2022
Oct 9 2022
Oct 7 2022
I just have one minor suggestion.
Otherwise, LGTM.
Oct 5 2022
LGTM, thanks!
I finally found time to go through the patch from beginning to end. I only have 2 more - hopefully minor questions.
Apart from that, LGTM.
Sep 29 2022
Sep 28 2022
Sep 27 2022
Sep 21 2022
Sep 15 2022
Sep 13 2022
I seem to remember that the speculative_load_hardening for AArch64 has a work-around when a user does reserve register X16 in inline assembly, see comment at https://github.com/llvm/llvm-project/blob/b7dae832e61da1f8b48cce1715514cbd5809eb3f/llvm/lib/Target/AArch64/AArch64SpeculationHardening.cpp#L35.
Given that comment and the tests in https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/speculation-hardening-dagisel.ll that check that behaviour, the warning/error implemented in this patch is not correct?
Aug 31 2022
I had just 2 more minor nitpicks - mostly about adding 2 more regression tests to test/document expected behavior in corner cases.
LGTM!
I just added a few minor nitpicks - I'll leave it to you to decide if following those suggestions would result in an improvement or not.
Aug 24 2022
See https://reviews.llvm.org/D56305 and https://reviews.llvm.org/D48580 for previous related discussions.
I think it would be helpful to understand the use case for being able to reserve x8, x16, x17 and x19 better.
Aug 23 2022
My understanding is that X8, X16, X17 and X19 cannot be reserved because the code generator in places will make use of them.
For example, using X19 as a base register in some cases. X16 and X17 are defined by the ABI to potentially be clobbered on function calls & when a veneer needs to be inserted by a linker, it does get clobbered. IIRC, some of the security mitigations implemented in LLVM also clobber these 2 registers on function calls.
I'm not fully sure why X8 cannot be reserved.
Aug 19 2022
Aug 18 2022
Aug 17 2022
As this is something I suggested - this LGTM.
Aug 10 2022
gentle ping.
Jun 30 2022
Jun 23 2022
Jun 20 2022
Jun 15 2022
Jun 3 2022
May 24 2022
May 23 2022
See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html, documentation for "mharden-sls": For AArch64, the options available on the command line are "retbr", "blr", "none" and "all".
I don't think the options necessarily have to be the same for x86.
But assuming I understand this patch correctly, it seems to me that with this patch -mharden-sls=all would mean fundamentally slightly different things for x86 vs arm and aarch64, which could be confusing to users.
IIUC this patch correctly, this patch implements the equivalent of aarch64/arm's -mharden-sls=retbr (i.e. add a straight-line-speculation mitigation for returns and indirect jumps, but not for indirect function calls).
Therefore, I wonder if it wouldn't be better to name this -mharden-sls=retbr for more consistency across architectures?
Or is the indirect function call case not relevant for x86 (sorry - I'm not up to speed on the details on the x86 side)?
May 20 2022
May 19 2022
updated guidance based on Aaron's feedback.
Use correct syntax to indicate we're adding a subsubsection, not a section, see https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections
May 18 2022
May 13 2022
May 3 2022
Apr 28 2022
LGTM
Mar 24 2022
Feb 21 2022
Feb 17 2022
Feb 16 2022
Feb 13 2022
LGTM, thank you!
Feb 8 2022
Feb 7 2022
LGTM, thank you @voltur01 !
Jan 21 2022
Jan 13 2022
I'm afraid I don't know if it's possible to check anywhere in public documentation that the values 0xc0 and 0xac3 are correct.
I'm assuming you verified those are the correct.
The code looks good, apart from one place where clang-format suggests different indentation.
With that indentation adapted, this looks good to me.
Jan 12 2022
Jan 11 2022
LGTM, thank you!
Dec 20 2021
I think this would be useful to have, as writing fine-grained filter rules is not necessarily easy or even doable in some mail clients, such as gmail.
With that in mind, I wonder if this action (which if I understand correctly will add a comment mentioning "@issue-subscribers-label"), results in the corresponding notification email adding "issue-subscribes-label@noreply.github.com"? I hope it does as that probably would make it easier to write email filters.
Dec 16 2021
Dec 14 2021
Nov 19 2021
I just have 2 bike-sheddy comments on the documentation text.
My comments should not let you delay in getting this committed if they do not make sense to you.