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

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

Dear,

On Wed, Dec 11, 2019 at 2:24 PM Kuan-Lin Chen <kuanlinchentw@gmail.com> wrote:
> > What's this trying to do?  It seem like it would cause leakage (or, I guess, is
> > more trying to work around some leakage).
> It's trying to merge size of two alignment fragment. (frag_align_code
> and frag_more in riscv_frag_align_code).

I think this is a skillful implementation!  Please let me know if I am
missing anything or wrong :)

==================================
* There is a simple assembly code as follows:

       .option norvc
       .option relax
foo:
        xori x0, x0, 1
        xori x0, x0, 2
        .byte 0xAA
        .byte 0xAB
        .align 3
        xori x0, x0, 3
        xori x0, x0, 4

==================================
* And the generated object file is as follows:

Disassembly of section .text:

00000000 <foo>:
   0:   00104013                xori    zero,zero,1
   4:   00204013                xori    zero,zero,2
   8:   abaa                    fsd     fa0,464(sp)
   a:   0001                    nop
   c:   0000                    unimp
   e:   0000                    unimp
  10:   00304013                xori    zero,zero,3
  14:   00404013                xori    zero,zero,4

Relocation section '.rela.text' at offset 0xe8 contains 1 entry:
 Offset     Info    Type                Sym. Value  Symbol's Name + Addend
0000000a  0000002b R_RISCV_ALIGN                     6
==================================

For the `.align 3` directive, we find that the begin address is not
aligned to the current instruction alignment (norvc, so the alignment
is 4 bytes), so we call `frag_align_code` to make sure the alignment,
and then we can use the `frag_more` to allocate the worst_case_bytes
as like before.  The `frag_align_code` will call `frag_var`, so we
will get at least two alignment fragments for the `.align` directive.
The first one is `rs_align_code` which changed from `rs_fill` by
`frag_align_code`, the second one is `rs_dummy` created by the
`frag_align_code`.  And we insert the fixup (relocation) to the first
alignment fragment with the worst_case_bytes addend.  The addend
should be (worst_case_bytes + 2) if we have allocated the bytes by
`frag_align_code`, and it will be fixed in the `md_apply_fix`.

The original `riscv_handle_align` just handle the non-relax cases, and
I think it is reasonable.  Fill the NOPS for the relax cases seems to
meaningless, since we will fill them when linker doing the alignment
relaxation.  I suspect that Kuan-Lin handle both relax and non-relax
cases in the `riscv_handle_align` is because of one reason.  He fills
the NOPs for the first fragment (frag_align_code), and then modify
it's `fragP->fr_var`.  Once the `fragP->fr_var` is modified, we will
later fix the correct addend for the R_RISCV_ALIGN in the
`md_apply_fix`.  Therefore, he can use just one fixup (relocation) of
the first fragment, and the fixup's size actually include the second
fragment (`rs_dummy`).

I used to worry about the fixup cross the fragment may cause some
unexpected problems in the previous reply.  But after Kuan-Lin's
explanation, I am OK for this now, and the solution looks great to me
:)

One minor thing is that the object file still may have non-filling
NOPS in the above example (enable relax).  But I think it is fine
since linker will help to fill them when relaxing.

Thanks and best regards
Nelson