public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Binutils <binutils@sourceware.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Waterman <andrew@sifive.com>,
	Jim Wilson <jim.wilson.gcc@gmail.com>,
	Nelson Chu <nelson@rivosinc.com>
Subject: [PATCH v2 7/7] RISC-V: adjust logic to avoid register name symbols
Date: Fri, 10 Mar 2023 10:28:23 +0100	[thread overview]
Message-ID: <54b0d4cc-855a-78c7-5233-22f7d454c0c4@suse.com> (raw)
In-Reply-To: <ba68222c-7de6-280d-e98e-503e3c27addc@suse.com>

Special casing GPR names in my_getSmallExpression() leads to a number of
inconsistencies. Generalize this by utilizing the md_parse_name() hook,
limited to when instruction operands are being parsed (really: probed).
Then both the GPR lookup there and the yet more ad hoc workaround for
PR/gas 29940 can be removed (including its extension needed for making
the compressed form JAL work again).
---
When a floating point extension (but not [hfdq]inx) is enabled, floating
point registers would perhaps better be recognized by riscv_parse_name()
as well. Same for vector registers and likely also CSRs. Otherwise
inconsistent behavior may (continue to) result when equates come into
play, afaict.

Considering equates, the new behavior isn't the only sensible one: We
could also omit the symbol_find() call, thus never honoring equates.
Then, however, even with the "i" insn suffix (or sometimes infix)
explicitly specified, equates with names matching a GPR name (or, see
above, potentially also other register names) wouldn't be accepted
anymore. I guess the primary question here is what the intended (and
consistent) behavior is to be.

Overall the uses of my_getSmallExpression() vs my_getExpression() look
pretty random to me: Is there any underlying principle when which of the
two is expected to be used?
---
v2: Re-work to not break things like %hi(<register-name>).

--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -171,6 +171,8 @@ static enum float_abi float_abi = FLOAT_
 
 static unsigned elf_flags = 0;
 
+static bool probing_insn_operands;
+
 /* Set the default_isa_spec.  Return 0 if the spec isn't supported.
    Otherwise, return 1.  */
 
@@ -2228,21 +2230,10 @@ my_getSmallExpression (expressionS *ep,
 		       char *str, const struct percent_op_match *percent_op)
 {
   size_t reloc_index;
-  unsigned crux_depth, str_depth, regno;
+  unsigned crux_depth, str_depth;
+  bool orig_probing = probing_insn_operands;
   char *crux;
 
-  /* First, check for integer registers.  No callers can accept a reg, but
-     we need to avoid accidentally creating a useless undefined symbol below,
-     if this is an instruction pattern that can't match.  A glibc build fails
-     if this is removed.  */
-  if (reg_lookup (&str, RCLASS_GPR, &regno))
-    {
-      ep->X_op = O_register;
-      ep->X_add_number = regno;
-      expr_parse_end = str;
-      return 0;
-    }
-
   /* Search for the start of the main expression.
 
      End the loop with CRUX pointing to the start of the main expression and
@@ -2274,9 +2265,17 @@ my_getSmallExpression (expressionS *ep,
        return 0;
     }
 
+  /* Anything inside parentheses or subject to a relocation operator cannot
+     be a register and hence can be treated the same as operands to
+     directives (other than .insn).  */
+  if (str_depth || reloc_index)
+    probing_insn_operands = false;
+
   my_getExpression (ep, crux);
   str = expr_parse_end;
 
+  probing_insn_operands = orig_probing;
+
   /* Match every open bracket.  */
   while (crux_depth > 0 && (*str == ')' || *str == ' ' || *str == '\t'))
     if (*str++ == ')')
@@ -2462,6 +2461,13 @@ riscv_is_priv_insn (insn_t insn)
 	  || ((insn ^ MATCH_SFENCE_VM) & MASK_SFENCE_VM) == 0);
 }
 
