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

[Kuan-Lin Chen <kuanlinchentw at gmail dot com>]

Hi Nelson,

About the fixup cross fragments, I think it's ok.
Because the addend just records the possible range for alignment.
We just have to be sure the next fragment has the enough space for alignment.
But if you have the solution without crossing the fragment, it's fine to me.
Thanks for your review.

Nelson Chu <nelson.chu@sifive.com> 於 2019年12月4日 週三 下午6:44寫道：
>
> 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



-- 
Best regards,
Kuan-Lin Chen.
kuanlinchentw@gmail.com