[RISCV][MC] Adjust conditions to emit R_RISCV_ADD*/R_RISCV_SUB* pairs

[MaskRay]

Summary

D132262 tried to simplify IsMetadataOrEHFrameSection originally introduced in
D127549 but caused a regression as .quad directives in

.section .note,"a",@note; note:
.quad extern-note    # extern is undefined

.section .rodata,"a",@progbits; rodata:
.quad extern-rodata  # extern is undefined

.section .nonalloc,"",@progbits; nw:
.quad extern-nw

are incorrectly rejected: these differences may be link-time constants and
are allowed in GNU assembler and LLVM MC's non-RISC-V ports.

Relax the conditions to allow these cases. For A-B, A may be defined later, but
this requiresFixups call has to eagerly make a decision. For now, emit ADD/SUB
unless A is .L*. This euristic handles many temporary label differences for
.debug_* and .apple_types sections. Ideally we should delay the decision of
PC-relative vs ADD/SUB until A is defined.

[compnerd]

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

161

This really seems like the original cases were correct but need to be augmented further to handle data sections. If there is a cross-section textual reference, that should generate the relocation pair. The data section is interesting because the .eh_frame, .apple_names, .apple_types, and .debug_* do not want the pairwise relocation. Adjusting this to handle it that way would not only fix this, but also likely fix the cases of fission that we currently fail on.

[MaskRay]

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

161

I think we want to avoid reliance on isText() (Kind as set by MCContext::getELFSection). What are the changes you envision?

I think it's right to hard code .eh_frame, .apple_names, and .apple_types for now, and don't try using a generic rule to handle them. .eh_frame may possibly be removed if we can fix MCDwarf.cpp (not sure how much effort it requires).

[compnerd]

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

161

