[PATCH] RISC-V: Update ABI to the elf_flags after parsing elf attributes.

[Nelson Chu]

On Thu, Oct 22, 2020 at 7:24 AM Jim Wilson <jimw@sifive.com> wrote:
>
> On Mon, Oct 19, 2020 at 8:39 PM Nelson Chu <nelson.chu@sifive.com> wrote:
> > Originally, if the -mabi option isn't set, then assembler will set the
> > abi according to the architecture string in the riscv_after_parse_args.
> > But we should also check and reset the abi later since the architecture
> > string may be reset by the elf attributes.  Therefore, set the abi to
> > the elf_flags in the riscv_after_parse_args seems too early, we can set
> > them in the riscv_elf_final_processing to make sure that the architecture
> > string won't be changed.
>
> It is OK to delay setting the ELF header flags but you are also delaying sets to abi_xlen, float_abi, and rve_abi.  float_abi and rve_abi are currently only used for setting ELF header flags so that is OK.  But abi_xlen is used in some macro instructions to determine whether we emit a lw or ld instruction.

Oh, you are right, I forgot that the abi_xlen will be used by the
pseudo instructions...

> So if I modify one of your testcases to get this
>
>  .attribute arch,"rv64i"
> .option pic
> .extern foo
> la a0, foo
>
> and assemble with -march=rv32ifd and disassemble I get
>
>    0: 00000517           auipc a0,0x0
> 0: R_RISCV_GOT_HI20 foo
>    4: 00052503           lw a0,0(a0) # 0 <.text>
> 4: R_RISCV_PCREL_LO12_I .L0
> 4: R_RISCV_RELAX *ABS*
>
> which is wrong because 64-bit code should use ld.  This fails both with and without your patch.  It fails without your patch because abi_xlen is wrongly set to 32.  And it fails with your patch because abi_xlen is now 0 when we check it.

Thank you very much for pointing this out, Jim.  we need to fix this.

> We already have code to give an error if an attribute arch follows an instruction.  This uses the start_assemble variable.  So it would be safe to call riscv_set_abi_by_arch at the place where we set start_assemble to TRUE.  If I try that, my modified testcase works.  I had to add a declaration.  Or you could move functions around.  Anyways, I think this all looks OK with this one change.

Thanks, I have sent the v2 patch to fix the problem according to your
suggestion!  Calling riscv_set_abi_by_arch at the place where we set
start_assemble to TRUE, and then move the functions around is a good
solution.  Umm but we may still need to call the riscv_set_abi_by_arch
at the final_processing, since a minor case that the file without any
instruction will not call the md_assemble, but it may have some elf
attributes setting, although I think the setting seems meaningless.

Besides, I also find other problems for the mis-matched ISA setting.
We can discuss the details in the v2 patches.

Thanks
Nelson