Re: [PATCH v2] sim: riscv: Fix some compatibility issues with gcc

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 4/12/24 16:48, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> This makes the riscv simulator able to execute a simple
>> "hello world" program when gcc is configured
>> with:
>>
>> .../gcc-trunk/configure --target=riscv-unknown-elf
>>
>> The first problem is that gcc generates rv32
>> code by default in this configuration, while
>> riscv64-unknown-elf generates rv64 code by default.
>>
>> So change the riscv/acinclude.m4 to use the same
>> logic here.
> 
> I'm sorry, but this change is not OK.  At least, I'll need more
> convincing before this could be merged.
> 
> Previously, the riscv sim would support ( in theory ) both RV32 and
> RV64, and would pick the correct word size based on the ELF
> architecture.
> 
> What you're finding is that the riscv sim is (apparently) pretty buggy.
> 
> With just the sim-main.c fix and the model_list.def change I can see the
> simulator trying to run the RV32 code.  But, the problem I see is
> confusion about whether registers should be sign extended or not.
> 
> One example of where this goes wrong is this.  Consider the instruction:
> 
>   c.li a1,-1
> 
> this will result in the value 0xffffffffffffffff being written into the
> register, even for RV32 code.
> 
> But as this is 32-bit code, I also see the code build the constant
> 0xffffffff by shifting and or-ing things together.  The code then
> compares the register containing 0xffffffffffffffff with the register
> containing 0xffffffff together, and expects them to be the same.
> 
> Obviously they are not the same, so the code takes an incorrect branch,
> and everything goes wrong...
> 
> If we force the build to be RV32 only then the register size will be
> 32-bits and the above problem is "solved".  But it's not really, we're
> just masking the issue.
> 
> So I think we need to solve the register size issue first.
> 

This patch series fixes only the case when no --architecture riscv:rv32
is given to riscv64-unknown-elf-run, also the argc argv parameters of main
use hard-coded values of type address_word which is uint64_t for riscv64
and uint32_t for other spec.  I could probably add an error if this non-working
architecture is requested.

Given the number of issues I ran into in the few days I started working
on this architecture I cant imagine that I can solve every issue, for
instance the missing f and d floating point instructions, would be far
more important than making --architecture riscv:rv32 work, since it can be
achieved by using the --target buildspec when building the simulator.

>>
>> And the second issue is that gcc does by default
>> generate instructions in INSN_CLASS_C, so move
>> the M(GC) to top of list, in riscv/model_list.def.
> 
> I haven't looked at this part of the change yet.  But just going by my
> gut, this doesn't feel right.  I would have expected there to be some
> logic "somewhere" that checks down the model list and looks for a mode
> with the required extensions.
> 
> Just moving the one we want/need to the top feels like we're just
> changing the default.  Is the "pick the correct model" code broken?
> Completely missing?
> 

It changes only the default when no --model RV32IMAC or sonmething
similar is given on the command line,  previously RV32G was the first
element in the list, and now RV32GC is the default, which allows
to use the C instruction set, which is really used all the time.

The default should at least allow all implemented instructions.


>>
>> Then there was apparently a confusion which cpu
>> model uses JAL and which ADDIW.  Fixed that in
>> execute_c, case MATCH_C_JAL | MATCH_C_ADDIW.
> 
> You might consider breaking this change out, as this third part could go
> in on its own I think.
> 

OK. Sure.
I can also update the comment.


Thanks
Bernd.