I think that we should prefer isText over the section name checks. RISCV seems to want to prefer the pair wise relocation in all "text" cases and in very specific data cases (as .debug_*, .eh_frame, .apple_names, .apple_types etc are all data sections where we don't want the pair wise relocation).

[MaskRay]

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

161

I prefer avoiding isText check. It relies on .Default(SectionKind::getText()); in MCContext::getELFSection which I am still unsure.
In GNU assembler, .rodata and .note sections use R_*_{ADD,SUB}* as well.

The code as-is appears to capture the GNU assembler behavior best, though perhaps there may be some cases that should be rejected while we allow. Hence I say "I suspect we may allow some cases that should be rejected, but this is better than incorrect rejection." in the last sentence of the summary.

[MaskRay]

Fix -gdwarf-aranges

[MaskRay]

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

161

OK, I agree that we should use isText here, though I think we should revisit the .Default(SectionKind::getText()) decision in
MCContext::getELFSection for your D133456.

For .long w1-w in the test, gas doesn't emit a pair of ADD/SUB relocations. We currently do and the patch retains the behavior. To properly fix it and match gas, we should delay the relocation decision, which is difficult to implement and definitely out of the scope of this patch.

[compnerd]

Yeah, I think that we will have to slowly erode the differences, the model for the pair-wise relocations here is a challenge.

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

bool RISCVELFStreamer::requiresFixups(MCContext &C, const MCExpr *Value,
   // as we do not track the state per symbol or section.  However, BFD will
   // always emit the relocation and so we follow suit which avoids the need to
   // track that information.
   if (A.isInSection() && A.getSection().getKind().isText())
     return true;
   if (B.isInSection() && B.getSection().getKind().isText())
     return true;
 
+  // If A is undefined and B is defined, we should emit ADD/SUB for A-B.
+  // Unfortunately, A may be defined later, but this requiresFixups call has to
+  // eagerly make a decision. For now, emit ADD/SUB unless A is .L*. This
+  // heuristic handles many temporary label differences for .debug_* and
+  // .apple_types sections.
+  //
+  // TODO Implement delayed relocation decision.
+  if (!A.isInSection() && !A.isTemporary() && B.isInSection())
+    return true;
+
   // Support cross-section symbolic differences ...
   return A.isInSection() && B.isInSection() &&
          A.getSection().getName() != B.getSection().getName();
 }
 
 void RISCVELFStreamer::reset() {
   static_cast<RISCVTargetStreamer *>(getTargetStreamer())->reset();
   MCELFStreamer::reset();

llvm/test/MC/RISCV/riscv64-64b-pcrel.s

 # RUN: llvm-mc -triple riscv64-unknown-linux-gnu -filetype obj -o - %s \
 # RUN:   | llvm-readobj -r - | FileCheck %s
 # RUN: not llvm-mc -triple riscv64-unknown-linux-gnu -filetype obj --defsym ERR=1 -o /dev/null %s 2>&1 \
 # RUN:   | FileCheck %s --check-prefix=ERROR
 
 # CHECK:      Relocations [
+# CHECK-NEXT:   Section ({{.*}}) .rela.note {
+# CHECK-NEXT:     0x0 R_RISCV_ADD64 extern 0x0
+# CHECK-NEXT:     0x0 R_RISCV_SUB64 note 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section ({{.*}}) .rela.rodata {
+# CHECK-NEXT:     0x0 R_RISCV_ADD64 extern 0x0
+# CHECK-NEXT:     0x0 R_RISCV_SUB64 rodata 0x0
+# CHECK-NEXT:   }
 # CHECK-NEXT:   Section ({{.*}}) .rela.alloc_w {
 # CHECK-NEXT:     0x0 R_RISCV_ADD64 extern 0x0
 # CHECK-NEXT:     0x0 R_RISCV_SUB64 w 0x0
 # CHECK-NEXT:     0x8 R_RISCV_ADD64 w 0x0
 # CHECK-NEXT:     0x8 R_RISCV_SUB64 extern 0x0
 # CHECK-NEXT:     0x10 R_RISCV_ADD32 x 0x0
 # CHECK-NEXT:     0x10 R_RISCV_SUB32 w 0x0
 # CHECK-NEXT:     0x14 R_RISCV_ADD32 w1 0x0
 # CHECK-NEXT:     0x14 R_RISCV_SUB32 w 0x0
 # CHECK-NEXT:     0x18 R_RISCV_ADD32 .L.str 0x0
 # CHECK-NEXT:     0x18 R_RISCV_SUB32 w 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Section ({{.*}}) .rela.alloc_x {
 # CHECK-NEXT:     0x0 R_RISCV_ADD64 y 0x0
 # CHECK-NEXT:     0x0 R_RISCV_SUB64 x 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT:   Section ({{.*}}) .rela.alloc_y {
 # CHECK-NEXT:     0x0 R_RISCV_ADD64 x 0x0
 # CHECK-NEXT:     0x0 R_RISCV_SUB64 y 0x0
 # CHECK-NEXT:   }
+# CHECK-NEXT:   Section ({{.*}}) .rela.nonalloc_w {
+# CHECK-NEXT:     0x0 R_RISCV_ADD64 extern 0x0
+# CHECK-NEXT:     0x0 R_RISCV_SUB64 nw 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section ({{.*}}) .rela.nonalloc_x {
+# CHECK-NEXT:     0x0 R_RISCV_ADD64 ny 0x0
+# CHECK-NEXT:     0x0 R_RISCV_SUB64 nx 0x0
+# CHECK-NEXT:   }
 # CHECK-NEXT:   Section ({{.*}}) .rela.nonalloc_y {
 # CHECK-NEXT:     0x0 R_RISCV_ADD64 nx 0x0
 # CHECK-NEXT:     0x0 R_RISCV_SUB64 ny 0x0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
-.ifdef ERR
 .section .note,"a",@note; note:
-# ERROR: :[[#@LINE+1]]:7: error: unsupported relocation type
 .quad extern-note
 .section .rodata,"a",@progbits; rodata:
-# ERROR: :[[#@LINE+1]]:7: error: unsupported relocation type
 .quad extern-rodata
-.endif
 
 .section .alloc_w,"aw",@progbits; w:
 .quad extern-w   # extern is undefined
 .quad w-extern
 .long x-w        # A is later defined in another section
 .long w1-w       # A is later defined in the same section
 .long .L.str-w   # A is temporary
 w1:
 
 .section .alloc_x,"aw",@progbits; x:
 .quad y-x
 .section .alloc_y,"aw",@progbits; y:
 .quad x-y
 
 .section .nonalloc_w; nw:
-.ifdef ERR
-# ERROR: :[[#@LINE+1]]:7: error: unsupported relocation type
 .quad extern-nw
+.ifdef ERR
 # ERROR: :[[#@LINE+1]]:7: error: symbol 'extern' can not be undefined in a subtraction expression
 .quad nw-extern
 .endif
 .section .nonalloc_x; nx:
-.ifdef ERR
-# ERROR: :[[#@LINE+1]]:7: error: unsupported relocation type
 .quad ny-nx
-.endif
 .section .nonalloc_y; ny:
 .quad nx-ny
 
 ## -gdwarf-aranges generated assembly expects no relocation.
 ## Otherwise, a .Lsec_end0 symbol (defined at the end of .rodata.str1.1)
 ## will be rejected by linkers.
 .section .rodata.str1.1,"aMS",@progbits,1
 .L.str: