Re: [PATCH 2/2] RISC-V: Avoid emitting invalid instructions in mixed RVC/no-RVC code

[Palmer Dabbelt <palmer at dabbelt dot com>]

Committed.

On Wed, 06 Sep 2017 13:43:48 PDT (-0700), Palmer Dabbelt wrote:
> When linking the following code
>
>     .global _prog_start
>     _prog_start:
>             mv x1, x1
>             mv x2, x2
>     .align 2
>     rvc_boundry:
>     .option norvc
>     .align 3
>             mv x3, x3
>
> we currently emit an invalid two-byte 0 instruction.  The actual output
> code looks like
>
>     0000000080000000 <_prog_start>:
>         80000000:   8086                    mv      ra,ra
>         80000002:   810a                    mv      sp,sp
>
>     0000000080000004 <rvc_boundry>:
>         80000004:   0000                    unimp
>         80000006:   0001                    nop
>         80000008:   00018193                mv      gp,gp
>
> This ends up manifesting due to the two-byte compressed NOP that's
> pessimisticly emitted by the ".align 2", which results in "rvc_boundry"
> being 2-byte aligned.  frag_align_code() then goes and outputs a 2-byte
> NOP (which is invalid in no-RVC mode) to align the code back to a 4-byte
> boundry, which can't be relaxed away by the linker as it's not part of
> the R_RISCV_RELAX relocation.
>
> The fix is to just always emit the worst case possible alignment into
> the output as a single R_RISCV_RELAX, which the linker will then fix up.
>
> With this patch I get the expected code generation
>
>     0000000080000000 <_prog_start>:
>         80000000:   8086                    mv      ra,ra
>         80000002:   810a                    mv      sp,sp
>
>     0000000080000004 <rvc_boundry>:
>         80000004:   00000013                nop
>         80000008:   00018193                mv      gp,gp
>
> gas/ChangeLog
>
> 2017-09-06  Palmer Dabbelt  <palmer@dabbelt.com>
>
>         * config/tc-riscv.c (riscv_frag_align_code): Emit the entire
>         alignment sequence inside R_RISCV_ALIGN.
> ---
>  gas/config/tc-riscv.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
> index 4c644ab65edf..1a15efc47985 100644
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -2274,30 +2274,21 @@ bfd_boolean
>  riscv_frag_align_code (int n)
>  {
>    bfd_vma bytes = (bfd_vma) 1 << n;
> -  bfd_vma min_text_alignment_order = riscv_opts.rvc ? 1 : 2;
> -  bfd_vma min_text_alignment = (bfd_vma) 1 << min_text_alignment_order;
> -
> -  /* First, get back to minimal alignment.  */
> -  frag_align_code (min_text_alignment_order, 0);
> +  bfd_vma worst_case_bytes = bytes - 2;
> +  char *nops = frag_more (worst_case_bytes);
> +  expressionS ex;
>
>    /* When not relaxing, riscv_handle_align handles code alignment.  */
>    if (!riscv_opts.relax)
>      return FALSE;
>
> -  if (bytes > min_text_alignment)
> -    {
> -      bfd_vma worst_case_bytes = bytes - min_text_alignment;
> -      char *nops = frag_more (worst_case_bytes);
> -      expressionS ex;
> +  ex.X_op = O_constant;
> +  ex.X_add_number = worst_case_bytes;
>
> -      ex.X_op = O_constant;
> -      ex.X_add_number = worst_case_bytes;
> +  riscv_make_nops (nops, worst_case_bytes);
>
> -      riscv_make_nops (nops, worst_case_bytes);
> -
> -      fix_new_exp (frag_now, nops - frag_now->fr_literal, 0,
> -		   &ex, FALSE, BFD_RELOC_RISCV_ALIGN);
> -    }
> +  fix_new_exp (frag_now, nops - frag_now->fr_literal, 0,
> +	       &ex, FALSE, BFD_RELOC_RISCV_ALIGN);
>
>    return TRUE;
>  }