[2/3] RISC-V: correct alignment directive handling for text sections

  .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.
  

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,
  

Probably refer to this,
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/210#issuecomment-929769772

On Wed, Sep 4, 2024 at 7:03 AM Palmer Dabbelt <palmer@dabbelt.com> wrote:

> 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.
>
  

On 04.09.2024 01:02, Palmer Dabbelt wrote:
> 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.

Mind me asking if there has been any progress here?

Jan
  

Hi Jan,

I think this should be the correct way to do it for mixed data/text cases,
although the current compiler, both gcc and llvm, won't generate this kind
of code, it should be worth trying and do something in advance.  Since
there still isn't any spec that gives a rule for mixed data/text cases (
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/210#issuecomment-929769772),
I think it should be fine to go ahead and try :-)

I am not sure who should need to know the changes, so I at least cc Kito
and Maskray.  Please feel free to cc or reference this information to
anyone who will need to know.

Thanks
Nelson

On Tue, Sep 24, 2024 at 4:31 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 04.09.2024 01:02, Palmer Dabbelt wrote:
> > 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.
>
> Mind me asking if there has been any progress here?
>
> Jan
>
  

On Wed, Sep 25, 2024 at 10:44 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
> Hi Jan,
>
> I think this should be the correct way to do it for mixed data/text cases, although the current compiler, both gcc and llvm, won't generate this kind of code, it should be worth trying and do something in advance.  Since there still isn't any spec that gives a rule for mixed data/text cases (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/210#issuecomment-929769772), I think it should be fine to go ahead and try :-)
>
> I am not sure who should need to know the changes, so I at least cc Kito and Maskray.  Please feel free to cc or reference this information to anyone who will need to know.
>
> Thanks
> Nelson
>
> On Tue, Sep 24, 2024 at 4:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.09.2024 01:02, Palmer Dabbelt wrote:
>> > 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.
>>
>> Mind me asking if there has been any progress here?
>>
>> Jan

For the new test relax-align.s, gas without this patch seems to
produce `ret` at a wrong location.
In contrast, `llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax`
produces the correct `ret`.

I haven't checked the functionality, but the new test looks correct.

The behavior difference from the current behavior (wrong; not used by
GCC codegen) should not hold off landing this patch.


(Dropping maskray@google.com as I am leaving Google and the email
address will bounce soon :)