[PATCH 2/3] RISC-V: correct alignment directive handling for text sections
Palmer Dabbelt
palmer@dabbelt.com
Tue Sep 3 23:02:59 GMT 2024
On Mon, 12 Aug 2024 08:08:04 PDT (-0700), jbeulich@suse.com wrote:
> .insn or data emitted inside text sections can lead to positions not
> being at insn granularity. In such situations using alignment
> directives should reliably enforce the requested alignment.
> Specifically requests to align back to insn granularity may not be
> ignored (where, as a subcase thereof, the ordering of ".option norvc"
> and e.g. ".p2align 2" should not matter; so far the alignment directive
> needs to come first to have any effect). Similarly ahead of emitting
> NOPs alignment first needs to be forced back to insn granularity.
>
> The new testcases actually point out a corner case issue in the
> disassembler as well, which is being corrected at the same time: We
> don't want to print "0x" without any subsequent digits.
Sorry for being slow here. Nelson and I talked about this a few times.
IIRC a bunch of the complexity in the code was related to trying to
avoid mixing RVC instructions into non-RVC regions in order to re-gain
alignment. I remember there being some reason that we didn't want to
emit instructions when changing rvc->norvc, but I can't remember what
that reason was. So maybe I'm just crazy here...
Jim might remember? IIRC he was at least sitting there talking about
this stuff as it was getting fixed, I forget if he wrote the code.
That said, I think you just found another bug: certainly ignoring the
alignment directive is going to break things, so that's bad. I don't
see anything wrong with the actual code here, though...
So Nelson is going to run some regressions and see what's up. If they
all pass and nobody can remember a reason this wouldn't work, then I
think we just go with it.
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -183,6 +183,9 @@ static enum float_abi float_abi = FLOAT_
>
> static unsigned elf_flags = 0;
>
> +/* Indicate we are already assemble any instructions or not. */
> +static bool start_assemble = false;
> +
> static bool probing_insn_operands;
>
> /* Set the default_isa_spec. Return 0 if the spec isn't supported.
> @@ -280,6 +283,16 @@ riscv_set_rvc (bool rvc_value)
> if (rvc_value)
> elf_flags |= EF_RISCV_RVC;
>
> + if (start_assemble && subseg_text_p (now_seg)
> + && riscv_opts.rvc && !rvc_value)
> + {
> + struct riscv_segment_info_type *info
> + = &seg_info(now_seg)->tc_segment_info_data;
> +
> + info->last_insn16 = true;
> + info->rvc = rvc_value;
> + }
> +
> riscv_opts.rvc = rvc_value;
> }
>
> @@ -349,10 +362,8 @@ riscv_set_arch (const char *s)
> riscv_parse_subset (&riscv_rps_as, s);
> riscv_reset_subsets_list_arch_str ();
>
> - riscv_set_rvc (false);
> - if (riscv_subset_supports (&riscv_rps_as, "c")
> - || riscv_subset_supports (&riscv_rps_as, "zca"))
> - riscv_set_rvc (true);
> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "c")
> + || riscv_subset_supports (&riscv_rps_as, "zca"));
>
> if (riscv_subset_supports (&riscv_rps_as, "ztso"))
> riscv_set_tso ();
> @@ -452,9 +463,6 @@ const char EXP_CHARS[] = "eE";
> As in 0f12.456 or 0d1.2345e12. */
> const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>
> -/* Indicate we are already assemble any instructions or not. */
> -static bool start_assemble = false;
> -
> /* Indicate ELF attributes are explicitly set. */
> static bool explicit_attr = false;
>
> @@ -622,6 +630,7 @@ riscv_mapping_state (enum riscv_seg_msta
>
> valueT value = (valueT) (frag_now_fix () - max_chars);
> seg_info (now_seg)->tc_segment_info_data.map_state = to_state;
> + seg_info (now_seg)->tc_segment_info_data.last_insn16 = false;
> const char *arch_str = reset_seg_arch_str
> ? riscv_rps_as.subset_list->arch_str : NULL;
> make_mapping_symbol (to_state, value, frag_now, arch_str,
> @@ -4148,12 +4157,13 @@ riscv_ip_hardcode (char *str,
> generic_bignum[num],
> llen);
> memset(ip->insn_long_opcode + repr_bytes, 0, bytes - repr_bytes);
> - return NULL;
> }
> -
> - if (bytes < sizeof(values[0]) && values[num - 1] >> (8 * bytes) != 0)
> + else if (bytes < sizeof(values[0]) && values[num - 1] >> (8 * bytes) != 0)
> return _("value conflicts with instruction length");
>
> + if (!riscv_opts.rvc && (bytes & 2))
> + seg_info (now_seg)->tc_segment_info_data.last_insn16 = true;
> +
> return NULL;
> }
>
> @@ -4840,10 +4850,8 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
> riscv_update_subset (&riscv_rps_as, name);
> riscv_reset_subsets_list_arch_str ();
>
> - riscv_set_rvc (false);
> - if (riscv_subset_supports (&riscv_rps_as, "c")
> - || riscv_subset_supports (&riscv_rps_as, "zca"))
> - riscv_set_rvc (true);
> + riscv_set_rvc (riscv_subset_supports (&riscv_rps_as, "c")
> + || riscv_subset_supports (&riscv_rps_as, "zca"));
>
> if (riscv_subset_supports (&riscv_rps_as, "ztso"))
> riscv_set_tso ();
> @@ -4951,15 +4959,27 @@ riscv_frag_align_code (int n)
> char *nops;
> expressionS ex;
>
> - /* If we are moving to a smaller alignment than the instruction size, then no
> - alignment is required. */
> + /* If we are moving to alignment no larger than the instruction size, then
> + no special alignment handling is required. */
> if (bytes <= insn_alignment)
> - return true;
> + {
> + if (bytes == insn_alignment)
> + seg_info (now_seg)->tc_segment_info_data.last_insn16 = false;
> + return false;
> + }
>
> /* When not relaxing, riscv_handle_align handles code alignment. */
> if (!riscv_opts.relax)
> return false;
>
> + /* If the last item emitted was not an ordinary insn, first align back to
> + insn granularity. Don't do this unconditionally, to avoid altering frags
> + when that's not actually needed. */
> + if (seg_info (now_seg)->tc_segment_info_data.map_state != MAP_INSN
> + || seg_info (now_seg)->tc_segment_info_data.last_insn16)
> + frag_align_code (riscv_opts.rvc ? 1 : 2, 0);
> + seg_info (now_seg)->tc_segment_info_data.last_insn16 = false;
> +
> /* Maybe we should use frag_var to create a new rs_align_code fragment,
> rather than just use frag_more to handle an alignment here? So that we
> don't need to call riscv_mapping_state again later, and then only need
> @@ -5277,6 +5297,18 @@ tc_riscv_regname_to_dw2regnum (char *reg
> }
>
> void
> +riscv_elf_section_change_hook (void)
> +{
> + struct riscv_segment_info_type *info
> + = &seg_info(now_seg)->tc_segment_info_data;
> +
> + if (info->rvc && !riscv_opts.rvc)
> + info->last_insn16 = true;
> +
> + info->rvc = riscv_opts.rvc;
> +}
> +
> +void
> riscv_elf_final_processing (void)
> {
> riscv_set_abi_by_arch ();
> --- a/gas/config/tc-riscv.h
> +++ b/gas/config/tc-riscv.h
> @@ -127,6 +127,9 @@ extern int tc_riscv_regname_to_dw2regnum
> /* Even on RV64, use 4-byte alignment, as F registers may be only 32 bits. */
> #define DWARF2_CIE_DATA_ALIGNMENT -4
>
> +#define md_elf_section_change_hook riscv_elf_section_change_hook
> +extern void riscv_elf_section_change_hook (void);
> +
> #define elf_tc_final_processing riscv_elf_final_processing
> extern void riscv_elf_final_processing (void);
>
> @@ -152,6 +155,8 @@ void riscv_mapping_state (enum riscv_seg
> struct riscv_segment_info_type
> {
> enum riscv_seg_mstate map_state;
> + bool rvc;
> + bool last_insn16;
> /* The current mapping symbol with architecture string. */
> symbolS *arch_map_symbol;
> };
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/relax-align-2.d
> @@ -0,0 +1,52 @@
> +#as: -mrelax
> +#objdump: -drw
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <rvc_func>:
> +[ ]+0:[ ]+8082[ ]+ret
> +[ ]+2:[ ]+0001[ ]+nop
> +[ ]+4:[ ]+00000013[ ]+nop[ ]+4: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +
> +0+008 <non_rvc_func>:
> +[ ]+8:[ ]+00008067[ ]+ret
> +
> +0+00c <insn>:
> +[ ]+c:[ ]+00000013[ ]+nop
> +[ ]+10:[ ]+0000[ ]+\.insn 2, 0x0+
> +[ ]+12:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+14:[ ]+00000013[ ]+nop[ ]+14: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +[ ]+18:[ ]+00008067[ ]+ret
> +
> +0+001c <hword>:
> +[ ]+1c:[ ]+00000013[ ]+nop
> +[ ]+20:[ ]+0000[ ]+\.short 0x0+
> +[ ]+22:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+24:[ ]+00000013[ ]+nop[ ]+24: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +[ ]+28:[ ]+00008067[ ]+ret
> +
> +0+002c <byte>:
> +[ ]+2c:[ ]+00000013[ ]+nop
> +[ ]+30:[ ]+00[ ]+\.byte 0x0+
> +[ ]+31:[ ]+00[ ]+\.byte 0x0+
> +[ ]+32:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+34:[ ]+00000013[ ]+nop[ ]+34: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +[ ]+38:[ ]+00008067[ ]+ret
> +[ ]+3c:[ ]+00000013[ ]+nop[ ]+3c: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +
> +0+0040 <func1>:
> +[ ]+40:[ ]+00000013[ ]+nop
> +[ ]+44:[ ]+00008067[ ]+ret
> +
> +0+0048 <func2>:
> +[ ]+48:[ ]+8082[ ]+ret
> +[ ]+4a:[ ]+0001[ ]+nop
> +[ ]+4c:[ ]+00000013[ ]+nop[ ]+4c: R_RISCV_ALIGN[ ]+\*ABS\*\+0x4
> +
> +0+0050 <func3>:
> +[ ]+50:[ ]+00000013[ ]+nop
> +[ ]+54:[ ]+00008067[ ]+ret
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/relax-align-2.s
> @@ -0,0 +1,50 @@
> + .text
> + .option rvc
> +rvc_func:
> + ret
> +
> + .option norvc
> + .p2align 3
> +non_rvc_func:
> + ret
> +
> +insn:
> + nop
> + .insn 0
> + .p2align 3
> + ret
> +
> +hword:
> + nop
> + .hword 0
> + .p2align 3
> + ret
> +
> +byte:
> + nop
> + .byte 0
> + .p2align 3
> + ret
> +
> + .p2align 3
> +func1:
> + nop
> + ret
> +
> + .pushsection .text1, "ax", @progbits
> + .option rvc
> + nop
> + .popsection
> +
> +func2:
> + ret
> +
> + .pushsection .text1, "ax", @progbits
> + nop
> + .option norvc
> + .popsection
> +
> + .p2align 3
> +func3:
> + nop
> + ret
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/relax-align.d
> @@ -0,0 +1,34 @@
> +#as: -mrelax
> +#objdump: -dr
> +
> +.*:[ ]+file format .*
> +
> +
> +Disassembly of section .text:
> +
> +0+000 <rvc_func>:
> +[ ]+0:[ ]+8082[ ]+ret
> +[ ]+2:[ ]+0001[ ]+nop
> +
> +0+004 <non_rvc_func>:
> +[ ]+4:[ ]+00008067[ ]+ret
> +
> +0+008 <insn>:
> +[ ]+8:[ ]+00000013[ ]+nop
> +[ ]+c:[ ]+0000[ ]+\.insn 2, 0x0+
> +[ ]+e:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+10:[ ]+00008067[ ]+ret
> +
> +0+0014 <hword>:
> +[ ]+14:[ ]+00000013[ ]+nop
> +[ ]+18:[ ]+0000[ ]+\.short 0x0+
> +[ ]+1a:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+1c:[ ]+00008067[ ]+ret
> +
> +0+0020 <byte>:
> +[ ]+20:[ ]+00000013[ ]+nop
> +[ ]+24:[ ]+00[ ]+\.byte 0x0+
> +[ ]+25:[ ]+00[ ]+\.byte 0x0+
> +[ ]+26:[ ]+0001[ ]+\.insn 2, 0x0*1
> +[ ]+28:[ ]+00008067[ ]+ret
> +#pass
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/relax-align.s
> @@ -0,0 +1,27 @@
> + .text
> + .option rvc
> +rvc_func:
> + ret
> +
> + .option norvc
> + .p2align 2
> +non_rvc_func:
> + ret
> +
> +insn:
> + nop
> + .insn 0
> + .p2align 2
> + ret
> +
> +hword:
> + nop
> + .hword 0
> + .p2align 2
> + ret
> +
> +byte:
> + nop
> + .byte 0
> + .p2align 2
> + ret
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -994,7 +994,7 @@ riscv_disassemble_insn (bfd_vma memaddr,
> {
> i -= 2;
> word = bfd_get_bits (packet + i, 16, false);
> - if (!word && !printed)
> + if (!word && !printed && i)
> continue;
>
> (*info->fprintf_styled_func) (info->stream, dis_style_immediate,
More information about the Binutils
mailing list