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

[Wu Zhangjin <wuzhangjin at gmail dot com>]

On Wed, 2009-11-18 at 19:45 +0000, Richard Sandiford wrote:
> 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.
> 

Hi, Richard

The above implementation works, I have merged it into "[PATCH v4] Fixups
of Loongson2F".

Thanks & Regards,
	Wu Zhangjin