Re: [PATCH] RISC-V: Fix .p2align is not at smallest instruction

[Nelson Chu <nelson dot chu at sifive dot com>]

Dear Kuan-Lin,

Thanks and appreciate for your email :)

> As far as I know, RISC-V compilers (LLVM & GCC) doesn't mix code and data
> in the same section.  But we cannot avoid users mixing them in manual assembly
> code.

Yes we don't mix the code and data in the same section currently.  Jim
had mentioned this in the open discussion page
(https://github.com/riscv/riscv-elf-psabi-doc/pull/116).  ARM has
mapping symbols, and as I know, their assembler set the alignment when
the state is transformed from MAP_DATA to MAP_INSN.  The ideally
solution may be similar to Arm's solution.

* For linker

The modification in the bfd/elfnn-riscv.c is good to me.  We still
need to write NOPs even if the number of NOPs is already correct,
since assembler may not write the complete NOP instructions for some
mixed case (mix data and text).

* For the assembler

(riscv_frag_align_code in the gas/config/tc-riscv.c)
> +  /* If we are moving to a smaller alignment than the instruction size,
> +     riscv_handle_align handles code alignment.  */
> +  if (bytes <= insn_alignment)
> +    return FALSE;

I agree with this, we should return FALSE for the smaller alignment if
the section contains both data and text, and let the do_align to fix
the specific alignment.

However, I see you call the frag_align_code in the
riscv_frag_align_code to force the alignment.  Since the
frag_align_code (it will call frag_var) may create a new fragment, I
worry about if the fixup cross the fragment may cause some unexpected
problems?  I am not sure about this, so maybe someone can confirm it
:)  Also, I probably understand why we need to do the modifications in
the md_apply_fix and riscv_handle_align.  It would be great if the
more comments can describe them in detail :)

I  am wondering if we can get the same expected result and don't need
to let the fixup cross the fragment.  Therefore, I try to do this in
the attached patch.  I just get pass for the rv32i and rv64gc binutils
testsuites, including the new alignment tests.

Besides, I get a similar result for the following case, but I think
set the ".p2align 1" in the NORVC section is strange, we can only
confirm the second nop to 2-byte alignment, but the norvc section
should always be 4 byte aligned.  The ideally solution should be
similar as ARM, that is, we support the mapping symbol and set the
alignment when the state is transformed from MAP_DATA to MAP_INSN.

[Assembly file]
.text
.globl main
.option norvc
.option relax
main:
        nop
        .byte 0x99
.p2align 1
        nop

[Object file]
align-2.o:     file format elf64-littleriscv

Disassembly of section .text:

0000000000000000 <main>:
   0:   00000013                nop
   4:   0099                        addi    ra,ra,6
   6:   00000013                nop
...


Any suggestion and idea is appreciate :)

Thanks again and Regards
Nelson

Attachment: 0001-RISC-V-Fix-.p2align-is-not-at-smallest-instruction.patch
Description: Binary data