[RISCV] Generate EF_RISCV_RVC when .option rvc

[StephenFan]

Summary

Because of .option rvc will enable RISC-V C extension, the eflag value
EF_RISCV_RVC should be specified in ELF Header Eflags.

[luismarques]

This seems to match the GNU behavior, so in that sense it's good.

What about the attributes? I thought that the attributes were basically a more flexible and extensible version of the Flags. But with this behavior we get RVC in Flags but not in Tag_RISCV_arch. That seems a bit confusing and maybe error-prone, although it does also match the GNU behavior.

I'm not even sure in what cases this matters. Flags is documented as being "used by the linker to disallow linking ELF files with incompatible ABIs together" but C and non-C are always compatible with each other, assuming the target does support C.

To be clear, if no one else has any opinion about this I think I'd still be in favor of merging this, as we would match the GNU tools and I don't see much downside, but this seems like a weird and non-ideal corner case, whichever way we slice it.

[StephenFan]

In D122490#3410911, @luismarques wrote:

What about the attributes? I thought that the attributes were basically a more flexible and extensible version of the Flags. But with this behavior we get RVC in Flags but not in Tag_RISCV_arch. That seems a bit confusing and maybe error-prone, although it does also match the GNU behavior.

Yeah, the attributes were missing in this patch. I will update it.

I'm not even sure in what cases this matters. Flags is documented as being "used by the linker to disallow linking ELF files with incompatible ABIs together" but C and non-C are always compatible with each other, assuming the target does support C.

RISC-V psabi says "When linking objects which specify EF_RISCV_RVC, the linker is permitted to use RVC instructions such as C.JAL in the relaxation process".

To be clear, if no one else has any opinion about this I think I'd still be in favor of merging this, as we would match the GNU tools and I don't see much downside, but this seems like a weird and non-ideal corner case, whichever way we slice it.

I am still confused about when do we need the RVC flag ? For example, if we have both .option rvc and option norvc in an assembly file, should RVC flag be emitted ? Or if we use command line -mattr=+c to assemble file, but option norvc is specified in assembly file, should RVC flag be emitted ?

[luismarques]

In D122490#3413034, @StephenFan wrote:

I am still confused about when do we need the RVC flag ? For example, if we have both .option rvc and option norvc in an assembly file, should RVC flag be emitted ? Or if we use command line -mattr=+c to assemble file, but option norvc is specified in assembly file, should RVC flag be emitted ?

This whole thing is weird but IMO the natural extrapolation from your current patch is that we'd do an OR and set the EF_RISCV_RVC flag whenever we emit some C code, be it due to -mattr=+c or .option rvc, even if some portion of the file disables RVC with .option norvc. If the GNU tools hadn't already gone the route of setting EF_RISCV_RVC due to .option rvc this is NOT what I would suggest, but I guess it's too late now to explore other options if we want to be fully compatible.

[luismarques]

@kito-cheng Could you please summarise your comment from today's sync-up call here, please? Thank you so much.

[kito-cheng]

@luismarques I didn't read all comment before the sync-up meeting, so I guess I miss the context where you already discussed here, I agree the behavior is kind of weird for .option rvc, that will setting EF_RISCV_RVC if .option rvc is appeared in file no matter .option norvc is here or not.

And that's also cause some problem on the linker relaxation side, as @StephenFan mention, psABI spec say: "When linking objects which specify EF_RISCV_RVC, the linker is permitted to use RVC instructions such as C.JAL in the relaxation process", so according the definition if user just want to a specific region having c extension, oh, that corrupt whole file, linker might create C instruction in the file other than the specific region, although the most usage is disable C extension for specific region for now, but that might break that too.

So I would suggest LLVM *DO NOT* follow this behavior and gonna to deprecate .option rvc/.option norvc, and recommend user using .option arch, +c/.option arch, -c instead.

But I guess the problem is we didn't have .option arch, +c implementation for LLVM, and also we didn't merge the PR[1] in riscv-asm-manual yet.

@StephenFan do you or other PLCT folks could help on implement that for LLVM?

@luismarques @asb did you help to review `.option arch, stuffs, I think we could push that forward together :)

[1] https://github.com/riscv-non-isa/riscv-asm-manual/pull/67

[kito-cheng]

A few more word on deprecating .option rvc/.option norvc: we didn't find good reason to deprecated that before, but I think we have one now :P

[StephenFan]

In D122490#3419774, @kito-cheng wrote:

@StephenFan do you or other PLCT folks could help on implement that for LLVM?

I will implement this for LLVM.

[luismarques]

I think this patch can be abandoned now?
Thanks for providing D123515 as an alternative.

[StephenFan]

In D122490#3461606, @luismarques wrote:

I think this patch can be abandoned now?
Thanks for providing D123515 as an alternative.

Yes. Thanks for reminding me.

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

 MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
   return static_cast<MCELFStreamer &>(Streamer);
 }
 
 void RISCVTargetELFStreamer::emitDirectiveOptionPush() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionPop() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionPIC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoPIC() {}
-void RISCVTargetELFStreamer::emitDirectiveOptionRVC() {}
+void RISCVTargetELFStreamer::emitDirectiveOptionRVC() {
+  MCAssembler &MCA = getStreamer().getAssembler();
+  unsigned Eflags = MCA.getELFHeaderEFlags();
+  Eflags |= ELF::EF_RISCV_RVC;
+  MCA.setELFHeaderEFlags(Eflags);
+}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRVC() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionRelax() {}
 void RISCVTargetELFStreamer::emitDirectiveOptionNoRelax() {}
 
 void RISCVTargetELFStreamer::emitAttribute(unsigned Attribute, unsigned Value) {
   setAttributeItem(Attribute, Value, /*OverwriteExisting=*/true);
 }
 

llvm/test/MC/RISCV/option-rvc-eflag.s

+# RUN: llvm-mc -triple riscv32 -filetype=obj %s -o - \
+# RUN:   | llvm-readobj -h - | FileCheck -check-prefix=CHECK %s
+# RUN: llvm-mc -triple riscv64 -filetype=obj %s -o - \
+# RUN:   | llvm-readobj -h - | FileCheck -check-prefix=CHECK %s
+
+# CHECK:       Flags [ (0x1)
+# CHECK-NEXT:    EF_RISCV_RVC (0x1)
+# CHECK-NEXT:  ]
+.option rvc