[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

We uncovered another functional issue with this patch, or at least, the interaction of this patch and other parts of LLVM.  In our support for STATEPOINT, PATCHPOINT, and STACKMAP we use N-byte nop sequences for regions of code which might be patched out.  It's important that these regions are exactly N bytes as concurrent patching which doesn't replace an integral number of instructions is ill-defined on X86-64.  This patch causes the N-byte nop sequence to sometimes become (N+M) bytes which breaks the patching.  I believe that the XRAY support may have a similar issue.

More generally, I'm worried about the legality of arbitrarily prefixing instructions from unknown sources.  In the particular example we saw, we had something along the following:

.Ltmp0:
	.p2align	3, 0x90
	(16 byte nop sequence)
.Ltmp3:
	jmp *%rax

In addition to the patching legality issue above, padding the nop sequence does something else interesting in this example.  It changes the alignment of Ltmp3.  Before, Ltmp3 was always 8 byte aligned, after prefixes are added, it's not.  It's not clear to me exactly what the required semantics here are, but we at least had been assuming the alignment of Ltmp3 was guaranteed in this case.  (That's actually how we found the patching issue.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70157/new/

https://reviews.llvm.org/D70157