On 06/12/2022 15:46, Andrea Corallo wrote:
Hi Richard,
thanks for reviewing.
Just one clarification before I complete the respin of this patch.
Richard Earnshaw <richard.earns...@foss.arm.com> writes:
[...]
Also, I think (out of an abundance of caution) we really need a
scheduling barrier placed before calls to gen_aut_nop() pattern is
emitted, to ensure that the scheduler never tries to move this
instruction away from the position we place it. Use gen_blockage()
for that (see TARGET_SCHED_PROLOG). Alternatively, we could make the
UNSPEC_PAC_NOP an unspec_volatile, which has the same effect (IIRC)
without needing an additional insn - if you use this approach, then
please make sure this is explained in a comment.
+(define_insn "pacbti_nop"
+ [(set (reg:SI IP_REGNUM)
+ (unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)]
+ UNSPEC_PACBTI_NOP))]
+ "arm_arch8m_main"
+ "pacbti\t%|ip, %|lr, %|sp"
+ [(set_attr "conds" "unconditional")])
The additional side-effect of this being a BTI landing pad means that
we mustn't move any other instruction before it. So I think this
needs to be an unspec_volatile as well.
IIUC from this we want to make all the three (UNSPEC_PAC_NOP,
UNSPEC_PACBTI_NOP, UNSPEC_AUT_NOP) unspec volatile, correct?
UNSPEC_PAC_NOP doesn't need to be volatile. The register constraints
will be enough to ensure it is run before any instruction that consumes
the result it produces.
UNSPEC_PAC_BTI_NOP needs to be volatile, as it's essential that when we
have an instruction (for example ldr r3, [r3]) early in the program that
doesn't interact with the prologue then it cannot be migrated before the
BTI as the BTI is a landing pad and must be the first instruction in the
function. This is why UNSPEC_BTI_NOP is volatile.
UNSPEC_AUT_NOP must be volatile because we want to ensure that no
instruction is moved after this one and before the return as that might
expose a ROP gadget to hackers.
R.
IIUC correctly the scheduler should not reorder them as we have
expressed which register they consume and produce but is for double
caution correct?
On the tests, they are OK as they stand, but we lack anything that
will be tested when suitable hardware is unavailable (all tests are
"dg-do run"). Can we please have some compile-only tests as well?
Ack.
BR
Andrea