> Thanks,
> Andrew
> 
> 
>>
>> With these changes a simple c-prgram can be executed,
>> however there is still work to do, since when the
>> program does floating point operations, gcc starts to
>> generate hardware floating point instructions, with no
>> obvious opt-out option.
>>
>> Note the gcc test suite can be used to test the
>> simulator in this way:
>>
>> make check-gcc RUNTESTFLAGS="--target_board=multi-sim SIM=riscv-unknown-elf-run"
>>
>> Now many tests are passed, except those which use
>> floating point instructions.
>>
>> To work around the not supported float instructions the
>> following gcc configuration can be used, which makes
>> most of the gcc test cases successfully executed:
>>
>> .../gcc-trunk/configure --prefix=... --target=riscv-unknown-elf
>>  --disable-multilib --with-arch=rv32imac --with-abi=ilp32
>>
>> Note: binutils are installed at prefix path and newlib/libgloss in-tree.
>>
>> Fixes 3224e32fb84f ("sim: riscv: Add support for compressed integer instructions")
>> ---
>>  sim/configure            | 6 +++---
>>  sim/riscv/acinclude.m4   | 4 ++--
>>  sim/riscv/model_list.def | 2 +-
>>  sim/riscv/sim-main.c     | 4 ++--
>>  4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> v2: updated the commit message, with some hints
>> how to compile a compatible gcc toolchain.
>>
>> diff --git a/sim/configure b/sim/configure
>> index 1ebef377973..fdc0a86d524 100755
>> --- a/sim/configure
>> +++ b/sim/configure
>> @@ -17479,10 +17479,10 @@ $as_echo "$sim_ppc_xor_endian" >&6; }
>>  
>>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking riscv bitsize" >&5
>>  $as_echo_n "checking riscv bitsize... " >&6; }
>> -SIM_RISCV_BITSIZE=64
>> +SIM_RISCV_BITSIZE=32
>>  case $target in #(
>> -  riscv32*) :
>> -    SIM_RISCV_BITSIZE=32 ;; #(
>> +  riscv64*) :
>> +    SIM_RISCV_BITSIZE=64 ;; #(
>>    *) :
>>       ;;
>>  esac
>> diff --git a/sim/riscv/acinclude.m4 b/sim/riscv/acinclude.m4
>> index 0c6290c9c08..e9953c1017d 100644
>> --- a/sim/riscv/acinclude.m4
>> +++ b/sim/riscv/acinclude.m4
>> @@ -15,8 +15,8 @@ dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>  dnl
>>  dnl NB: This file is included in sim/configure, so keep settings namespaced.
>>  AC_MSG_CHECKING([riscv bitsize])
>> -SIM_RISCV_BITSIZE=64
>> +SIM_RISCV_BITSIZE=32
>>  AS_CASE([$target],
>> -	[riscv32*], [SIM_RISCV_BITSIZE=32])
>> +	[riscv64*], [SIM_RISCV_BITSIZE=64])
>>  AC_MSG_RESULT([$SIM_RISCV_BITSIZE])
>>  AC_SUBST(SIM_RISCV_BITSIZE)
>> diff --git a/sim/riscv/model_list.def b/sim/riscv/model_list.def
>> index b83557e5539..df9ec897126 100644
>> --- a/sim/riscv/model_list.def
>> +++ b/sim/riscv/model_list.def
>> @@ -1,9 +1,9 @@
>> +M(GC)
>>  M(G)
>>  M(I)
>>  M(IM)
>>  M(IMA)
>>  M(IA)
>> -M(GC)
>>  M(IC)
>>  M(IMC)
>>  M(IMAC)
>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c
>> index adff99921c6..9c0d070aa60 100644
>> --- a/sim/riscv/sim-main.c
>> +++ b/sim/riscv/sim-main.c
>> @@ -1018,7 +1018,7 @@ execute_c (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>      case MATCH_C_JAL | MATCH_C_ADDIW:
>>        /* JAL and ADDIW have the same mask but are only available on RV64 or
>>  	 RV32 respectively.  */
>> -      if (RISCV_XLEN (cpu) == 64)
>> +      if (RISCV_XLEN (cpu) == 32)
>>  	{
>>  	  imm = EXTRACT_CJTYPE_IMM (iw);
>>  	  TRACE_INSN (cpu, "c.jal %" PRIxTW,
>> @@ -1027,7 +1027,7 @@ execute_c (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>  	  pc = riscv_cpu->pc + imm;
>>  	  TRACE_BRANCH (cpu, "to %#" PRIxTW, pc);
>>  	}
>> -      else if (RISCV_XLEN (cpu) == 32)
>> +      else if (RISCV_XLEN (cpu) == 64)
>>  	{
>>  	  imm = EXTRACT_CITYPE_IMM (iw);
>>  	  TRACE_INSN (cpu, "c.addiw %s, %s, %#" PRIxTW ";  // %s += %#" PRIxTW,
>> -- 
>> 2.39.2
>