+static symbolS *deferred_sym_rootP;
+static symbolS *deferred_sym_lastP;
+/* Since symbols can't easily be freed, try to recycle ones which weren't
+   committed.  */
+static symbolS *orphan_sym_rootP;
+static symbolS *orphan_sym_lastP;
+
 /* This routine assembles an instruction into its binary format.  As a
    side effect, it sets the global variable imm_reloc to the type of
    relocation to do if one of the operands is an address expression.  */
@@ -2497,6 +2503,8 @@ riscv_ip (char *str, struct riscv_cl_ins
 
   insn = (struct riscv_opcode *) str_hash_find (hash, str);
 
+  probing_insn_operands = true;
+
   asargStart = asarg;
   for ( ; insn && insn->name && strcmp (insn->name, str) == 0; insn++)
     {
@@ -2513,6 +2521,17 @@ riscv_ip (char *str, struct riscv_cl_ins
       /* Reset error message of the previous round.  */
       error.msg = _("illegal operands");
       error.missing_ext = NULL;
+
+      /* Purge deferred symbols from the previous round, if any.  */
+      while (deferred_sym_rootP)
+	{
+	  symbolS *sym = deferred_sym_rootP;
+
+	  symbol_remove (sym, &deferred_sym_rootP, &deferred_sym_lastP);
+	  symbol_append (sym, orphan_sym_lastP, &orphan_sym_rootP,
+			 &orphan_sym_lastP);
+	}
+
       create_insn (ip, insn);
 
       imm_expr->X_op = O_absent;
@@ -2567,9 +2586,22 @@ riscv_ip (char *str, struct riscv_cl_ins
 		}
 	      if (*asarg != '\0')
 		break;
+
 	      /* Successful assembly.  */
 	      error.msg = NULL;
 	      insn_with_csr = false;
+
+	      /* Commit deferred symbols, if any.  */
+	      while (deferred_sym_rootP)
+		{
+		  symbolS *sym = deferred_sym_rootP;
+
+		  symbol_remove (sym, &deferred_sym_rootP,
+				 &deferred_sym_lastP);
+		  symbol_append (sym, symbol_lastP, &symbol_rootP,
+				 &symbol_lastP);
+		  symbol_table_insert (sym);
+		}
 	      goto out;
 
 	    case 'C': /* RVC */
@@ -2773,8 +2805,6 @@ riscv_ip (char *str, struct riscv_cl_ins
 		case 'p':
 		  goto branch;
 		case 'a':
-		  if (oparg == insn->args + 1)
-		    goto jump_check_gpr;
 		  goto jump;
 		case 'S': /* Floating-point RS1 x8-x15.  */
 		  if (!reg_lookup (&asarg, RCLASS_FPR, &regno)
@@ -3278,18 +3308,6 @@ riscv_ip (char *str, struct riscv_cl_ins
 	      continue;
 
 	    case 'a': /* 20-bit PC-relative offset.  */
-	      /* Like in my_getSmallExpression() we need to avoid emitting
-		 a stray undefined symbol if the 1st JAL entry doesn't match,
-		 but the 2nd (with 2 operands) might.  */
-	      if (oparg == insn->args)
-		{
-	    jump_check_gpr:
-		  asargStart = asarg;
-		  if (reg_lookup (&asarg, RCLASS_GPR, NULL)
-		      && (*asarg == ',' || (ISSPACE (*asarg) && asarg[1] == ',')))
-		    break;
-		  asarg = asargStart;
-		}
 	    jump:
 	      my_getExpression (imm_expr, asarg);
 	      asarg = expr_parse_end;
@@ -3512,6 +3530,8 @@ riscv_ip (char *str, struct riscv_cl_ins
   if (save_c)
     *(asargStart  - 1) = save_c;
 
+  probing_insn_operands = false;
+
   return error;
 }
 
@@ -3808,6 +3828,53 @@ riscv_after_parse_args (void)
     flag_dwarf_cie_version = 3;
 }
 
+bool riscv_parse_name (const char *name, struct expressionS *ep,
+		       enum expr_mode mode)
+{
+  unsigned int regno;
+  symbolS *sym;
+
+  if (!probing_insn_operands)
+    return false;
+
+  gas_assert (mode == expr_normal);
+
+  regno = reg_lookup_internal (name, RCLASS_GPR);
+  if (regno == (unsigned int)-1)
+    return false;
+
+  if (symbol_find (name) != NULL)
+    return false;
+
+  /* Create a symbol without adding it to the symbol table yet.
+     Insertion will happen only once we commit to using the insn
+     we're probing operands for.  */
+  for (sym = deferred_sym_rootP; sym; sym = symbol_next (sym))
+    if (strcmp (name, S_GET_NAME (sym)) == 0)
+      break;
+  if (!sym)
+    {
+      for (sym = orphan_sym_rootP; sym; sym = symbol_next (sym))
+	if (strcmp (name, S_GET_NAME (sym)) == 0)
+	  {
+	    symbol_remove (sym, &orphan_sym_rootP, &orphan_sym_lastP);
+	    break;
+	  }
+      if (!sym)
+	sym = symbol_create (name, undefined_section,
+			     &zero_address_frag, 0);
+
+      symbol_append (sym, deferred_sym_lastP, &deferred_sym_rootP,
+		     &deferred_sym_lastP);
+    }
+
+  ep->X_op = O_symbol;
+  ep->X_add_symbol = sym;
+  ep->X_add_number = 0;
+
+  return true;
+}
+
 long
 md_pcrel_from (fixS *fixP)
 {
--- a/gas/config/tc-riscv.h
+++ b/gas/config/tc-riscv.h
@@ -123,6 +123,10 @@ extern void riscv_elf_final_processing (
 /* Adjust debug_line after relaxation.  */
 #define DWARF2_USE_FIXED_ADVANCE_PC 1
 
+#define md_parse_name(name, exp, mode, c) \
+  riscv_parse_name (name, exp, mode)
+bool riscv_parse_name (const char *, struct expressionS *, enum expr_mode);
+
 #define md_finish riscv_md_finish
 #define CONVERT_SYMBOLIC_ATTRIBUTE riscv_convert_symbolic_attribute
 


  parent reply	other threads:[~2023-03-10  9:28 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-10  9:23 [PATCH v2 0/7] RISC-V/gas: insn operand parsing Jan Beulich
2023-03-10  9:25 ` [PATCH v2 1/7] RISC-V: minor effort reduction in relocation specifier parsing Jan Beulich
2023-03-10 11:24   ` Jan Beulich
2023-03-10  9:26 ` [PATCH v2 2/7] RISC-V: drop "percent_op" parameter from my_getOpcodeExpression() Jan Beulich
2023-03-10  9:26 ` [PATCH v2 3/7] RISC-V: avoid redundant and misleading/wrong error messages Jan Beulich
2023-03-10  9:27 ` [PATCH v2 4/7] RISC-V: don't recognize bogus relocations Jan Beulich
2023-03-10  9:27 ` [PATCH v2 5/7] RISC-V: relax post-relocation-operator separator expectation Jan Beulich
2023-04-25  8:13   ` Nelson Chu
2023-04-25  8:37   ` Andreas Schwab
2023-04-25  8:44     ` Nelson Chu
2023-04-25  8:52       ` Jan Beulich
2023-04-28  1:29         ` Nelson Chu
2023-04-28  6:05           ` Jan Beulich
2023-04-28  8:03             ` Fangrui Song
2023-05-11 11:27     ` Jan Beulich
2023-03-10  9:27 ` [PATCH v2 6/7] RISC-V: test for expected / no unexpected symbols Jan Beulich
2023-03-10  9:28 ` Jan Beulich [this message]
2023-04-25  8:40 ` [PATCH v2 0/7] RISC-V/gas: insn operand parsing Nelson Chu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54b0d4cc-855a-78c7-5233-22f7d454c0c4@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew@sifive.com \
    --cc=binutils@sourceware.org \
    --cc=jim.wilson.gcc@gmail.com \
    --cc=nelson@rivosinc.com \
    --cc=palmer@dabbelt.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).