[PATCH 1/3] RISC-V: process rs_align_code also when relaxing
Nelson Chu
nelson@rivosinc.com
Tue Aug 20 02:44:03 GMT 2024
Thanks, this looks the right way to go, since the section padding may not
be padded before when ".option relax/norelax" are both set.
(Not related to this patch) There is another alignment issue here, but I
forgot if somewhere had been recorded - Is there a way for frag_align_code
to also get the second parameter of .align, which is the fill value,
works? Or in other words, if the padding value of .align is given, then we
will no longer add R_RISCV_ALIGN to the file. Is this behavior correct?
Thanks
Nelson
On Mon, Aug 12, 2024 at 11:07 PM Jan Beulich <jbeulich@suse.com> wrote:
> riscv_handle_align() runs after all input was processed. Whether
> relaxation is enabled for any particular piece of code is not recorded
> anywhere. (This issue was even "worked around" in a gas testcase, which
> is adjusted accordingly.) Furthermore, as demonstrated by an ld
> testcase, tail padding in an object file's executable sections depended
> on whether relaxation was enabled at the end of assembly: NOPs were
> emitted only when relaxation was off; zeroes were emitted with
> relaxation enabled. (It could probably be either way, but it should be
> independent of relaxation state at the end of assembly. Except of course
> write.c, in a comment ahead of #define-ing SUB_SEGMENT_ALIGN(),
> explicitly says "proper nop-filling".)
>
> While re-indenting, drop the "odd_padding" variable. It's used exactly
> once, and having the actual expression right in the if() is imo helping
> readers to understand what the intentions are.
>
> While touching the ld testcase, also tighten the expectations for the
> addresses of the two symbols: The last two digits have to have fixed
> values.
> ---
> I'm not quite convinced of adding the 27 lines of NOPs to the ld
> testcase expectations, as there looks to be another anomaly: The
> assembler inserts 27 NOPs simply because in the object file "func" isn't
> aligned to a 128-byte boundary. Correct alignment is only effected by
> the linker, processing the ALIGN relocation. Therefore the tail
> padding done by gas doesn't really have the (presumably) intended effect
> in the final binary. Correct behavior would imo be for the assembler to
> insert another ALIGN relocation in addition, as if there was an
> implicit ".p2align <section-alignment>" at the end of each text section.
> Then in the final executable there would be 31 NOPs, not 27. Adding
> "#..." at the end of the expectations, otoh, also doesn't feel quite
> right.
>
> I wonder anyway why section tail padding is done for RISC-V. All other
> architectures I'm at least vaguely familiar with have "#define
> SUB_SEGMENT_ALIGN(...) 0". After all the other related arch hook,
> md_section_align(), already doesn't alter "size" in any way for RISC-V.
>
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -4993,40 +4993,37 @@ riscv_handle_align (fragS *fragP)
> switch (fragP->fr_type)
> {
> case rs_align_code:
> - /* When relaxing, riscv_frag_align_code handles code alignment. */
> - if (!riscv_opts.relax)
> - {
> - bfd_signed_vma bytes = (fragP->fr_next->fr_address
> - - fragP->fr_address - fragP->fr_fix);
> - /* We have 4 byte uncompressed nops. */
> - bfd_signed_vma size = 4;
> - bfd_signed_vma excess = bytes % size;
> - bfd_boolean odd_padding = (excess % 2 == 1);
> - char *p = fragP->fr_literal + fragP->fr_fix;
> + {
> + bfd_signed_vma bytes = (fragP->fr_next->fr_address
> + - fragP->fr_address - fragP->fr_fix);
> + /* We have 4 byte uncompressed nops. */
> + bfd_signed_vma size = 4;
> + bfd_signed_vma excess = bytes % size;
> + char *p = fragP->fr_literal + fragP->fr_fix;
>
> - if (bytes <= 0)
> - break;
> + if (bytes <= 0)
> + break;
>
> - /* Insert zeros or compressed nops to get 4 byte alignment. */
> - if (excess)
> - {
> - if (odd_padding)
> - riscv_add_odd_padding_symbol (fragP);
> - riscv_make_nops (p, excess);
> - fragP->fr_fix += excess;
> - p += excess;
> - }
> + /* Insert zeros or compressed nops to get 4 byte alignment. */
> + if (excess)
> + {
> + if (excess % 2)
> + riscv_add_odd_padding_symbol (fragP);
> + riscv_make_nops (p, excess);
> + fragP->fr_fix += excess;
> + p += excess;
> + }
>
> - /* The frag will be changed to `rs_fill` later. The function
> - `write_contents` will try to fill the remaining spaces
> - according to the patterns we give. In this case, we give
> - a 4 byte uncompressed nop as the pattern, and set the size
> - of the pattern into `fr_var`. The nop will be output to the
> - file `fr_offset` times. However, `fr_offset` could be zero
> - if we don't need to pad the boundary finally. */
> - riscv_make_nops (p, size);
> - fragP->fr_var = size;
> - }
> + /* The frag will be changed to `rs_fill` later. The function
> + `write_contents` will try to fill the remaining spaces
> + according to the patterns we give. In this case, we give
> + a 4 byte uncompressed nop as the pattern, and set the size
> + of the pattern into `fr_var`. The nop will be output to the
> + file `fr_offset` times. However, `fr_offset` could be zero
> + if we don't need to pad the boundary finally. */
> + riscv_make_nops (p, size);
> + fragP->fr_var = size;
> + }
> break;
>
> default:
> --- a/gas/testsuite/gas/riscv/mapping.s
> +++ b/gas/testsuite/gas/riscv/mapping.s
> @@ -1,7 +1,4 @@
> .attribute arch, "rv32ic"
> -.option norelax # FIXME: assembler fill the
> paddings after parsing everything,
> - # so we probably won't fill anything for
> the norelax region when
> - # the riscv_opts.relax is enabled at
> somewhere.
>
> .section .text.cross.section.A, "ax"
> .option push
> --- a/ld/testsuite/ld-riscv-elf/relax-max-align-gp.d
> +++ b/ld/testsuite/ld-riscv-elf/relax-max-align-gp.d
> @@ -7,7 +7,7 @@
>
> Disassembly of section .text:
>
> -0+[0-9a-f]+ <_start>:
> +0+[0-9a-f]+00 <_start>:
> .*:[ ]+[0-9a-f]+[ ]+addi[ ]+.*<gdata>
> .*:[ ]+[0-9a-f]+[ ]+jal[ ]+.*
> .*:[ ]+[0-9a-f]+[ ]+j[ ]+.*
> @@ -41,6 +41,32 @@ Disassembly of section .text:
> .*:[ ]+[0-9a-f]+[ ]+nop
> .*:[ ]+[0-9a-f]+[ ]+nop
>
> -0+[0-9a-f]+ <func>:
> +0+[0-9a-f]+80 <func>:
> .*:[ ]+[0-9a-f]+[ ]+ret
> -[ ]+...
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
> +.*:[ ]+[0-9a-f]+[ ]+nop
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/binutils/attachments/20240820/38fd1ee0/attachment.htm>
More information about the Binutils
mailing list