labrinea added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1665
+
+ CmdArgs.push_back("-mllvm");
+ if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))
----------------
ostannard wrote:
> Are these optional also being passed through to the linker when doing LTO?
No, the mitigation is only performed in the compiler. Also, I believe that
`-flto` and `-mcmse` are incompatible options.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1666
+ CmdArgs.push_back("-mllvm");
+ if (A->getOption().matches(options::OPT_mfix_cmse_cve_2021_35465))
+ CmdArgs.push_back("-arm-fix-cmse-cve-2021-35465=1");
----------------
SjoerdMeijer wrote:
> If `-mcpu=cortex-[m33|m35|m55]` was provided, then
> `-arm-fix-cmse-cve-2021-35465=1` is already set and we are adding another
> option here? For example, for
>
> -mcpu=cortex-m33 -mcmse -mfix-cmse-cve-2021-35465
>
> I am expecting:
>
> "-mllvm" "-arm-fix-cmse-cve-2021-35465=1" "-mllvm"
> "-arm-fix-cmse-cve-2021-35465=1"
>
> and with `-mno-fix-cmse-cve-2021-35465`:
>
> "-mllvm" "-arm-fix-cmse-cve-2021-35465=1" "-mllvm"
> "-arm-fix-cmse-cve-2021-35465=0"
>
> Probably it's nicer to just pass this once.
>
> Also, in the tests, I think cases are missing for `-mcpu=...` and
> `-m[no-]fix-cmse-cve-2021-35465`.
Your interpretetion is correct. The established policy is "last option wins",
but I could make the Driver pass only one option if that's preferable.
================
Comment at: clang/test/Driver/arm-cmse-cve-2021-35465.c:3
+//
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8-m.main %s -### \
+// RUN: -mcmse -mno-fix-cmse-cve-2021-35465 2>&1 |\
----------------
ostannard wrote:
> The last few paragraphs of
> https://developer.arm.com/support/arm-security-updates/vlldm-instruction-security-vulnerability
> claim that this is enabled by default for -march=armv8-m.main in AC6 and
> GCC, is there a reason we're not matching that?
Yes, the inconsistency lies on the fact that GCC implements the mitigation in
library code, therefore it is always present for `-march=armv8-m.main`, whereas
in llvm there's no such limitation. We've contacted the authors of this page to
rectify the documentation.
================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1564
.addReg(Reg)
+ .addReg(ARM::CPSR, RegState::ImplicitDefine)
.add(predOps(ARMCC::AL));
----------------
ostannard wrote:
> Why are these needed?
These prevent the reordering with the mitigation sequence. It answers your next
question.
================
Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1626
+ // Thumb2ITBlockPass will not recognise the instruction as conditional.
+ BuildMI(MBB, MBBI, DL, TII->get(ARM::t2IT))
+ .addImm(ARMCC::NE)
----------------
ostannard wrote:
> This pass runs before the final scheduler pass, so is there a risk that the
> IT and VMOV instructions could be moved apart? I think it would be safer to
> either put the IT instruction inside the inline asm block, or make this a new
> pseudo-instruction expanded in the asm printer.
The use of `.addReg(ARM::CPSR, RegState::ImplicitDefine)` prevents the
reordering. There are regression tests in place that check the mitigation
sequence ordering.
Is this what you meant? Where you refering specifically to the case where
`!STI->hasFPRegs()`, when we generate inline asm, or to both scenarios?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109157/new/
https://reviews.llvm.org/D109157
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
