[RISC-V] Fix receiving of 2nd register in struct

[t-mustafin]

Fixes output to console.
Part of #84834.
cc @alpencolt @gbalykov @clamp03

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Part of #84834.
cc @alpencolt @gbalykov @clamp03

Author:	t-mustafin
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[clamp03]

@t-mustafin Could you give examples (or tests) which this PR can fix? I just want to try and see emitted codes. Thank you.

[t-mustafin]

@t-mustafin Could you give examples (or tests) which this PR can fix? I just want to try and see emitted codes. Thank you.

@clamp03 I got exception with this backtrace for helloworld.dll launch without output redirection:

tmustafin@starfive:~/main_riscv_PR84797$ ./corerun ../hello/hello.dll
Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Globalization.Ordinal.IndexOfOrdinalIgnoreCase(System.ReadOnlySpan`1<Char>, System.ReadOnlySpan`1<Char>)
   at System.Globalization.Ordinal.IndexOf(System.String, System.String, Int32, Int32, Boolean)
   at System.String.IndexOf(System.String, Int32, Int32, System.StringComparison)
   at System.String.IndexOf(System.String, System.StringComparison)
   at System.String.Contains(System.String, System.StringComparison)
   at System.TerminalFormatStrings..ctor(Database)
   at System.ConsolePal+<>c.<.cctor>b__114_0()
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ViaFactory(System.Threading.LazyThreadSafetyMode)
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionAndPublication(System.LazyHelper, Boolean)
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CreateValue()
   at System.Lazy`1[[System.__Canon, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value()
   at System.ConsolePal.get_TerminalFormatStringsInstance()
   at System.ConsolePal.EnsureInitializedCore()
   at System.ConsolePal.EnsureConsoleInitialized()
   at System.ConsolePal.Write(Microsoft.Win32.SafeHandles.SafeFileHandle, System.ReadOnlySpan`1<Byte>, Boolean)
   at System.ConsolePal+UnixConsoleStream.Write(System.ReadOnlySpan`1<Byte>)
   at System.IO.StreamWriter.Flush(Boolean, Boolean)
   at System.IO.StreamWriter.WriteSpan(System.ReadOnlySpan`1<Char>, Boolean)
   at System.IO.StreamWriter.WriteLine(System.String)
   at System.IO.TextWriter+SyncTextWriter.WriteLine(System.String)
   at System.Console.WriteLine(System.String)
   at Hello.Program.Main()
Aborted

IndexOfOrdinalIgnoreCase function assembler is:

G_M26687_IG01:        ; offs=000000H, size=0088H, bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
  800002B7  lui          t0, -524288
  FFF28293  addi         t0, t0, -1
  00B29293  slli         t0, t0, 11
  7FF28293  addi         t0, t0, 2047
  00B29293  slli         t0, t0, 11
  7FE28293  addi         t0, t0, 2046
  00B29293  slli         t0, t0, 11
  7B028293  addi         t0, t0, 1968
  00510133  add          sp, sp, t0
  00813023  sd           fp, 0(sp)
  00113423  sd           ra, 8(sp)
  00010413  addi         fp, sp, 0
  1E840293  addi         t0, fp, 488
  06400313  addi         t1, zero, 100
  0002B423  sd           zero, 8(t0)
  0002B023  sd           zero, 0(t0)
  FFF30313  addi         t1, t1, -1
  01028293  addi         t0, t0, 16
  FE0318E3  bne          t1, zero, -16
  0002B023  sd           zero, 0(t0) 
  000010B7  lui          ra, 1
  84008093  addi         ra, ra, -1984
  008080B3  add          ra, ra, fp
  00A0B023  sd           a0, 0(ra)           ; ra is valid stack address here
  00808093  addi         ra, ra, 8
  008080B3  add          ra, ra, fp         ; ra already has a valid address, addition fp to ra breaks ra.
  00B0B023  sd           a1, 0(ra)           ; Segmentation fault is here. ra = 0x7fffff8748; sp = 0x3fffffbf80
  000010B7  lui          ra, 1
  83008093  addi         ra, ra, -2000
  008080B3  add          ra, ra, fp
  00C0B023  sd           a2, 0(ra)

