fix and clean up Xtensa GAS operand checks

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

fix and clean up Xtensa GAS operand checks

Bob Wilson
The error checking for symbolic operands in the Xtensa port of GAS has been
convoluted and confused.  This patch cleans it up a bit.  Expressions for the
difference of 2 symbols have not been allowed as operands of Xtensa instructions
for some time, but there was still some complicated logic to check for them in
certain cases depending on whether the instruction might later be relaxed.  It
is somewhat simpler now, and the patch also reduces the memory usage by removing
some fields in Xtensa-specific data structures.  Tested with an xtensa-elf build
and committed on mainline.

2006-01-31  Bob Wilson  <[hidden email]>

        * config/xtensa-istack.h (TInsn): Remove record_fix and sub_symbol
        fields.
        * config/tc-xtensa.h (xtensa_frag_type): Remove slot_sub_symbols field.
        * config/tc-xtensa.c (md_apply_fix): Check for unexpected uses of
        subtracted symbols.
        (relaxation_requirements): Add pfinish_frag argument and use it to
        replace setting tinsn->record_fix fields.
        (xg_assemble_vliw_tokens): Adjust calls to relaxation_requirements
        and vinsn_to_insnbuf.  Remove references to record_fix and
        slot_sub_symbols fields.
        (xtensa_mark_narrow_branches): Delete unused code.
        (is_narrow_branch_guaranteed_in_range): Handle expr that is not just
        a symbol.
        (convert_frag_immed): Adjust vinsn_to_insnbuf call and do not set
        record_fix fields.
        (tinsn_immed_from_frag): Remove code for handling slot_sub_symbols.
        (vinsn_to_insnbuf): Change use of record_fixup argument, replacing use
        of the record_fix field.  Simplify error messages for unexpected
        symbolic operands.
        (set_expr_symbol_offset_diff): Delete.


