Re: PATCH: Avoid signed/unsigned warnings in tc-arm.c

[Nick Clifton <nickc at redhat dot com>]

Hi Khem,

> Thanks for reviewing it. Here is renewed patch and Zack's suggestion on 
> changelog entry verbatim.

Sorry - but I still do not think that this patch is correct.  The 
problem is the choice of which things you chose to convert into signed 
values:

>  static void
>  do_shift (void)
>  {
> -  unsigned int Rm = (inst.operands[1].present
> +  int Rm = (inst.operands[1].present
>  		     ? inst.operands[1].reg
>  		     : inst.operands[0].reg);

Rm here is a register number.  There are no negative register values. 
Hence it makes sense that it is an unsigned quantity.  I realise that 
this change is because you have made the "reg" field of the operands 
sub-structure a signed integer, but I feel that this too is wrong for 
the same reason.

Looking at the error messages generated by gcc 4.0 I realise that the 
problem is the comparison between the "imm" field and the "reg" field at 
the end of do_ldrd().  This happens because the "imm" field is being 
overloaded and used as a secondary "reg" field.  Ideally we would use a 
union of an signed and an unsigned integer to get around this, but that 
is getting needlessly complicated.

A cleaner approach in my opinion then is to allow the "imm" field to 
become a signed field as needed to pacify other warnings, but to make it 
explicitly signed not just "int" but "signed int".   Keep the "reg" 
field as unsigned and when the two fields are compared use casts to 
convert the overloaded "imm" field to an unsigned value.

ie - the attached patch, which is the version that I am going to check in.

Cheers
  Nick

Index: gas/config/tc-arm.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-arm.c,v
retrieving revision 1.211
diff -c -3 -p -r1.211 tc-arm.c
*** gas/config/tc-arm.c	30 Jun 2005 18:33:16 -0000	1.211
--- gas/config/tc-arm.c	4 Jul 2005 14:18:35 -0000
*************** struct arm_it
*** 226,232 ****
    struct
    {
      unsigned reg;
!     unsigned imm;
      unsigned present	: 1;  /* operand present */
      unsigned isreg	: 1;  /* operand was a register */
      unsigned immisreg	: 1;  /* .imm field is a second register */
--- 226,232 ----
    struct
    {
      unsigned reg;
!     signed int imm;
      unsigned present	: 1;  /* operand present */
      unsigned isreg	: 1;  /* operand was a register */
      unsigned immisreg	: 1;  /* .imm field is a second register */
*************** parse_reg_list (char ** strp)
*** 1035,1041 ****
     register.  Double precision registers are matched if DP is nonzero.	*/
  
  static int
! parse_vfp_reg_list (char **str, int *pbase, int dp)
  {
    int base_reg;
    int new_base;
--- 1035,1041 ----
     register.  Double precision registers are matched if DP is nonzero.	*/
  
  static int
! parse_vfp_reg_list (char **str, unsigned int *pbase, int dp)
  {
    int base_reg;
    int new_base;
*************** static void
*** 2335,2341 ****
  s_arm_unwind_save_vfp (void)
  {
    int count;
!   int reg;
    valueT op;
  
    count = parse_vfp_reg_list (&input_line_pointer, &reg, 1);
--- 2335,2341 ----
  s_arm_unwind_save_vfp (void)
  {
    int count;
!   unsigned int reg;
    valueT op;
  
    count = parse_vfp_reg_list (&input_line_pointer, &reg, 1);
*************** enum operand_parse_code
*** 3574,3580 ****
     structure.  Returns SUCCESS or FAIL depending on whether the
     specified grammar matched.  */
  static int
! parse_operands (char *str, const char *pattern)
  {
    unsigned const char *upat = pattern;
    char *backtrack_pos = 0;
--- 3574,3580 ----
     structure.  Returns SUCCESS or FAIL depending on whether the
     specified grammar matched.  */
  static int
! parse_operands (char *str, const unsigned char *pattern)
  {
    unsigned const char *upat = pattern;
    char *backtrack_pos = 0;
*************** do_ldrd (void)
*** 4645,4652 ****
        /* For an index-register load, the index register must not overlap the
  	 destination (even if not write-back).	*/
        else if (inst.operands[2].immisreg
! 	       && (inst.operands[2].imm == inst.operands[0].reg
! 		   || inst.operands[2].imm == inst.operands[1].reg))
  	as_warn (_("index register overlaps destination register"));
      }
  
--- 4645,4652 ----
        /* For an index-register load, the index register must not overlap the
  	 destination (even if not write-back).	*/
        else if (inst.operands[2].immisreg
! 	       && ((unsigned) inst.operands[2].imm == inst.operands[0].reg
! 		   || (unsigned) inst.operands[2].imm == inst.operands[1].reg))
  	as_warn (_("index register overlaps destination register"));
      }