Re: [PATCH v3] Work around the NOP issue of Loongson2F

[Richard Sandiford <rdsandiford at googlemail dot com>]

Wu Zhangjin <wuzhangjin@gmail.com> writes:
> [...]
>> > 
>> > Even if the answer's no, I'm uncomfortable with one kind of nop being
>> > treated differently.
>> 
>> The same to me, I will try to find out how does the .align and .fill
>> works and why it uses the 0 as the nop but doesn't use the NOP_INSN
>> initialized in md_begin(). what about your suggestion here? I have tried
>> to return NOP_INSN->insn_opcode in mips_nop_opcode() and modify
>> mips_handle_align():
>> 
>> -      md_number_to_chars (p, mips16_nop_insn.insn_opcode, 2);
>> +      md_number_to_chars (p, NOP_INSN->insn_opcode, 2);
>> 
>> It did generate the source code like this: 8250825,
>
> If do the following change:
>
> -	fragp->fr_var = 2;
> +	fragp->fr_var = 4;
>
> It works, so, the final solution:
>
> gas/config/tc-mips.c:
>
> @@ -14779,11 +14801,18 @@ static procS *cur_proc_ptr;
>  static int numprocs;
>  
>  /* Implement NOP_OPCODE.  We encode a MIPS16 nop as "1" and a normal
> -   nop as "0".  */
> +   nop as "0".
> +
> +   If -mfix-loongson2f is passed, we encode the nop as "1". This is
> +   needed to enable the work around in mips_handle_align() for
> +   loongson2f.
> +   */
>  
>  char
>  
>  char
>  mips_nop_opcode (void)
>  {
> +  if (mips_fix_loongson2f)
> +         return 1;
>    return seg_info (now_seg)->tc_segment_info_data.mips16;
>  }
>  
> @@ -14809,8 +14838,13 @@ mips_handle_align (fragS *fragp)
>           *p++ = 0;
>           fragp->fr_fix++;
>         }
> -      md_number_to_chars (p, mips16_nop_insn.insn_opcode, 2);
> -      fragp->fr_var = 2;
> +      if (mips_fix_loongson2f && !mips_opts.mips16) {
> +        md_number_to_chars (p, nop_insn.insn_opcode, 4);
> +        fragp->fr_var = 4;
> +      } else {
> +        md_number_to_chars (p, mips16_nop_insn.insn_opcode, 2);
> +        fragp->fr_var = 2;
> +      }
>      }
>  }

This isn't right:

- You're changing the behaviour for MIPS16 nops when -mfix-loongson2f
  is specified.  (This is a valid combination; the application might
  only execute the MIPS16 code when MIPS16 support is detected.)

- "p" points to 3 bytes only, so you could get buffer overrun in the
  Loongson nop.

- You're not handling the case where (bytes & 3) is either 2 or 3.
  You need to add 2 or 3 zeros to the end of the fixed part of the
  frag in that case, whereas the patch would add 0 and 1 respectively.

The right fix IMO is to use something like the following for
mips_handle_align:

  char *p;
  valueT opcode, size, bytes, excess;

  if (fragp->fr_type != rs_align_code)
    return;

  /* Pick the opcode recorded by mips_nop_opcde.  */
  p = fragp->fr_literal + fragp->fr_fix;
  if (*p)
    {
      opcode = mips16_nop_insn.insn_opcode;
      size = 2;
    }
  else
    {
      opcode = nop_insn.insn_opcode;
      size = 4;
    }

  bytes = fragp->fr_next->fr_address - fragp->fr_address - fragp->fr_fix;
  excess = bytes % size;
  if (excess != 0)
    {
      /* If we're not inserting a whole number of instructions, pad the
         end of the fixed part of the frag with zeros.  */
      memset (p, 0, excess);
      p += excess;
      fragp->fr_fix += excess;
    }
  md_number_to_chars (p, opcode, size);
  fragp->fr_var = size;

and then change the tc-mips.h definition of MAX_MEM_FOR_RS_ALIGN_CODE to:

  #define MAX_MEM_FOR_RS_ALIGN_CODE  (3 + 4)

(completely untested).  No changes should be needed to mips_nop_opcode.

Richard