Index: gas/config/tc-xtensa.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-xtensa.c,v
retrieving revision 1.54
diff -u -p -r1.54 tc-xtensa.c
--- gas/config/tc-xtensa.c 26 Jan 2006 05:21:43 -0000 1.54
+++ gas/config/tc-xtensa.c 31 Jan 2006 04:19:50 -0000
@@ -510,8 +510,6 @@ void set_expr_const (expressionS *, offs
 bfd_boolean expr_is_register (const expressionS *);
 offsetT get_expr_register (const expressionS *);
 void set_expr_symbol_offset (expressionS *, symbolS *, offsetT);
-static void set_expr_symbol_offset_diff
-  (expressionS *, symbolS *, symbolS *, offsetT);
 bfd_boolean expr_is_equal (expressionS *, expressionS *);
 static void copy_expr (expressionS *, const expressionS *);
 
@@ -5537,12 +5535,19 @@ md_apply_fix (fixS *fixP, valueT *valP,
   char *const fixpos = fixP->fx_frag->fr_literal + fixP->fx_where;
   valueT val = 0;
 
+  /* Subtracted symbols are only allowed for a few relocation types, and
+     unless linkrelax is enabled, they should not make it to this point.  */
+  if (fixP->fx_subsy && !(linkrelax && (fixP->fx_r_type == BFD_RELOC_32
+ || fixP->fx_r_type == BFD_RELOC_16
+ || fixP->fx_r_type == BFD_RELOC_8)))
+    as_bad_where (fixP->fx_file, fixP->fx_line, _("expression too complex"));
+
   switch (fixP->fx_r_type)
     {
     case BFD_RELOC_32:
     case BFD_RELOC_16:
     case BFD_RELOC_8:
-      if (linkrelax && fixP->fx_subsy)
+      if (fixP->fx_subsy)
  {
   switch (fixP->fx_r_type)
     {
@@ -6460,8 +6465,9 @@ xg_find_narrowest_format (vliw_insn *vin
    each tinsn in the vinsn.  */
 
 static int
-relaxation_requirements (vliw_insn *vinsn)
+relaxation_requirements (vliw_insn *vinsn, bfd_boolean *pfinish_frag)
 {
+  bfd_boolean finish_frag = FALSE;
   int extra_space = 0;
   int slot;
 
@@ -6479,13 +6485,6 @@ relaxation_requirements (vliw_insn *vins
       /* Difference in bytes between narrow and wide insns...  */
       extra_space += 1;
       tinsn->subtype = RELAX_NARROW;
-      tinsn->record_fix = TRUE;
-      break;
-    }
-  else
-    {
-      tinsn->record_fix = FALSE;
-      /* No extra_space needed.  */
     }
  }
       else
@@ -6510,16 +6509,17 @@ relaxation_requirements (vliw_insn *vins
       tinsn->literal_space = max_literal_size;
 
       tinsn->subtype = RELAX_IMMED;
-      tinsn->record_fix = FALSE;
       extra_space += max_size;
     }
   else
     {
-      tinsn->record_fix = TRUE;
-      /* No extra space needed.  */
+      /* A fix record will be added for this instruction prior
+ to relaxation, so make it end the frag.  */
+      finish_frag = TRUE;
     }
  }
     }
+  *pfinish_frag = finish_frag;
   return extra_space;
 }
 
@@ -6635,7 +6635,7 @@ total_frag_text_expansion (fragS *fragP)
 static void
 xg_assemble_vliw_tokens (vliw_insn *vinsn)
 {
-  bfd_boolean finish_frag = FALSE;
+  bfd_boolean finish_frag;
   bfd_boolean is_jump = FALSE;
   bfd_boolean is_branch = FALSE;
   xtensa_isa isa = xtensa_default_isa;
@@ -6762,7 +6762,7 @@ xg_assemble_vliw_tokens (vliw_insn *vins
 
   insn_size = xtensa_format_length (isa, vinsn->format);
 
-  extra_space = relaxation_requirements (vinsn);
+  extra_space = relaxation_requirements (vinsn, &finish_frag);
 
   /* vinsn_to_insnbuf will produce the error.  */
   if (vinsn->format != XTENSA_UNDEFINED)
@@ -6772,7 +6772,7 @@ xg_assemble_vliw_tokens (vliw_insn *vins
       frag_now->tc_frag_data.is_insn = TRUE;
     }
 
-  vinsn_to_insnbuf (vinsn, f, frag_now, TRUE);
+  vinsn_to_insnbuf (vinsn, f, frag_now, FALSE);
   if (vinsn->format == XTENSA_UNDEFINED)
     return;
 
@@ -6790,7 +6790,6 @@ xg_assemble_vliw_tokens (vliw_insn *vins
       TInsn *tinsn = &vinsn->slots[slot];
       frag_now->tc_frag_data.slot_subtypes[slot] = tinsn->subtype;
       frag_now->tc_frag_data.slot_symbols[slot] = tinsn->symbol;
-      frag_now->tc_frag_data.slot_sub_symbols[slot] = tinsn->sub_symbol;
       frag_now->tc_frag_data.slot_offsets[slot] = tinsn->offset;
       frag_now->tc_frag_data.literal_frags[slot] = tinsn->literal_frag;
       if (tinsn->literal_space != 0)
@@ -6803,8 +6802,8 @@ xg_assemble_vliw_tokens (vliw_insn *vins
       if (xtensa_opcode_is_branch (isa, tinsn->opcode) == 1)
  is_branch = TRUE;
 
-      if (tinsn->subtype || tinsn->symbol || tinsn->record_fix
-  || tinsn->offset || tinsn->literal_frag || is_jump || is_branch)
+      if (tinsn->subtype || tinsn->symbol || tinsn->offset
+  || tinsn->literal_frag || is_jump || is_branch)
  finish_frag = TRUE;
     }
 
@@ -7038,15 +7037,10 @@ xtensa_mark_narrow_branches (void)
       && fragP->tc_frag_data.slot_subtypes[0] == RELAX_IMMED)
     {
       vliw_insn vinsn;
-      const expressionS *expr;
-      symbolS *symbolP;
 
       vinsn_from_chars (&vinsn, fragP->fr_opcode);
       tinsn_immed_from_frag (&vinsn.slots[0], fragP, 0);
 
-      expr = &vinsn.slots[0].tok[1];
-      symbolP = expr->X_add_symbol;
-
       if (vinsn.num_slots == 1
   && xtensa_opcode_is_branch (xtensa_default_isa,
       vinsn.slots[0].opcode)
@@ -7087,8 +7081,14 @@ is_narrow_branch_guaranteed_in_range (fr
 {
   const expressionS *expr = &tinsn->tok[1];
   symbolS *symbolP = expr->X_add_symbol;
-  fragS *target_frag = symbol_get_frag (symbolP);
   offsetT max_distance = expr->X_add_number;
+  fragS *target_frag;
+
+  if (expr->X_op != O_symbol)
+    return FALSE;
+
+  target_frag = symbol_get_frag (symbolP);
+
   max_distance += (S_GET_VALUE (symbolP) - target_frag->fr_address);
   if (is_branch_jmp_to_next (tinsn, fragP))
     return FALSE;
@@ -9198,7 +9198,7 @@ convert_frag_immed (segT segP,
   build_nop (&cur_vinsn.slots[0], bytes);
   fragP->fr_fix += fragP->tc_frag_data.text_expansion[0];
  }
-      vinsn_to_insnbuf (&cur_vinsn, fr_opcode, frag_now, FALSE);
+      vinsn_to_insnbuf (&cur_vinsn, fr_opcode, frag_now, TRUE);
       xtensa_insnbuf_to_chars
  (isa, cur_vinsn.insnbuf, (unsigned char *) fr_opcode, 0);
       fragP->fr_var = 0;
@@ -9333,7 +9333,6 @@ convert_frag_immed (segT segP,
   first = FALSE;
   if (opcode_fits_format_slot (tinsn->opcode, fmt, slot))
     {
-      tinsn->record_fix = TRUE;
       cur_vinsn.slots[slot] = *tinsn;
     }
   else
@@ -9341,7 +9340,6 @@ convert_frag_immed (segT segP,
       cur_vinsn.slots[slot].opcode =
  xtensa_format_slot_nop_opcode (isa, fmt, slot);
       cur_vinsn.slots[slot].ntok = 0;
-      cur_vinsn.slots[slot].record_fix = FALSE;
     }
   vinsn_to_insnbuf (&cur_vinsn, immed_instr, fragP, TRUE);
   xtensa_insnbuf_to_chars (isa, cur_vinsn.insnbuf,
@@ -11042,12 +11040,7 @@ tinsn_has_invalid_symbolic_operands (con
  default:
   /* Symbolic immediates are only allowed on the last immediate
      operand.  At this time, CONST16 is the only opcode where we
-     support non-PC-relative relocations.  (It isn't necessary
-     to complain about non-PC-relative relocations here, but
-     otherwise, no error is reported until the relocations are
-     generated, and the assembler won't get that far if there
-     are any other errors.  It's nice to see all the problems
-     at once.)  */
+     support non-PC-relative relocations.  */
   if (i != get_relaxable_immed (insn->opcode)
       || (xtensa_operand_is_PCrelative (isa, insn->opcode, i) != 1
   && insn->opcode != xtensa_const16_opcode))
@@ -11284,21 +11277,9 @@ tinsn_immed_from_frag (TInsn *tinsn, fra
     {
       opnum = get_relaxable_immed (opcode);
       assert (opnum >= 0);
-      if (fragP->tc_frag_data.slot_sub_symbols[slot])
- {
-  set_expr_symbol_offset_diff
-    (&tinsn->tok[opnum],
-     fragP->tc_frag_data.slot_symbols[slot],
-     fragP->tc_frag_data.slot_sub_symbols[slot],
-     fragP->tc_frag_data.slot_offsets[slot]);
- }
-      else
- {
-  set_expr_symbol_offset
-    (&tinsn->tok[opnum],
-     fragP->tc_frag_data.slot_symbols[slot],
-     fragP->tc_frag_data.slot_offsets[slot]);
- }
+      set_expr_symbol_offset (&tinsn->tok[opnum],
+      fragP->tc_frag_data.slot_symbols[slot],
+      fragP->tc_frag_data.slot_offsets[slot]);
     }
 }
 
@@ -11401,17 +11382,8 @@ xg_free_vinsn (vliw_insn *v)
 }
 
 
-/* Before this is called, we should have
-   filled out the following fields:
-
-   1) the number of operands for each opcode are correct
-   2) the tinsn in the slots are ITYPE_INSN
-   3) ONLY the relaxable_ is built
-   4) All operands are
-       O_constant, O_symbol
-      All constants fit
-
-   The return value tells whether there are any remaining O_symbols.  */
+/* Encode a vliw_insn into an insnbuf.  Return TRUE if there are any symbolic
+   operands.  See also the assumptions listed for tinsn_to_slotbuf.  */
 
 static bfd_boolean
 vinsn_to_insnbuf (vliw_insn *vinsn,
@@ -11436,14 +11408,7 @@ vinsn_to_insnbuf (vliw_insn *vinsn,
 
       xtensa_format_set_slot (isa, fmt, slot,
       insnbuf, vinsn->slotbuf[slot]);
-      /* tinsn_has_fixup tracks if there is a fixup at all.
- record_fixup controls globally.  I.E., we use this
- function from several places, some of which are after
- fixups have already been recorded.  Finally,
- tinsn->record_fixup controls based on the individual ops,
- which may or may not need it based on the relaxation
- requirements.  */
-      if (tinsn_has_fixup && record_fixup)
+      if (tinsn_has_fixup)
  {
   int i;
   xtensa_opcode opcode = tinsn->opcode;
@@ -11460,48 +11425,35 @@ vinsn_to_insnbuf (vliw_insn *vinsn,
  case O_hi16:
   if (get_relaxable_immed (opcode) == i)
     {
-      if (tinsn->record_fix || expr->X_op != O_symbol)
+      /* Add a fix record for the instruction, except if this
+ function is being called prior to relaxation, i.e.,
+ if record_fixup is false, and the instruction might
+ be relaxed later.  */
+      if (record_fixup
+  || tinsn->is_specific_opcode
+  || !xg_is_relaxable_insn (tinsn, 0))
  {
-  if (!xg_add_opcode_fix
-      (tinsn, i, fmt, slot, expr, fragP,
-       frag_offset - fragP->fr_literal))
-    as_bad (_("instruction with constant operands does not fit"));
+  xg_add_opcode_fix (tinsn, i, fmt, slot, expr, fragP,
+     frag_offset - fragP->fr_literal);
  }
       else
  {
+  if (expr->X_op != O_symbol)
+    as_bad (_("invalid operand"));
   tinsn->symbol = expr->X_add_symbol;
   tinsn->offset = expr->X_add_number;
  }
     }
   else
-    as_bad (_("invalid operand %d on '%s'"),
-    i, xtensa_opcode_name (isa, opcode));
+    as_bad (_("symbolic operand not allowed"));
   break;
 
  case O_constant:
  case O_register:
   break;
 
- case O_subtract:
-  if (get_relaxable_immed (opcode) == i)
-    {
-      if (tinsn->record_fix)
-  as_bad (_("invalid subtract operand"));
-      else
- {
-  tinsn->symbol = expr->X_add_symbol;
-  tinsn->sub_symbol = expr->X_op_symbol;
-  tinsn->offset = expr->X_add_number;
- }
-    }
-  else
-    as_bad (_("invalid operand %d on '%s'"),
-    i, xtensa_opcode_name (isa, opcode));
-  break;
-
  default:
-  as_bad (_("invalid expression for operand %d on '%s'"),
-  i, xtensa_opcode_name (isa, opcode));
+  as_bad (_("expression too complex"));
   break;
  }
     }
@@ -11607,21 +11559,6 @@ set_expr_symbol_offset (expressionS *s,
 }
 
 
-/* Set the expression to symbol - minus_sym + offset.  */
-
-static void
-set_expr_symbol_offset_diff (expressionS *s,
-     symbolS *sym,
-     symbolS *minus_sym,
-     offsetT offset)
-{
-  s->X_op = O_subtract;
-  s->X_add_symbol = sym;
-  s->X_op_symbol = minus_sym; /* unused */
-  s->X_add_number = offset;
-}
-
-
 /* Return TRUE if the two expressions are equal.  */
 
 bfd_boolean
Index: gas/config/tc-xtensa.h
===================================================================
RCS file: /cvs/src/src/gas/config/tc-xtensa.h,v
retrieving revision 1.18
diff -u -p -r1.18 tc-xtensa.h
--- gas/config/tc-xtensa.h 30 Dec 2005 00:57:27 -0000 1.18
+++ gas/config/tc-xtensa.h 31 Jan 2006 04:19:50 -0000
@@ -242,7 +242,6 @@ struct xtensa_frag_type
   fragS *literal_frags[MAX_SLOTS];
   enum xtensa_relax_statesE slot_subtypes[MAX_SLOTS];
   symbolS *slot_symbols[MAX_SLOTS];
-  symbolS *slot_sub_symbols[MAX_SLOTS];
   offsetT slot_offsets[MAX_SLOTS];
 
   /* The global aligner needs to walk backward through the list of
Index: gas/config/xtensa-istack.h
===================================================================
RCS file: /cvs/src/src/gas/config/xtensa-istack.h,v
retrieving revision 1.7
diff -u -p -r1.7 xtensa-istack.h
--- gas/config/xtensa-istack.h 20 Dec 2005 18:13:32 -0000 1.7
+++ gas/config/xtensa-istack.h 31 Jan 2006 04:19:50 -0000
@@ -51,12 +51,10 @@ typedef struct tinsn_struct
   struct fixP *fixup;
 
   /* Filled out by relaxation_requirements:  */
-  bfd_boolean record_fix;
   enum xtensa_relax_statesE subtype;
   int literal_space;
   /* Filled out by vinsn_to_insnbuf:  */
   symbolS *symbol;
-  symbolS *sub_symbol;
   offsetT offset;
   fragS *literal_frag;
 } TInsn;