[X86] Disable nop padding before instruction following hardcode

[skan]

Avoid the warning reported by clang-tidy

[LuoYuanke]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

507

Does hard coded instruction occupy one separate fragment? Need comments to explain how to check hard code. Better to have a diagram.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

507

Hard coded instruction may occupy one separate fragment. It's not a good way to enumerate all the cases but I will add more comments here.

[skan]

Address review comments

[skan]

Update comment

[LuoYuanke]

LGTM.

[skan]

Rebase

[reames]

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion. I don't think this is really a problem for the compiler use case (as we already have the suppression mechanism), but since you seem set on supporting arbitrary assembly I wanted to point that out. I still think that is a bad idea and that you're better off defining syntax for an opt in system.

We probably need some form of a marker on the fragment or a separate fragment to mark a boundary it is unsafe to pad.

[skan]

In D76176#1930168, @reames wrote:

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion.

Yes, this patch doesn't prevent prefix insertion since it does not make more instructions prefixable and this patch does exactly what it describes, namely, "Disable nop padding before ...". D76286 makes more instruction prefixable (be emitted into RelaxableFragment), so I also prevent prefix insertion before the new "relaxable" instruction if it follows prefix or hardcode. As to the old relaxable instruction (relative jump), I thought you believed inserting prefixes before it is not a problem even if it follows prefix or hardcode, since you didn't consider that in D75300.
But according to your comment here, I think I misunderstood you :-( .

We probably need some form of a marker on the fragment or a separate fragment to mark a boundary it is unsafe to pad.

I think you mean "mark a branch" rather than "mark a boundary". I agree we can add a marker to the relaxable fragment to do this. Do you want to write this patch by yourself ? If you do not, I am happy to write it.

I don't think this is really a problem for the compiler use case (as we already have the suppression mechanism), but since you seem set on supporting arbitrary assembly I wanted to point that out.

Yes, we want to support arbitrary assembly since we do have that need.

I still think that is a bad idea and that you're better off defining syntax for an opt in system.

To definie such a syntax, we need to keep consistent with gas, which may take more time. I think we can prevent nop/prefix insertion after hardcode/prefix first, and after the syntax is determined, we can remove the code for checking hardcode/prefix if necessary.

[skan]

In D76176#1930168, @reames wrote:

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion.

Complete solution here D76475.

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

class X86AsmBackend : public MCAsmBackend {
   uint8_t determinePaddingPrefix(const MCInst &Inst) const;
 
   bool isMacroFused(const MCInst &Cmp, const MCInst &Jcc) const;
 
   bool needAlign(MCObjectStreamer &OS) const;
   bool needAlignInst(const MCInst &Inst) const;
   MCInst PrevInst;
   MCBoundaryAlignFragment *PendingBoundaryAlign = nullptr;
+  std::pair<MCFragment *, size_t> PrevInstPosition;
 
 public:
   X86AsmBackend(const Target &T, const MCSubtargetInfo &STI)
       : MCAsmBackend(support::little), STI(STI),
         MCII(T.createMCInstrInfo()) {
     if (X86AlignBranchWithin32BBoundaries) {
       // At the moment, this defaults to aligning fused branches, unconditional
       // jumps, and (unfused) conditional jumps with nops.  Both the
static bool hasInterruptDelaySlot(const MCInst &Inst) {
   case X86::MOV16sm:
     if (Inst.getOperand(0).getReg() == X86::SS)
       return true;
     break;
   }
   return false;
 }
 
+/// Check if the instruction to be emitted is right after any data.
+static bool
+isRightAfterData(MCFragment *CurrentFragment,
+                 const std::pair<MCFragment *, size_t> &PrevInstPosition) {
+  MCFragment *F = CurrentFragment;
+  // Empty data fragments may be created to prevent further data being
+  // added into the previous fragment, we need to skip them since they
+  // have no contents.
+  for (; isa_and_nonnull<MCDataFragment>(F); F = F->getPrevNode())
+    if (cast<MCDataFragment>(F)->getContents().size() != 0)
+      break;
+
+  // Since data is always emitted into a DataFragment, our check strategy is
+  // simple here.
+  //   - If the fragment is a DataFragment
+  //     - If it's not the fragment where the previous instruction is,
+  //       returns true.
+  //     - If it's the fragment holding the previous instruction but its
+  //       size changed since the the previous instruction was emitted into
+  //       it, returns true.
+  //     - Otherwise returns false.
+  //   - If the fragment is not a DataFragment, returns false.
+  if (auto *DF = dyn_cast_or_null<MCDataFragment>(F))
+    return DF != PrevInstPosition.first ||
+           DF->getContents().size() != PrevInstPosition.second;
+
+  return false;
+}
+
+/// \returns the fragment size if it has instructions, otherwise returns 0.
+static size_t getSizeForInstFragment(const MCFragment *F) {
+  if (!F || !F->hasInstructions())
+    return 0;
+  // MCEncodedFragmentWithContents being templated makes this tricky.
+  switch (F->getKind()) {
+  default:
+    llvm_unreachable("Unknown fragment with instructions!");
+  case MCFragment::FT_Data:
+    return cast<MCDataFragment>(*F).getContents().size();
+  case MCFragment::FT_Relaxable:
+    return cast<MCRelaxableFragment>(*F).getContents().size();
+  case MCFragment::FT_CompactEncodedInst:
+    return cast<MCCompactEncodedInstFragment>(*F).getContents().size();
+  }
+}
+
 /// Check if the instruction operand needs to be aligned. Padding is disabled
 /// before intruction which may be rewritten by linker(e.g. TLSCALL).
 bool X86AsmBackend::needAlignInst(const MCInst &Inst) const {
   // Linker may rewrite the instruction with variant symbol operand.
   if (hasVariantSymbol(Inst))
     return false;
 
   const MCInstrDesc &InstDesc = MCII->get(Inst.getOpcode());
if (hasInterruptDelaySlot(PrevInst))
     // instruction delay, inserting a nop would change behavior.
     return;
 
   if (isPrefix(PrevInst, *MCII))
     // If this instruction follows a prefix, inserting a nop would change
     // semantic.
     return;
 
+  if (isRightAfterData(OS.getCurrentFragment(), PrevInstPosition))
+    // If this instruction follows any data, there is no clear
+    // instruction boundary, inserting a nop would change semantic.
+    return;
+
   if (!isMacroFused(PrevInst, Inst))
     // Macro fusion doesn't happen indeed, clear the pending.
     PendingBoundaryAlign = nullptr;
 
   if (PendingBoundaryAlign &&
       OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign) {
     // Macro fusion actually happens and there is no other fragment inserted
     // after the previous instruction.

 }
 
 /// Set the last fragment to be aligned for the BoundaryAlignFragment.
 void X86AsmBackend::emitInstructionEnd(MCObjectStreamer &OS, const MCInst &Inst) {
   if (!needAlign(OS))
     return;
 
   PrevInst = Inst;
+  MCFragment *CF = OS.getCurrentFragment();
+  PrevInstPosition = std::make_pair(CF, getSizeForInstFragment(CF));
 
   if (!needAlignInst(Inst) || !PendingBoundaryAlign)
     return;
 
   // Tie the aligned instructions into a a pending BoundaryAlign.
-  PendingBoundaryAlign->setLastFragment(OS.getCurrentFragment());
+  PendingBoundaryAlign->setLastFragment(CF);
   PendingBoundaryAlign = nullptr;
 
   // We need to ensure that further data isn't added to the current
   // DataFragment, so that we can get the size of instructions later in
   // MCAssembler::relaxBoundaryAlign. The easiest way is to insert a new empty
   // DataFragment.
-  if (isa_and_nonnull<MCDataFragment>(OS.getCurrentFragment()))
+  if (isa_and_nonnull<MCDataFragment>(CF))
     OS.insert(new MCDataFragment());
 
   // Update the maximum alignment on the current section if necessary.
   MCSection *Sec = OS.getCurrentSectionOnly();
   if (AlignBoundary.value() > Sec->getAlignment())
     Sec->setAlignment(AlignBoundary);
 }
 

llvm/test/MC/X86/align-branch-64-hardcode.s

+  # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu --x86-align-branch-boundary=32 --x86-align-branch=jmp+call %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
+
+  # Exercise cases where instructions to be aligned is after hardcode
+  # and thus can't add a nop in between without changing semantic.
+
+  .text
+
+  # CHECK: 1d:       int3
+  # CHECK: 1e:       jmp
+  # CHECK: 24:       int3
+  .p2align  5
+  .rept 30
+  int3
+  .endr
+  .byte 0x2e
+  jmp baz
+  int3
+
+  # CHECK: 5d:       int3
+  # CHECK: 5e:       call
+  # CHECK: 66:       int3
+  .p2align  5
+  .rept 30
+  int3
+  .endr
+  .byte 0x66
+  call *___tls_get_addr@GOT(%ecx)
+  int3
+
+  .section ".text.other"
+bar:
+  retq