[RISCV] Fix crash for initial section alignment with .option norvc

[luismarques]

Summary

The existing code wasn't getting the subtarget info from the fragment, so the current status of RVC would be ignored. This would cause a crash for the new test case when the target then reported it couldn't write the requested number of code alignment bytes.

[luismarques]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

610–612

This one wasn't needed to fix the crash but, presumably, this check should also be fixed?

[luismarques]

Ping.

[StephenFan]

This patch makes sense to me and I think it is a correct fix.
In LLVM MC, the STI's feature bits is not mutable, it was determined by the command line before parsing the file. And to deal with directives like .option rvc, the Parser maintains an STI and replaces it with a new STI when the subtarget extensions were enabled or disabled. So we need to use MCAlignFragment's STI which is generated using the replaced STI by the parser. Is my understanding correct?

[StephenFan]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

610–612

I agree this check should also be fixed. And if it would be better to have a test that can test this fix?

[luismarques]

Add test for shouldInsertFixupForCodeAlign fix.

[luismarques]

This patch makes sense to me and I think it is a correct fix.
In LLVM MC, the STI's feature bits is not mutable, it was determined by the command line before parsing the file. And to deal with directives like .option rvc, the Parser maintains an STI and replaces it with a new STI when the subtarget extensions were enabled or disabled. So we need to use MCAlignFragment's STI which is generated using the replaced STI by the parser. Is my understanding correct?

I wouldn't focus too much on it being mutable or not. I think the important point is that because we aren't doing everything in a single pass we need to track the state of the flags at various points in the assembly file. Which we do, but apparently we weren't then using the saved state for those locations, just the global state. Being mutable (and not copying it) would only help if we were doing everything in a single pass. At least that was my interpretation of the issue. Sorry to nitpick your explanation, I just wanted to clarify that point :) Broadly speaking I would say your explanation is correct.

[luismarques]

Can we get this landed on time for backporting into 14.0.1? (it took me some time to address the review feedback because I was sick).

[asb]

This LGTM - thanks!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 
 // Linker relaxation may change code size. We have to insert Nops
 // for .align directive when linker relaxation enabled. So then Linker
 // could satisfy alignment by removing Nops.
 // The function return the total Nops Size we need to insert.
 bool RISCVAsmBackend::shouldInsertExtraNopBytesForCodeAlign(
     const MCAlignFragment &AF, unsigned &Size) {
   // Calculate Nops Size only when linker relaxation enabled.
-  if (!STI.getFeatureBits()[RISCV::FeatureRelax])
+  const MCSubtargetInfo *STI = AF.getSubtargetInfo();
+  if (!STI->getFeatureBits()[RISCV::FeatureRelax])
     return false;
 
-  bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
+  bool HasStdExtC = STI->getFeatureBits()[RISCV::FeatureStdExtC];
   unsigned MinNopLen = HasStdExtC ? 2 : 4;
 
   if (AF.getAlignment() <= MinNopLen) {
     return false;
   } else {
     Size = AF.getAlignment() - MinNopLen;
     return true;
   }
 }
 
 // We need to insert R_RISCV_ALIGN relocation type to indicate the
 // position of Nops and the total bytes of the Nops have been inserted
 // when linker relaxation enabled.
 // The function insert fixup_riscv_align fixup which eventually will
 // transfer to R_RISCV_ALIGN relocation type.
 bool RISCVAsmBackend::shouldInsertFixupForCodeAlign(MCAssembler &Asm,
                                                     const MCAsmLayout &Layout,
                                                     MCAlignFragment &AF) {
   // Insert the fixup only when linker relaxation enabled.
-  if (!STI.getFeatureBits()[RISCV::FeatureRelax])
+  const MCSubtargetInfo *STI = AF.getSubtargetInfo();
+  if (!STI->getFeatureBits()[RISCV::FeatureRelax])
     return false;
 
   // Calculate total Nops we need to insert. If there are none to insert
   // then simply return.
   unsigned Count;
   if (!shouldInsertExtraNopBytesForCodeAlign(AF, Count) || (Count == 0))
     return false;
 
   MCContext &Ctx = Asm.getContext();

llvm/test/MC/RISCV/align-option-relax.s

+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=-relax < %s \
+# RUN:     | llvm-readobj -r - | FileCheck %s
+
+# Check that .option relax overrides -mno-relax and enables R_RISCV_ALIGN
+# relocations.
+# CHECK: R_RISCV_ALIGN
+  .option relax
+  .align 4

llvm/test/MC/RISCV/align.s

 # C-EXT-RELAX-RELOC-NOT: R_RISCV_ALIGN
 data1:
   .word 7
   .p2align        4
 # RELAX-RELOC-NOT: R_RISCV_ALIGN
 # C-EXT-RELAX-RELOC-NOT: R_RISCV_ALIGN
 data2:
   .word 9
+# Check that the initial alignment is properly handled when using .option to
+# disable the C extension. This used to crash.
+# C-EXT-RELAX-INST:      <.text2>:
+# C-EXT-RELAX-INST-NEXT: add a0, a0, a1
+  .section .text2, "x"
+  .option norvc
+  .balign 4
+  add a0, a0, a1