For output redirection case (| tee or > filename) function IndexOfOrdinalIgnoreCase is not called and problem does not exists.

[clamp03]

Thank you!

[t-mustafin]

I made a JIT tests launch with and without this change, but flaky SEHException is the only change on this tests:

-JIT/Regression/CLR-x86-JIT/V1.2-M01/b08046cs/b08046cs/b08046cs.sh 
+JIT/Performance/CodeQuality/V8/Crypto/Crypto/Crypto.sh
-JIT/opt/OSR/synchronized/synchronized.sh 
-JIT/Performance/CodeQuality/SIMD/RayTracer/RayTracer/RayTracer.sh

[clamp03]

IMO, below lines should be fixed too. Could you check?

runtime/src/coreclr/jit/codegenriscv64.cpp

Lines 7939 to 7940 in 5053c7c

	GetEmitter()->emitIns_R_R_I(INS_addi, EA_PTRSIZE, REG_RA, REG_RA, TARGET_POINTER_SIZE);
	GetEmitter()->emitIns_S_R_R(INS_sd, size, REG_SCRATCH, REG_RA, varNum, -slotSize - 8);

[t-mustafin]

IMO, below lines should be fixed too. Could you check?

@clamp03 thanks for showing this, it has same logic.

This paths work in rare case: more then 1024bytes for local variables && structs of size in range (XLEN, 2*XLEN] passing as arguments. I think it is the reason why test launches show no difference.

[clamp03]

@t-mustafin I found that emitIns_S_R_R do more things than emitIns_R_R_I.

id->idAddr()->iiaLclVar.initLclVarAddr(varx, offs);
id->idSetIsLclVar();

Then it can affect below parts in emitter::emitOutputInstr

runtime/src/coreclr/jit/emitriscv64.cpp

Lines 2636 to 2662 in 16fc92f

	if (emitInsWritesToLclVarStackLoc(id) /*|| emitInsWritesToLclVarStackLocPair(id)*/)
	{
	int varNum = id->idAddr()->iiaLclVar.lvaVarNum();
	unsigned ofs = AlignDown(id->idAddr()->iiaLclVar.lvaOffset(), TARGET_POINTER_SIZE);
	bool FPbased;
	int adr = emitComp->lvaFrameAddress(varNum, &FPbased);
	if (id->idGCref() != GCT_NONE)
	{
	emitGCvarLiveUpd(adr + ofs, varNum, id->idGCref(), dstRW2 - writeableOffset DEBUG_ARG(varNum));
	}
	else
	{
	// If the type of the local is a gc ref type, update the liveness.
	var_types vt;
	if (varNum >= 0)
	{
	// "Regular" (non-spill-temp) local.
	vt = var_types(emitComp->lvaTable[varNum].lvType);
	}
	else
	{
	TempDsc* tmpDsc = codeGen->regSet.tmpFindNum(varNum);
	vt = tmpDsc->tdTempType();
	}
	if (vt == TYP_REF || vt == TYP_BYREF)
	emitGCvarDeadUpd(adr + ofs, dstRW2 - writeableOffset DEBUG_ARG(varNum));
	}

What I suggest are

1. Remove the use of tmp_reg. It means reassigning the baseoffset for the second again
2. Update emitIns_S_R_R cleanly. I am sorry for my messy codes.

@jakobbotsch Could you suggest better solutions for us?

Thank you!

+ @t-mustafin Could you run jit-format for the commits?

[jakobbotsch]

@jakobbotsch Could you suggest better solutions for us?

In general I would suggest replicating how arm64 codegen handles these cases. You can see that it forms the temporary register with the offset inside emitIns_S_R.