[PATCH] RISC-V: Change -march parsing.

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] RISC-V: Change -march parsing.

Maxim Blinov-2
Change -march parsing logic so we can pass "z" prefixed arguments. The
code is originally taken from the riscv-bitmanip branch.

bfd/ChangeLog:

        * bfd/elfnn-riscv.c (riscv_skip_prefix): New function to skip
        extension prefixes.
        (riscv_non_stop_ext_p): Unused function removed.
        (riscv_std_sv_ext_p): Same
        (riscv_non_std_sv_ext_p): Same
        (riscv_merge_arch_attr_info): Replace 3 calls to
        `riscv_merge_non_std_and_sv_ext' with single call to
        `riscv_merge_multi_letter_ext'

        * bfd/elfxx-riscv.c (riscv_parse_std_ext): Break if we
        encounter a 'z' prefix.
        (riscv_get_prefix_class): New function, return prefix class based
        on first few characters of input string.
        (riscv_parse_config): New structure to factor out minor differences
        in extension class parsing behaviour.
        (riscv_parse_sv_or_non_std_ext): Rename to...
        (riscv_parse_prefixed_ext), and parameterise with
        `riscv_parse_config`.
        (riscv_std_z_ext_strtab):
        (riscv_std_s_ext_strtab): String tables of known z/s extensions.
        (riscv_parse_subset): Delegate all non-single-letter parsing work
        to `riscv_parse_prefixed_ext'.

        * bfd/elfxx-riscv.h (riscv_isa_ext_class): New type.

gas/testsuite/ChangeLog:

        * march-ok-s.d, march-ok-sx.d, march-ok-s-with-version: sx is no
        longer valid and s exts must be known, so rename *ok* to *fail*.
        * march-fail-s.l, march-fail-sx.l, march-fail-sx-with-version.l:
        Expected error messages for above.
---
 bfd/elfnn-riscv.c                             | 139 ++++++------
 bfd/elfxx-riscv.c                             | 197 ++++++++++++++----
 bfd/elfxx-riscv.h                             |  20 ++
 .../gas/riscv/march-fail-s-with-version       |   2 +
 ...-version.d => march-fail-s-with-version.d} |   1 +
 .../gas/riscv/march-fail-s-with-version.l     |   2 +
 .../riscv/{march-ok-s.d => march-fail-s.d}    |   1 +
 gas/testsuite/gas/riscv/march-fail-s.l        |   2 +
 .../riscv/{march-ok-sx.d => march-fail-sx.d}  |   3 +-
 gas/testsuite/gas/riscv/march-fail-sx.l       |   2 +
 10 files changed, 264 insertions(+), 105 deletions(-)
 create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version
 rename gas/testsuite/gas/riscv/{march-ok-s-with-version.d => march-fail-s-with-version.d} (68%)
 create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version.l
 rename gas/testsuite/gas/riscv/{march-ok-s.d => march-fail-s.d} (75%)
 create mode 100644 gas/testsuite/gas/riscv/march-fail-s.l
 rename gas/testsuite/gas/riscv/{march-ok-sx.d => march-fail-sx.d} (56%)
 create mode 100644 gas/testsuite/gas/riscv/march-fail-sx.l

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 997f786602..82249fc88a 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -2735,30 +2735,6 @@ riscv_std_ext_p (const char *name)
   return (strlen (name) == 1) && (name[0] != 'x') && (name[0] != 's');
 }
 
-/* Predicator for non-standard extension.  */
-
-static bfd_boolean
-riscv_non_std_ext_p (const char *name)
-{
-  return (strlen (name) >= 2) && (name[0] == 'x');
-}
-
-/* Predicator for standard supervisor extension.  */
-
-static bfd_boolean
-riscv_std_sv_ext_p (const char *name)
-{
-  return (strlen (name) >= 2) && (name[0] == 's') && (name[1] != 'x');
-}
-
-/* Predicator for non-standard supervisor extension.  */
-
-static bfd_boolean
-riscv_non_std_sv_ext_p (const char *name)
-{
-  return (strlen (name) >= 3) && (name[0] == 's') && (name[1] == 'x');
-}
-
 /* Error handler when version mis-match.  */
 
 static void
@@ -2884,53 +2860,95 @@ riscv_merge_std_ext (bfd *ibfd,
   return TRUE;
 }
 
-/* Merge non-standard and supervisor extensions.
-   Return Value:
-     Return FALSE if failed to merge.
+static const char *
+riscv_skip_prefix (const char *ext, riscv_isa_ext_class_t c)
+{
+  switch (c)
+    {
+    case RV_ISA_CLASS_X: return &ext[1];
+    case RV_ISA_CLASS_S: return &ext[1];
+    case RV_ISA_CLASS_Z: return &ext[1];
+    default: return ext;
+    }
+}
 
-   Arguments:
-     `bfd`: bfd handler.
-     `in_arch`: Raw arch string for input object.
-     `out_arch`: Raw arch string for output object.
-     `pin`: subset list for input object, and it'll skip all merged subset after
-            merge.
-     `pout`: Like `pin`, but for output object. */
+/* Compare prefixed extension names canonically.  */
+
+static int
+riscv_prefix_cmp (const char *a, const char *b)
+{
+  riscv_isa_ext_class_t ca = riscv_get_prefix_class (a);
+  riscv_isa_ext_class_t cb = riscv_get_prefix_class (b);
+
+  /* Extension name without prefix  */
+  const char *anp = riscv_skip_prefix (a, ca);
+  const char *bnp = riscv_skip_prefix (b, cb);
+
+  if (ca == cb)
+    return strcmp (anp, bnp);
+
+  return (int)ca - (int)cb;
+}
 
 static bfd_boolean
-riscv_merge_non_std_and_sv_ext (bfd *ibfd,
- riscv_subset_t **pin,
- riscv_subset_t **pout,
- bfd_boolean (*predicate_func) (const char *))
+riscv_merge_multi_letter_ext (bfd *ibfd,
+      riscv_subset_t **pin,
+      riscv_subset_t **pout)
 {
   riscv_subset_t *in = *pin;
   riscv_subset_t *out = *pout;
+  riscv_subset_t *tail;
 
-  for (in = *pin; in != NULL && predicate_func (in->name); in = in->next)
-    riscv_add_subset (&merged_subsets, in->name, in->major_version,
-      in->minor_version);
+  int cmp;
 
-  for (out = *pout; out != NULL && predicate_func (out->name); out = out->next)
+  while (in && out)
     {
-      riscv_subset_t *find_ext =
- riscv_lookup_subset (&merged_subsets, out->name);
-      if (find_ext != NULL)
+      cmp = riscv_prefix_cmp (in->name, out->name);
+
+      if (cmp < 0)
+ {
+  /* `in' comes before `out', append `in' and increment.  */
+  riscv_add_subset (&merged_subsets, in->name, in->major_version,
+    in->minor_version);
+  in = in->next;
+ }
+      else if (cmp > 0)
+ {
+  /* `out' comes before `in', append `out' and increment.  */
+  riscv_add_subset (&merged_subsets, out->name, out->major_version,
+    out->minor_version);
+  out = out->next;
+ }
+      else
  {
-  /* Check version is same or not. */
-  /* TODO: Allow different merge policy.  */
-  if ((find_ext->major_version != out->major_version)
-      || (find_ext->minor_version != out->minor_version))
+  /* Both present, check version and increment both.  */
+  if ((in->major_version != out->major_version)
+      || (in->minor_version != out->minor_version))
     {
-      riscv_version_mismatch (ibfd, find_ext, out);
+      riscv_version_mismatch (ibfd, in, out);
       return FALSE;
     }
+
+  riscv_add_subset (&merged_subsets, out->name, out->major_version,
+    out->minor_version);
+  out = out->next;
+  in = in->next;
  }
-      else
- riscv_add_subset (&merged_subsets, out->name,
-  out->major_version, out->minor_version);
     }
 
-  *pin = in;
-  *pout = out;
+  if (in || out) {
+    /* If we're here, either `in' or `out' is running longer than
+       the other. So, we need to append the corresponding tail.  */
+    tail = in ? in : out;
+
+    while (tail)
+      {
+ riscv_add_subset (&merged_subsets, tail->name, tail->major_version,
+  tail->minor_version);
+ tail = tail->next;
+      }
+  }
+
   return TRUE;
 }
 
@@ -2989,14 +3007,9 @@ riscv_merge_arch_attr_info (bfd *ibfd, char *in_arch, char *out_arch)
   /* Merge standard extension.  */
   if (!riscv_merge_std_ext (ibfd, in_arch, out_arch, &in, &out))
     return NULL;
-  /* Merge non-standard extension.  */
-  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_ext_p))
-    return NULL;
-  /* Merge standard supervisor extension.  */
-  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_std_sv_ext_p))
-    return NULL;
-  /* Merge non-standard supervisor extension.  */
-  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_sv_ext_p))
+
+  /* Merge all non-single letter extensions with single call.  */
+  if (!riscv_merge_multi_letter_ext (ibfd, &in, &out))
     return NULL;
 
   if (xlen_in != xlen_out)
diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index 245717f70f..37f78e969b 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -1193,7 +1193,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
     {
       char subset[2] = {0, 0};
 
-      if (*p == 'x' || *p == 's')
+      if (*p == 'x' || *p == 's' || *p == 'z')
  break;
 
       if (*p == '_')
@@ -1237,28 +1237,58 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
   return p;
 }
 
-/* Parsing function for non-standard and supervisor extensions.
+/* Classify the argument 'arch' into one of riscv_isa_ext_class_t.  */
 
-   Return Value:
-     Points to the end of extensions.
+riscv_isa_ext_class_t
+riscv_get_prefix_class (const char *arch)
+{
+  switch (*arch)
+    {
+    case 's':
+      return RV_ISA_CLASS_S;
 
-   Arguments:
-     `rps`: Hooks and status for parsing subset.
-     `march`: Full arch string.
-     `p`: Curent parsing position.
-     `ext_type`: What kind of extensions, 'x', 's' or 'sx'.
-     `ext_type_str`: Full name for kind of extension.  */
+    case 'x': return RV_ISA_CLASS_X;
+    case 'z': return RV_ISA_CLASS_Z;
+    default: return RV_ISA_CLASS_UNKNOWN;
+    }
+}
+
+/* Structure describing parameters to use when parsing a particular
+   riscv_isa_ext_class_t. One of these should be provided for each
+   possible class, except RV_ISA_CLASS_UNKNOWN.  */
+
+typedef struct riscv_parse_config
+{
+  /* Class of the extension. */
+  riscv_isa_ext_class_t class;
+
+  /* Lower-case prefix string for error printing
+     and internal parser usage, e.g. "z", "x".  */
+  const char *prefix;
+
+  /* Predicate which is used for checking whether
+     this is a "known" extension. For 'x',
+     it always returns true (since they are by
+     definition non-standard and cannot be known.  */
+  bfd_boolean (*ext_valid_p) (const char *);
+} riscv_parse_config_t;
+
+/* Parse a generic prefixed extension.
+   march: The full architecture string as passed in by "-march=...".
+   p: Point from which to start parsing the -march string.
+   config: What class of extensions to parse, predicate funcs,
+   and strings to use in error reporting.  */
 
 static const char *
-riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
-       const char *march,
-       const char *p,
-       const char *ext_type,
-       const char *ext_type_str)
+riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
+  const char *march,
+  const char *p,
+  const riscv_parse_config_t *config)
 {
   unsigned major_version = 0;
   unsigned minor_version = 0;
-  size_t ext_type_len = strlen (ext_type);
+  const char *last_name;
+  riscv_isa_ext_class_t class;
 
   while (*p)
     {
@@ -1268,12 +1298,9 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
   continue;
  }
 
-      if (strncmp (p, ext_type, ext_type_len) != 0)
- break;
-
-      /* It's non-standard supervisor extension if it prefix with sx.  */
-      if ((ext_type[0] == 's') && (ext_type_len == 1)
-  && (*(p + 1) == 'x'))
+      /* Assert that the current extension specifier matches our parsing class.  */
+      class = riscv_get_prefix_class (p);
+      if (class != config->class)
  break;
 
       char *subset = xstrdup (p);
@@ -1294,6 +1321,43 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
 
       *q = '\0';
 
+      /* Check that the name is valid.
+ For 'x', anything goes but it cannot simply be 'x'.
+ For 'z', it must be known from a list and also cannot simply be 'z'.
+ For 's', it must be known from a list and also *can* simply be 's'.  */
+
+      /* Check that the extension name is well-formed.  */
+      if (!config->ext_valid_p (subset))
+ {
+  rps->error_handler
+    ("-march=%s: Invalid or unknown %s ISA extension: '%s'",
+     march, config->prefix, subset);
+  free (subset);
+  return NULL;
+ }
+
+      /* Check that the last item is not the same as this.  */
+      last_name = rps->subset_list->tail->name;
+
+      if (!strcasecmp (last_name, subset))
+ {
+  rps->error_handler ("-march=%s: Duplicate %s ISA extension: \'%s\'",
+      march, config->prefix, subset);
+  free (subset);
+  return NULL;
+ }
+
+      /* Check that we are in alphabetical order within the subset.  */
+      if (!strncasecmp (last_name, config->prefix, 1)
+  && strcasecmp (last_name, subset) > 0)
+ {
+  rps->error_handler ("-march=%s: %s ISA extension not in alphabetical order: "
+      "\'%s\' must come before \'%s\'.",
+      march, config->prefix, subset, last_name);
+  free (subset);
+  return NULL;
+ }
+
       riscv_add_subset (rps->subset_list, subset, major_version, minor_version);
       free (subset);
       p += end_of_version - subset;
@@ -1301,7 +1365,7 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
       if (*p != '\0' && *p != '_')
  {
   rps->error_handler ("-march=%s: %s must seperate with _",
-      march, ext_type_str);
+      march, config->prefix);
   return NULL;
  }
     }
@@ -1309,6 +1373,69 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
   return p;
 }
 
+const char * const riscv_std_z_ext_strtab[] =
+  {
+    NULL
+  };
+
+const char * const riscv_std_s_ext_strtab[] =
+  {
+    NULL
+  };
+
+static bfd_boolean
+riscv_multi_letter_ext_valid_p (const char *ext,
+ const char *const *known_exts)
+{
+  for (size_t i = 0; known_exts[i]; ++i)
+    {
+      if (!strcmp (ext, known_exts[i]))
+ return TRUE;
+    }
+
+  return FALSE;
+}
+
+/* Predicator function for x-prefixed extensions.
+   Anything goes, except the literal 'x'.  */
+
+static bfd_boolean
+riscv_ext_x_valid_p (const char *arg)
+{
+  if (!strcasecmp (arg, "x"))
+    return FALSE;
+
+  return TRUE;
+}
+
+/* Predicator functions for z-prefixed extensions.
+   Only known z-extensions are permitted.  */
+
+static bfd_boolean
+riscv_ext_z_valid_p (const char *arg)
+{
+  return riscv_multi_letter_ext_valid_p (arg, riscv_std_z_ext_strtab);
+}
+
+/* Predicator function for 's' prefixed extensions.
+   Must be either literal 's', or a known s-prefixed extension.  */
+
+static bfd_boolean
+riscv_ext_s_valid_p (const char *arg)
+{
+  return riscv_multi_letter_ext_valid_p (arg, riscv_std_s_ext_strtab);
+}
+
+/* Parsing order that is needed for bitmanip.  */
+
+static const riscv_parse_config_t parse_config[] =
+{
+   {RV_ISA_CLASS_S, "s", riscv_ext_s_valid_p},
+   {RV_ISA_CLASS_Z, "z", riscv_ext_z_valid_p},
+   {RV_ISA_CLASS_X, "x", riscv_ext_x_valid_p},
+   {RV_ISA_CLASS_UNKNOWN, NULL, NULL}
+};
+
 /* Function for parsing arch string.
 
    Return Value:
@@ -1347,26 +1474,14 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
   if (p == NULL)
     return FALSE;
 
-  /* Parsing non-standard extension.  */
-  p = riscv_parse_sv_or_non_std_ext (
- rps, arch, p, "x", "non-standard extension");
-
-  if (p == NULL)
-    return FALSE;
-
-  /* Parsing supervisor extension.  */
-  p = riscv_parse_sv_or_non_std_ext (
- rps, arch, p, "s", "supervisor extension");
-
-  if (p == NULL)
-    return FALSE;
+  /* Parse the different classes of extensions in the specified order.  */
 
-  /* Parsing non-standard supervisor extension.  */
-  p = riscv_parse_sv_or_non_std_ext (
- rps, arch, p, "sx", "non-standard supervisor extension");
+  for (size_t i = 0; i < ARRAY_SIZE (parse_config); ++i) {
+    p = riscv_parse_prefixed_ext (rps, arch, p, &parse_config[i]);
 
-  if (p == NULL)
-    return FALSE;
+    if (p == NULL)
+      return FALSE;
+  }
 
   if (*p != '\0')
     {
diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
index 19f7bd2ecc..12ee7e3276 100644
--- a/bfd/elfxx-riscv.h
+++ b/bfd/elfxx-riscv.h
@@ -86,3 +86,23 @@ riscv_release_subset_list (riscv_subset_list_t *);
 
 extern char *
 riscv_arch_str (unsigned, const riscv_subset_list_t *);
+
+extern const char * const z_ext_strtab[];
+extern const char * const riscv_std_s_ext_strtab[];
+
+/* ISA extension name class. E.g. "zbb" corresponds to RV_ISA_CLASS_Z,
+   "xargs" corresponds to RV_ISA_CLASS_X, etc.  Order is important
+   here.  */
+
+typedef enum riscv_isa_ext_class
+  {
+   RV_ISA_CLASS_S,
+   RV_ISA_CLASS_Z,
+   RV_ISA_CLASS_X,
+   RV_ISA_CLASS_UNKNOWN
+  } riscv_isa_ext_class_t;
+
+/* Classify the argument 'ext' into one of riscv_isa_ext_class_t.  */
+
+riscv_isa_ext_class_t
+riscv_get_prefix_class (const char *ext);
diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version b/gas/testsuite/gas/riscv/march-fail-s-with-version
new file mode 100644
index 0000000000..a514d4aec7
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-s-with-version
@@ -0,0 +1,2 @@
+Assembler messages:
+.*: Invalid or unknown s ISA extension: 'sfoo'
\ No newline at end of file
diff --git a/gas/testsuite/gas/riscv/march-ok-s-with-version.d b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
similarity index 68%
rename from gas/testsuite/gas/riscv/march-ok-s-with-version.d
rename to gas/testsuite/gas/riscv/march-fail-s-with-version.d
index 6296a15c95..9881c2a0e0 100644
--- a/gas/testsuite/gas/riscv/march-ok-s-with-version.d
+++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
@@ -1,5 +1,6 @@
 #as: -march=rv32isfoo3p4
 #objdump: -dr
 #source: empty.s
+#error_output: march-fail-s-with-version.l
 
 .*:     file format elf32-littleriscv
diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version.l b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
new file mode 100644
index 0000000000..6b1f957276
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
@@ -0,0 +1,2 @@
+Assembler messages:
+.*: Invalid or unknown s ISA extension: 'sfoo'
diff --git a/gas/testsuite/gas/riscv/march-ok-s.d b/gas/testsuite/gas/riscv/march-fail-s.d
similarity index 75%
rename from gas/testsuite/gas/riscv/march-ok-s.d
rename to gas/testsuite/gas/riscv/march-fail-s.d
index 7daa0a11d0..ebc8377aaf 100644
--- a/gas/testsuite/gas/riscv/march-ok-s.d
+++ b/gas/testsuite/gas/riscv/march-fail-s.d
@@ -1,5 +1,6 @@
 #as: -march=rv32isfoo
 #objdump: -dr
 #source: empty.s
+#error_output: march-fail-s.l
 
 .*:     file format elf32-littleriscv
diff --git a/gas/testsuite/gas/riscv/march-fail-s.l b/gas/testsuite/gas/riscv/march-fail-s.l
new file mode 100644
index 0000000000..6b1f957276
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-s.l
@@ -0,0 +1,2 @@
+Assembler messages:
+.*: Invalid or unknown s ISA extension: 'sfoo'
diff --git a/gas/testsuite/gas/riscv/march-ok-sx.d b/gas/testsuite/gas/riscv/march-fail-sx.d
similarity index 56%
rename from gas/testsuite/gas/riscv/march-ok-sx.d
rename to gas/testsuite/gas/riscv/march-fail-sx.d
index e2172f2348..144a85c2fb 100644
--- a/gas/testsuite/gas/riscv/march-ok-sx.d
+++ b/gas/testsuite/gas/riscv/march-fail-sx.d
@@ -1,5 +1,6 @@
-#as: -march=rv32isfoo_sxbar
+#as: -march=rv32i_sxbar
 #objdump: -dr
 #source: empty.s
+#error_output: march-fail-sx.l
 
 .*:     file format elf32-littleriscv
diff --git a/gas/testsuite/gas/riscv/march-fail-sx.l b/gas/testsuite/gas/riscv/march-fail-sx.l
new file mode 100644
index 0000000000..b8ead71a3b
--- /dev/null
+++ b/gas/testsuite/gas/riscv/march-fail-sx.l
@@ -0,0 +1,2 @@
+Assembler messages:
+.*: Invalid or unknown s ISA extension: 'sxbar'
--
2.17.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] RISC-V: Change -march parsing.

Nelson Chu-2
Dear Maxim,

Thank you very much.  I have reviewed this in the riscv-bitmanip
branch, it looks great to me :)

The patch has considered the alphabetical order for the Z and S
extensions.  Also, It is easy to extend the sub extensions, we just
need to add the new extensions in the riscv_std_z_ext_strtab and
riscv_std_s_ext_strtab arrays (the new extension also need to be in
alphabetical order).

There were no regressions for the rv32i-elf and rv64gc-linux toolchain
(build and checks).  I think the patch is exactly what we need now :)

Thanks and regards
Nelson

On Wed, Dec 4, 2019 at 7:34 PM Maxim Blinov <[hidden email]> wrote:

>
> Change -march parsing logic so we can pass "z" prefixed arguments. The
> code is originally taken from the riscv-bitmanip branch.
>
> bfd/ChangeLog:
>
>         * bfd/elfnn-riscv.c (riscv_skip_prefix): New function to skip
>         extension prefixes.
>         (riscv_non_stop_ext_p): Unused function removed.
>         (riscv_std_sv_ext_p): Same
>         (riscv_non_std_sv_ext_p): Same
>         (riscv_merge_arch_attr_info): Replace 3 calls to
>         `riscv_merge_non_std_and_sv_ext' with single call to
>         `riscv_merge_multi_letter_ext'
>
>         * bfd/elfxx-riscv.c (riscv_parse_std_ext): Break if we
>         encounter a 'z' prefix.
>         (riscv_get_prefix_class): New function, return prefix class based
>         on first few characters of input string.
>         (riscv_parse_config): New structure to factor out minor differences
>         in extension class parsing behaviour.
>         (riscv_parse_sv_or_non_std_ext): Rename to...
>         (riscv_parse_prefixed_ext), and parameterise with
>         `riscv_parse_config`.
>         (riscv_std_z_ext_strtab):
>         (riscv_std_s_ext_strtab): String tables of known z/s extensions.
>         (riscv_parse_subset): Delegate all non-single-letter parsing work
>         to `riscv_parse_prefixed_ext'.
>
>         * bfd/elfxx-riscv.h (riscv_isa_ext_class): New type.
>
> gas/testsuite/ChangeLog:
>
>         * march-ok-s.d, march-ok-sx.d, march-ok-s-with-version: sx is no
>         longer valid and s exts must be known, so rename *ok* to *fail*.
>         * march-fail-s.l, march-fail-sx.l, march-fail-sx-with-version.l:
>         Expected error messages for above.
> ---
>  bfd/elfnn-riscv.c                             | 139 ++++++------
>  bfd/elfxx-riscv.c                             | 197 ++++++++++++++----
>  bfd/elfxx-riscv.h                             |  20 ++
>  .../gas/riscv/march-fail-s-with-version       |   2 +
>  ...-version.d => march-fail-s-with-version.d} |   1 +
>  .../gas/riscv/march-fail-s-with-version.l     |   2 +
>  .../riscv/{march-ok-s.d => march-fail-s.d}    |   1 +
>  gas/testsuite/gas/riscv/march-fail-s.l        |   2 +
>  .../riscv/{march-ok-sx.d => march-fail-sx.d}  |   3 +-
>  gas/testsuite/gas/riscv/march-fail-sx.l       |   2 +
>  10 files changed, 264 insertions(+), 105 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version
>  rename gas/testsuite/gas/riscv/{march-ok-s-with-version.d => march-fail-s-with-version.d} (68%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version.l
>  rename gas/testsuite/gas/riscv/{march-ok-s.d => march-fail-s.d} (75%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s.l
>  rename gas/testsuite/gas/riscv/{march-ok-sx.d => march-fail-sx.d} (56%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-sx.l
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 997f786602..82249fc88a 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2735,30 +2735,6 @@ riscv_std_ext_p (const char *name)
>    return (strlen (name) == 1) && (name[0] != 'x') && (name[0] != 's');
>  }
>
> -/* Predicator for non-standard extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 'x');
> -}
> -
> -/* Predicator for standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 's') && (name[1] != 'x');
> -}
> -
> -/* Predicator for non-standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 3) && (name[0] == 's') && (name[1] == 'x');
> -}
> -
>  /* Error handler when version mis-match.  */
>
>  static void
> @@ -2884,53 +2860,95 @@ riscv_merge_std_ext (bfd *ibfd,
>    return TRUE;
>  }
>
> -/* Merge non-standard and supervisor extensions.
> -   Return Value:
> -     Return FALSE if failed to merge.
> +static const char *
> +riscv_skip_prefix (const char *ext, riscv_isa_ext_class_t c)
> +{
> +  switch (c)
> +    {
> +    case RV_ISA_CLASS_X: return &ext[1];
> +    case RV_ISA_CLASS_S: return &ext[1];
> +    case RV_ISA_CLASS_Z: return &ext[1];
> +    default: return ext;
> +    }
> +}
>
> -   Arguments:
> -     `bfd`: bfd handler.
> -     `in_arch`: Raw arch string for input object.
> -     `out_arch`: Raw arch string for output object.
> -     `pin`: subset list for input object, and it'll skip all merged subset after
> -            merge.
> -     `pout`: Like `pin`, but for output object. */
> +/* Compare prefixed extension names canonically.  */
> +
> +static int
> +riscv_prefix_cmp (const char *a, const char *b)
> +{
> +  riscv_isa_ext_class_t ca = riscv_get_prefix_class (a);
> +  riscv_isa_ext_class_t cb = riscv_get_prefix_class (b);
> +
> +  /* Extension name without prefix  */
> +  const char *anp = riscv_skip_prefix (a, ca);
> +  const char *bnp = riscv_skip_prefix (b, cb);
> +
> +  if (ca == cb)
> +    return strcmp (anp, bnp);
> +
> +  return (int)ca - (int)cb;
> +}
>
>  static bfd_boolean
> -riscv_merge_non_std_and_sv_ext (bfd *ibfd,
> -                               riscv_subset_t **pin,
> -                               riscv_subset_t **pout,
> -                               bfd_boolean (*predicate_func) (const char *))
> +riscv_merge_multi_letter_ext (bfd *ibfd,
> +                             riscv_subset_t **pin,
> +                             riscv_subset_t **pout)
>  {
>    riscv_subset_t *in = *pin;
>    riscv_subset_t *out = *pout;
> +  riscv_subset_t *tail;
>
> -  for (in = *pin; in != NULL && predicate_func (in->name); in = in->next)
> -    riscv_add_subset (&merged_subsets, in->name, in->major_version,
> -                     in->minor_version);
> +  int cmp;
>
> -  for (out = *pout; out != NULL && predicate_func (out->name); out = out->next)
> +  while (in && out)
>      {
> -      riscv_subset_t *find_ext =
> -       riscv_lookup_subset (&merged_subsets, out->name);
> -      if (find_ext != NULL)
> +      cmp = riscv_prefix_cmp (in->name, out->name);
> +
> +      if (cmp < 0)
> +       {
> +         /* `in' comes before `out', append `in' and increment.  */
> +         riscv_add_subset (&merged_subsets, in->name, in->major_version,
> +                           in->minor_version);
> +         in = in->next;
> +       }
> +      else if (cmp > 0)
> +       {
> +         /* `out' comes before `in', append `out' and increment.  */
> +         riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +                           out->minor_version);
> +         out = out->next;
> +       }
> +      else
>         {
> -         /* Check version is same or not. */
> -         /* TODO: Allow different merge policy.  */
> -         if ((find_ext->major_version != out->major_version)
> -             || (find_ext->minor_version != out->minor_version))
> +         /* Both present, check version and increment both.  */
> +         if ((in->major_version != out->major_version)
> +             || (in->minor_version != out->minor_version))
>             {
> -             riscv_version_mismatch (ibfd, find_ext, out);
> +             riscv_version_mismatch (ibfd, in, out);
>               return FALSE;
>             }
> +
> +         riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +                           out->minor_version);
> +         out = out->next;
> +         in = in->next;
>         }
> -      else
> -       riscv_add_subset (&merged_subsets, out->name,
> -                         out->major_version, out->minor_version);
>      }
>
> -  *pin = in;
> -  *pout = out;
> +  if (in || out) {
> +    /* If we're here, either `in' or `out' is running longer than
> +       the other. So, we need to append the corresponding tail.  */
> +    tail = in ? in : out;
> +
> +    while (tail)
> +      {
> +       riscv_add_subset (&merged_subsets, tail->name, tail->major_version,
> +                         tail->minor_version);
> +       tail = tail->next;
> +      }
> +  }
> +
>    return TRUE;
>  }
>
> @@ -2989,14 +3007,9 @@ riscv_merge_arch_attr_info (bfd *ibfd, char *in_arch, char *out_arch)
>    /* Merge standard extension.  */
>    if (!riscv_merge_std_ext (ibfd, in_arch, out_arch, &in, &out))
>      return NULL;
> -  /* Merge non-standard extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_ext_p))
> -    return NULL;
> -  /* Merge standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_std_sv_ext_p))
> -    return NULL;
> -  /* Merge non-standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_sv_ext_p))
> +
> +  /* Merge all non-single letter extensions with single call.  */
> +  if (!riscv_merge_multi_letter_ext (ibfd, &in, &out))
>      return NULL;
>
>    if (xlen_in != xlen_out)
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 245717f70f..37f78e969b 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1193,7 +1193,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>      {
>        char subset[2] = {0, 0};
>
> -      if (*p == 'x' || *p == 's')
> +      if (*p == 'x' || *p == 's' || *p == 'z')
>         break;
>
>        if (*p == '_')
> @@ -1237,28 +1237,58 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> -/* Parsing function for non-standard and supervisor extensions.
> +/* Classify the argument 'arch' into one of riscv_isa_ext_class_t.  */
>
> -   Return Value:
> -     Points to the end of extensions.
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *arch)
> +{
> +  switch (*arch)
> +    {
> +    case 's':
> +      return RV_ISA_CLASS_S;
>
> -   Arguments:
> -     `rps`: Hooks and status for parsing subset.
> -     `march`: Full arch string.
> -     `p`: Curent parsing position.
> -     `ext_type`: What kind of extensions, 'x', 's' or 'sx'.
> -     `ext_type_str`: Full name for kind of extension.  */
> +    case 'x': return RV_ISA_CLASS_X;
> +    case 'z': return RV_ISA_CLASS_Z;
> +    default: return RV_ISA_CLASS_UNKNOWN;
> +    }
> +}
> +
> +/* Structure describing parameters to use when parsing a particular
> +   riscv_isa_ext_class_t. One of these should be provided for each
> +   possible class, except RV_ISA_CLASS_UNKNOWN.  */
> +
> +typedef struct riscv_parse_config
> +{
> +  /* Class of the extension. */
> +  riscv_isa_ext_class_t class;
> +
> +  /* Lower-case prefix string for error printing
> +     and internal parser usage, e.g. "z", "x".  */
> +  const char *prefix;
> +
> +  /* Predicate which is used for checking whether
> +     this is a "known" extension. For 'x',
> +     it always returns true (since they are by
> +     definition non-standard and cannot be known.  */
> +  bfd_boolean (*ext_valid_p) (const char *);
> +} riscv_parse_config_t;
> +
> +/* Parse a generic prefixed extension.
> +   march: The full architecture string as passed in by "-march=...".
> +   p: Point from which to start parsing the -march string.
> +   config: What class of extensions to parse, predicate funcs,
> +   and strings to use in error reporting.  */
>
>  static const char *
> -riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
> -                              const char *march,
> -                              const char *p,
> -                              const char *ext_type,
> -                              const char *ext_type_str)
> +riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> +                         const char *march,
> +                         const char *p,
> +                         const riscv_parse_config_t *config)
>  {
>    unsigned major_version = 0;
>    unsigned minor_version = 0;
> -  size_t ext_type_len = strlen (ext_type);
> +  const char *last_name;
> +  riscv_isa_ext_class_t class;
>
>    while (*p)
>      {
> @@ -1268,12 +1298,9 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>           continue;
>         }
>
> -      if (strncmp (p, ext_type, ext_type_len) != 0)
> -       break;
> -
> -      /* It's non-standard supervisor extension if it prefix with sx.  */
> -      if ((ext_type[0] == 's') && (ext_type_len == 1)
> -         && (*(p + 1) == 'x'))
> +      /* Assert that the current extension specifier matches our parsing class.  */
> +      class = riscv_get_prefix_class (p);
> +      if (class != config->class)
>         break;
>
>        char *subset = xstrdup (p);
> @@ -1294,6 +1321,43 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>
>        *q = '\0';
>
> +      /* Check that the name is valid.
> +        For 'x', anything goes but it cannot simply be 'x'.
> +        For 'z', it must be known from a list and also cannot simply be 'z'.
> +        For 's', it must be known from a list and also *can* simply be 's'.  */
> +
> +      /* Check that the extension name is well-formed.  */
> +      if (!config->ext_valid_p (subset))
> +       {
> +         rps->error_handler
> +           ("-march=%s: Invalid or unknown %s ISA extension: '%s'",
> +            march, config->prefix, subset);
> +         free (subset);
> +         return NULL;
> +       }
> +
> +      /* Check that the last item is not the same as this.  */
> +      last_name = rps->subset_list->tail->name;
> +
> +      if (!strcasecmp (last_name, subset))
> +       {
> +         rps->error_handler ("-march=%s: Duplicate %s ISA extension: \'%s\'",
> +                             march, config->prefix, subset);
> +         free (subset);
> +         return NULL;
> +       }
> +
> +      /* Check that we are in alphabetical order within the subset.  */
> +      if (!strncasecmp (last_name, config->prefix, 1)
> +         && strcasecmp (last_name, subset) > 0)
> +       {
> +         rps->error_handler ("-march=%s: %s ISA extension not in alphabetical order: "
> +                             "\'%s\' must come before \'%s\'.",
> +                             march, config->prefix, subset, last_name);
> +         free (subset);
> +         return NULL;
> +       }
> +
>        riscv_add_subset (rps->subset_list, subset, major_version, minor_version);
>        free (subset);
>        p += end_of_version - subset;
> @@ -1301,7 +1365,7 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>        if (*p != '\0' && *p != '_')
>         {
>           rps->error_handler ("-march=%s: %s must seperate with _",
> -                             march, ext_type_str);
> +                             march, config->prefix);
>           return NULL;
>         }
>      }
> @@ -1309,6 +1373,69 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> +const char * const riscv_std_z_ext_strtab[] =
> +  {
> +    NULL
> +  };
> +
> +const char * const riscv_std_s_ext_strtab[] =
> +  {
> +    NULL
> +  };
> +
> +static bfd_boolean
> +riscv_multi_letter_ext_valid_p (const char *ext,
> +                               const char *const *known_exts)
> +{
> +  for (size_t i = 0; known_exts[i]; ++i)
> +    {
> +      if (!strcmp (ext, known_exts[i]))
> +       return TRUE;
> +    }
> +
> +  return FALSE;
> +}
> +
> +/* Predicator function for x-prefixed extensions.
> +   Anything goes, except the literal 'x'.  */
> +
> +static bfd_boolean
> +riscv_ext_x_valid_p (const char *arg)
> +{
> +  if (!strcasecmp (arg, "x"))
> +    return FALSE;
> +
> +  return TRUE;
> +}
> +
> +/* Predicator functions for z-prefixed extensions.
> +   Only known z-extensions are permitted.  */
> +
> +static bfd_boolean
> +riscv_ext_z_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_z_ext_strtab);
> +}
> +
> +/* Predicator function for 's' prefixed extensions.
> +   Must be either literal 's', or a known s-prefixed extension.  */
> +
> +static bfd_boolean
> +riscv_ext_s_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_s_ext_strtab);
> +}
> +
> +/* Parsing order that is needed for bitmanip.  */
> +
> +static const riscv_parse_config_t parse_config[] =
> +{
> +   {RV_ISA_CLASS_S, "s", riscv_ext_s_valid_p},
> +   {RV_ISA_CLASS_Z, "z", riscv_ext_z_valid_p},
> +   {RV_ISA_CLASS_X, "x", riscv_ext_x_valid_p},
> +   {RV_ISA_CLASS_UNKNOWN, NULL, NULL}
> +};
> +
>  /* Function for parsing arch string.
>
>     Return Value:
> @@ -1347,26 +1474,14 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
>    if (p == NULL)
>      return FALSE;
>
> -  /* Parsing non-standard extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "x", "non-standard extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> -
> -  /* Parsing supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "s", "supervisor extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> +  /* Parse the different classes of extensions in the specified order.  */
>
> -  /* Parsing non-standard supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "sx", "non-standard supervisor extension");
> +  for (size_t i = 0; i < ARRAY_SIZE (parse_config); ++i) {
> +    p = riscv_parse_prefixed_ext (rps, arch, p, &parse_config[i]);
>
> -  if (p == NULL)
> -    return FALSE;
> +    if (p == NULL)
> +      return FALSE;
> +  }
>
>    if (*p != '\0')
>      {
> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> index 19f7bd2ecc..12ee7e3276 100644
> --- a/bfd/elfxx-riscv.h
> +++ b/bfd/elfxx-riscv.h
> @@ -86,3 +86,23 @@ riscv_release_subset_list (riscv_subset_list_t *);
>
>  extern char *
>  riscv_arch_str (unsigned, const riscv_subset_list_t *);
> +
> +extern const char * const z_ext_strtab[];
> +extern const char * const riscv_std_s_ext_strtab[];
> +
> +/* ISA extension name class. E.g. "zbb" corresponds to RV_ISA_CLASS_Z,
> +   "xargs" corresponds to RV_ISA_CLASS_X, etc.  Order is important
> +   here.  */
> +
> +typedef enum riscv_isa_ext_class
> +  {
> +   RV_ISA_CLASS_S,
> +   RV_ISA_CLASS_Z,
> +   RV_ISA_CLASS_X,
> +   RV_ISA_CLASS_UNKNOWN
> +  } riscv_isa_ext_class_t;
> +
> +/* Classify the argument 'ext' into one of riscv_isa_ext_class_t.  */
> +
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *ext);
> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version b/gas/testsuite/gas/riscv/march-fail-s-with-version
> new file mode 100644
> index 0000000000..a514d4aec7
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> \ No newline at end of file
> diff --git a/gas/testsuite/gas/riscv/march-ok-s-with-version.d b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> similarity index 68%
> rename from gas/testsuite/gas/riscv/march-ok-s-with-version.d
> rename to gas/testsuite/gas/riscv/march-fail-s-with-version.d
> index 6296a15c95..9881c2a0e0 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s-with-version.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo3p4
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s-with-version.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version.l b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-s.d b/gas/testsuite/gas/riscv/march-fail-s.d
> similarity index 75%
> rename from gas/testsuite/gas/riscv/march-ok-s.d
> rename to gas/testsuite/gas/riscv/march-fail-s.d
> index 7daa0a11d0..ebc8377aaf 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s.l b/gas/testsuite/gas/riscv/march-fail-s.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-sx.d b/gas/testsuite/gas/riscv/march-fail-sx.d
> similarity index 56%
> rename from gas/testsuite/gas/riscv/march-ok-sx.d
> rename to gas/testsuite/gas/riscv/march-fail-sx.d
> index e2172f2348..144a85c2fb 100644
> --- a/gas/testsuite/gas/riscv/march-ok-sx.d
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.d
> @@ -1,5 +1,6 @@
> -#as: -march=rv32isfoo_sxbar
> +#as: -march=rv32i_sxbar
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-sx.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-sx.l b/gas/testsuite/gas/riscv/march-fail-sx.l
> new file mode 100644
> index 0000000000..b8ead71a3b
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sxbar'
> --
> 2.17.1
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] RISC-V: Change -march parsing.

Kito Cheng-2
In reply to this post by Maxim Blinov-2
Hi Maxim:

Thanks your patch, it's LGTM, just some minor review comment :)

On Wed, Dec 4, 2019 at 7:34 PM Maxim Blinov <[hidden email]> wrote:

>
> Change -march parsing logic so we can pass "z" prefixed arguments. The
> code is originally taken from the riscv-bitmanip branch.
>
> bfd/ChangeLog:
>
>         * bfd/elfnn-riscv.c (riscv_skip_prefix): New function to skip
>         extension prefixes.
>         (riscv_non_stop_ext_p): Unused function removed.
>         (riscv_std_sv_ext_p): Same
>         (riscv_non_std_sv_ext_p): Same
>         (riscv_merge_arch_attr_info): Replace 3 calls to
>         `riscv_merge_non_std_and_sv_ext' with single call to
>         `riscv_merge_multi_letter_ext'
>
>         * bfd/elfxx-riscv.c (riscv_parse_std_ext): Break if we
>         encounter a 'z' prefix.
>         (riscv_get_prefix_class): New function, return prefix class based
>         on first few characters of input string.
>         (riscv_parse_config): New structure to factor out minor differences
>         in extension class parsing behaviour.
>         (riscv_parse_sv_or_non_std_ext): Rename to...
>         (riscv_parse_prefixed_ext), and parameterise with
>         `riscv_parse_config`.
>         (riscv_std_z_ext_strtab):
>         (riscv_std_s_ext_strtab): String tables of known z/s extensions.
>         (riscv_parse_subset): Delegate all non-single-letter parsing work
>         to `riscv_parse_prefixed_ext'.
>
>         * bfd/elfxx-riscv.h (riscv_isa_ext_class): New type.

Missing entry for riscv_get_prefix_class :P


>
> gas/testsuite/ChangeLog:
>
>         * march-ok-s.d, march-ok-sx.d, march-ok-s-with-version: sx is no
>         longer valid and s exts must be known, so rename *ok* to *fail*.
>         * march-fail-s.l, march-fail-sx.l, march-fail-sx-with-version.l:
>         Expected error messages for above.
> ---
>  bfd/elfnn-riscv.c                             | 139 ++++++------
>  bfd/elfxx-riscv.c                             | 197 ++++++++++++++----
>  bfd/elfxx-riscv.h                             |  20 ++
>  .../gas/riscv/march-fail-s-with-version       |   2 +
>  ...-version.d => march-fail-s-with-version.d} |   1 +
>  .../gas/riscv/march-fail-s-with-version.l     |   2 +
>  .../riscv/{march-ok-s.d => march-fail-s.d}    |   1 +
>  gas/testsuite/gas/riscv/march-fail-s.l        |   2 +
>  .../riscv/{march-ok-sx.d => march-fail-sx.d}  |   3 +-
>  gas/testsuite/gas/riscv/march-fail-sx.l       |   2 +
>  10 files changed, 264 insertions(+), 105 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version
>  rename gas/testsuite/gas/riscv/{march-ok-s-with-version.d => march-fail-s-with-version.d} (68%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version.l
>  rename gas/testsuite/gas/riscv/{march-ok-s.d => march-fail-s.d} (75%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s.l
>  rename gas/testsuite/gas/riscv/{march-ok-sx.d => march-fail-sx.d} (56%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-sx.l
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 997f786602..82249fc88a 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2735,30 +2735,6 @@ riscv_std_ext_p (const char *name)
>    return (strlen (name) == 1) && (name[0] != 'x') && (name[0] != 's');
>  }
>
> -/* Predicator for non-standard extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 'x');
> -}
> -
> -/* Predicator for standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 's') && (name[1] != 'x');
> -}
> -
> -/* Predicator for non-standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 3) && (name[0] == 's') && (name[1] == 'x');
> -}
> -
>  /* Error handler when version mis-match.  */
>
>  static void
> @@ -2884,53 +2860,95 @@ riscv_merge_std_ext (bfd *ibfd,
>    return TRUE;
>  }
>
> -/* Merge non-standard and supervisor extensions.
> -   Return Value:
> -     Return FALSE if failed to merge.
> +static const char *
> +riscv_skip_prefix (const char *ext, riscv_isa_ext_class_t c)
> +{
> +  switch (c)
> +    {
> +    case RV_ISA_CLASS_X: return &ext[1];
> +    case RV_ISA_CLASS_S: return &ext[1];
> +    case RV_ISA_CLASS_Z: return &ext[1];
> +    default: return ext;
> +    }
> +}
>
> -   Arguments:
> -     `bfd`: bfd handler.
> -     `in_arch`: Raw arch string for input object.
> -     `out_arch`: Raw arch string for output object.
> -     `pin`: subset list for input object, and it'll skip all merged subset after
> -            merge.
> -     `pout`: Like `pin`, but for output object. */
> +/* Compare prefixed extension names canonically.  */
> +
> +static int
> +riscv_prefix_cmp (const char *a, const char *b)
> +{
> +  riscv_isa_ext_class_t ca = riscv_get_prefix_class (a);
> +  riscv_isa_ext_class_t cb = riscv_get_prefix_class (b);
> +
> +  /* Extension name without prefix  */
> +  const char *anp = riscv_skip_prefix (a, ca);
> +  const char *bnp = riscv_skip_prefix (b, cb);
> +
> +  if (ca == cb)
> +    return strcmp (anp, bnp);
All other place are using strcasecmp, so I think use strcasecmp here
would be more consistent.


> +
> +  return (int)ca - (int)cb;
> +}
>
>  static bfd_boolean
> -riscv_merge_non_std_and_sv_ext (bfd *ibfd,
> -                               riscv_subset_t **pin,
> -                               riscv_subset_t **pout,
> -                               bfd_boolean (*predicate_func) (const char *))
> +riscv_merge_multi_letter_ext (bfd *ibfd,
> +                             riscv_subset_t **pin,
> +                             riscv_subset_t **pout)
>  {
>    riscv_subset_t *in = *pin;
>    riscv_subset_t *out = *pout;
> +  riscv_subset_t *tail;
>
> -  for (in = *pin; in != NULL && predicate_func (in->name); in = in->next)
> -    riscv_add_subset (&merged_subsets, in->name, in->major_version,
> -                     in->minor_version);
> +  int cmp;
>
> -  for (out = *pout; out != NULL && predicate_func (out->name); out = out->next)
> +  while (in && out)
>      {
> -      riscv_subset_t *find_ext =
> -       riscv_lookup_subset (&merged_subsets, out->name);
> -      if (find_ext != NULL)
> +      cmp = riscv_prefix_cmp (in->name, out->name);
> +
> +      if (cmp < 0)
> +       {
> +         /* `in' comes before `out', append `in' and increment.  */
> +         riscv_add_subset (&merged_subsets, in->name, in->major_version,
> +                           in->minor_version);
> +         in = in->next;
> +       }
> +      else if (cmp > 0)
> +       {
> +         /* `out' comes before `in', append `out' and increment.  */
> +         riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +                           out->minor_version);
> +         out = out->next;
> +       }
> +      else
>         {
> -         /* Check version is same or not. */
> -         /* TODO: Allow different merge policy.  */
> -         if ((find_ext->major_version != out->major_version)
> -             || (find_ext->minor_version != out->minor_version))
> +         /* Both present, check version and increment both.  */
> +         if ((in->major_version != out->major_version)
> +             || (in->minor_version != out->minor_version))
>             {
> -             riscv_version_mismatch (ibfd, find_ext, out);
> +             riscv_version_mismatch (ibfd, in, out);
>               return FALSE;
>             }
> +
> +         riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +                           out->minor_version);
> +         out = out->next;
> +         in = in->next;
>         }
> -      else
> -       riscv_add_subset (&merged_subsets, out->name,
> -                         out->major_version, out->minor_version);
>      }
>
> -  *pin = in;
> -  *pout = out;
> +  if (in || out) {
> +    /* If we're here, either `in' or `out' is running longer than
> +       the other. So, we need to append the corresponding tail.  */
> +    tail = in ? in : out;
> +
> +    while (tail)
> +      {
> +       riscv_add_subset (&merged_subsets, tail->name, tail->major_version,
> +                         tail->minor_version);
> +       tail = tail->next;
> +      }
> +  }
> +
>    return TRUE;
>  }
>
> @@ -2989,14 +3007,9 @@ riscv_merge_arch_attr_info (bfd *ibfd, char *in_arch, char *out_arch)
>    /* Merge standard extension.  */
>    if (!riscv_merge_std_ext (ibfd, in_arch, out_arch, &in, &out))
>      return NULL;
> -  /* Merge non-standard extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_ext_p))
> -    return NULL;
> -  /* Merge standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_std_sv_ext_p))
> -    return NULL;
> -  /* Merge non-standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_sv_ext_p))
> +
> +  /* Merge all non-single letter extensions with single call.  */
> +  if (!riscv_merge_multi_letter_ext (ibfd, &in, &out))
>      return NULL;
>
>    if (xlen_in != xlen_out)
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 245717f70f..37f78e969b 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1193,7 +1193,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>      {
>        char subset[2] = {0, 0};
>
> -      if (*p == 'x' || *p == 's')
> +      if (*p == 'x' || *p == 's' || *p == 'z')
>         break;
>
>        if (*p == '_')
> @@ -1237,28 +1237,58 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> -/* Parsing function for non-standard and supervisor extensions.
> +/* Classify the argument 'arch' into one of riscv_isa_ext_class_t.  */
>
> -   Return Value:
> -     Points to the end of extensions.
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *arch)
> +{
> +  switch (*arch)
> +    {
> +    case 's':
> +      return RV_ISA_CLASS_S;
>
> -   Arguments:
> -     `rps`: Hooks and status for parsing subset.
> -     `march`: Full arch string.
> -     `p`: Curent parsing position.
> -     `ext_type`: What kind of extensions, 'x', 's' or 'sx'.
> -     `ext_type_str`: Full name for kind of extension.  */
> +    case 'x': return RV_ISA_CLASS_X;
> +    case 'z': return RV_ISA_CLASS_Z;
> +    default: return RV_ISA_CLASS_UNKNOWN;
> +    }
> +}
> +
> +/* Structure describing parameters to use when parsing a particular
> +   riscv_isa_ext_class_t. One of these should be provided for each
> +   possible class, except RV_ISA_CLASS_UNKNOWN.  */
> +
> +typedef struct riscv_parse_config
> +{
> +  /* Class of the extension. */
> +  riscv_isa_ext_class_t class;
> +
> +  /* Lower-case prefix string for error printing
> +     and internal parser usage, e.g. "z", "x".  */
> +  const char *prefix;
> +
> +  /* Predicate which is used for checking whether
> +     this is a "known" extension. For 'x',
> +     it always returns true (since they are by
> +     definition non-standard and cannot be known.  */
> +  bfd_boolean (*ext_valid_p) (const char *);
> +} riscv_parse_config_t;
> +
> +/* Parse a generic prefixed extension.
> +   march: The full architecture string as passed in by "-march=...".
> +   p: Point from which to start parsing the -march string.
> +   config: What class of extensions to parse, predicate funcs,
> +   and strings to use in error reporting.  */
>
>  static const char *
> -riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
> -                              const char *march,
> -                              const char *p,
> -                              const char *ext_type,
> -                              const char *ext_type_str)
> +riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> +                         const char *march,
> +                         const char *p,
> +                         const riscv_parse_config_t *config)
>  {
>    unsigned major_version = 0;
>    unsigned minor_version = 0;
> -  size_t ext_type_len = strlen (ext_type);
> +  const char *last_name;
> +  riscv_isa_ext_class_t class;
>
>    while (*p)
>      {
> @@ -1268,12 +1298,9 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>           continue;
>         }
>
> -      if (strncmp (p, ext_type, ext_type_len) != 0)
> -       break;
> -
> -      /* It's non-standard supervisor extension if it prefix with sx.  */
> -      if ((ext_type[0] == 's') && (ext_type_len == 1)
> -         && (*(p + 1) == 'x'))
> +      /* Assert that the current extension specifier matches our parsing class.  */
> +      class = riscv_get_prefix_class (p);
> +      if (class != config->class)
>         break;
>
>        char *subset = xstrdup (p);
> @@ -1294,6 +1321,43 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>
>        *q = '\0';
>
> +      /* Check that the name is valid.
> +        For 'x', anything goes but it cannot simply be 'x'.
> +        For 'z', it must be known from a list and also cannot simply be 'z'.
> +        For 's', it must be known from a list and also *can* simply be 's'.  */
> +
> +      /* Check that the extension name is well-formed.  */
> +      if (!config->ext_valid_p (subset))
> +       {
> +         rps->error_handler
> +           ("-march=%s: Invalid or unknown %s ISA extension: '%s'",
> +            march, config->prefix, subset);
> +         free (subset);
> +         return NULL;
> +       }
> +
> +      /* Check that the last item is not the same as this.  */
> +      last_name = rps->subset_list->tail->name;
> +
> +      if (!strcasecmp (last_name, subset))
> +       {
> +         rps->error_handler ("-march=%s: Duplicate %s ISA extension: \'%s\'",
> +                             march, config->prefix, subset);
> +         free (subset);
> +         return NULL;
> +       }
> +
> +      /* Check that we are in alphabetical order within the subset.  */
> +      if (!strncasecmp (last_name, config->prefix, 1)
> +         && strcasecmp (last_name, subset) > 0)
> +       {
> +         rps->error_handler ("-march=%s: %s ISA extension not in alphabetical order: "
> +                             "\'%s\' must come before \'%s\'.",
> +                             march, config->prefix, subset, last_name);
> +         free (subset);
> +         return NULL;
> +       }
> +
>        riscv_add_subset (rps->subset_list, subset, major_version, minor_version);
>        free (subset);
>        p += end_of_version - subset;
> @@ -1301,7 +1365,7 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>        if (*p != '\0' && *p != '_')
>         {
>           rps->error_handler ("-march=%s: %s must seperate with _",
> -                             march, ext_type_str);
> +                             march, config->prefix);
>           return NULL;
>         }
>      }
> @@ -1309,6 +1373,69 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> +const char * const riscv_std_z_ext_strtab[] =
Mark as static, and need comment for that.

> +  {
> +    NULL
> +  };
> +
> +const char * const riscv_std_s_ext_strtab[] =
Same as riscv_std_z_ext_strtab.

> +  {
> +    NULL
> +  };
> +
> +static bfd_boolean
> +riscv_multi_letter_ext_valid_p (const char *ext,
> +                               const char *const *known_exts)
> +{
> +  for (size_t i = 0; known_exts[i]; ++i)
> +    {
> +      if (!strcmp (ext, known_exts[i]))
> +       return TRUE;
> +    }
> +
> +  return FALSE;
> +}
> +
> +/* Predicator function for x-prefixed extensions.
> +   Anything goes, except the literal 'x'.  */
> +
> +static bfd_boolean
> +riscv_ext_x_valid_p (const char *arg)
> +{
> +  if (!strcasecmp (arg, "x"))
> +    return FALSE;
> +
> +  return TRUE;
> +}
> +
> +/* Predicator functions for z-prefixed extensions.
> +   Only known z-extensions are permitted.  */
> +
> +static bfd_boolean
> +riscv_ext_z_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_z_ext_strtab);
> +}
> +
> +/* Predicator function for 's' prefixed extensions.
> +   Must be either literal 's', or a known s-prefixed extension.  */
> +
> +static bfd_boolean
> +riscv_ext_s_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_s_ext_strtab);
> +}
> +
> +/* Parsing order that is needed for bitmanip.  */
> +
> +static const riscv_parse_config_t parse_config[] =
> +{
> +   {RV_ISA_CLASS_S, "s", riscv_ext_s_valid_p},
> +   {RV_ISA_CLASS_Z, "z", riscv_ext_z_valid_p},
> +   {RV_ISA_CLASS_X, "x", riscv_ext_x_valid_p},
> +   {RV_ISA_CLASS_UNKNOWN, NULL, NULL}
> +};
> +
>  /* Function for parsing arch string.
>
>     Return Value:
> @@ -1347,26 +1474,14 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
>    if (p == NULL)
>      return FALSE;
>
> -  /* Parsing non-standard extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "x", "non-standard extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> -
> -  /* Parsing supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "s", "supervisor extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> +  /* Parse the different classes of extensions in the specified order.  */
>
> -  /* Parsing non-standard supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> -       rps, arch, p, "sx", "non-standard supervisor extension");
> +  for (size_t i = 0; i < ARRAY_SIZE (parse_config); ++i) {
> +    p = riscv_parse_prefixed_ext (rps, arch, p, &parse_config[i]);
>
> -  if (p == NULL)
> -    return FALSE;
> +    if (p == NULL)
> +      return FALSE;
> +  }
>
>    if (*p != '\0')
>      {
> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> index 19f7bd2ecc..12ee7e3276 100644
> --- a/bfd/elfxx-riscv.h
> +++ b/bfd/elfxx-riscv.h
> @@ -86,3 +86,23 @@ riscv_release_subset_list (riscv_subset_list_t *);
>
>  extern char *
>  riscv_arch_str (unsigned, const riscv_subset_list_t *);
> +
> +extern const char * const z_ext_strtab[];
> +extern const char * const riscv_std_s_ext_strtab[];
Those two line can be removed, those two variable are no user out side
elfxx-riscv.c.

> +
> +/* ISA extension name class. E.g. "zbb" corresponds to RV_ISA_CLASS_Z,
> +   "xargs" corresponds to RV_ISA_CLASS_X, etc.  Order is important
> +   here.  */
> +
> +typedef enum riscv_isa_ext_class
> +  {
> +   RV_ISA_CLASS_S,
> +   RV_ISA_CLASS_Z,
> +   RV_ISA_CLASS_X,
> +   RV_ISA_CLASS_UNKNOWN
> +  } riscv_isa_ext_class_t;
> +
> +/* Classify the argument 'ext' into one of riscv_isa_ext_class_t.  */
> +
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *ext);
Argument name should omitted for function declaration.

> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version b/gas/testsuite/gas/riscv/march-fail-s-with-version
> new file mode 100644
> index 0000000000..a514d4aec7
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> \ No newline at end of file
> diff --git a/gas/testsuite/gas/riscv/march-ok-s-with-version.d b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> similarity index 68%
> rename from gas/testsuite/gas/riscv/march-ok-s-with-version.d
> rename to gas/testsuite/gas/riscv/march-fail-s-with-version.d
> index 6296a15c95..9881c2a0e0 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s-with-version.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo3p4
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s-with-version.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version.l b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-s.d b/gas/testsuite/gas/riscv/march-fail-s.d
> similarity index 75%
> rename from gas/testsuite/gas/riscv/march-ok-s.d
> rename to gas/testsuite/gas/riscv/march-fail-s.d
> index 7daa0a11d0..ebc8377aaf 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s.l b/gas/testsuite/gas/riscv/march-fail-s.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-sx.d b/gas/testsuite/gas/riscv/march-fail-sx.d
> similarity index 56%
> rename from gas/testsuite/gas/riscv/march-ok-sx.d
> rename to gas/testsuite/gas/riscv/march-fail-sx.d
> index e2172f2348..144a85c2fb 100644
> --- a/gas/testsuite/gas/riscv/march-ok-sx.d
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.d
> @@ -1,5 +1,6 @@
> -#as: -march=rv32isfoo_sxbar
> +#as: -march=rv32i_sxbar
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-sx.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-sx.l b/gas/testsuite/gas/riscv/march-fail-sx.l
> new file mode 100644
> index 0000000000..b8ead71a3b
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sxbar'
> --
> 2.17.1
>
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] RISC-V: Change -march parsing.

Maxim Blinov-2

mymsg (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Sourceware - binutils list mailing list
On Mon, 09 Dec 2019 09:10:47 PST (-0800), [hidden email] wrote:

> Thanks for reviewing Kito.
>
> On Mon, Dec 09, 2019 at 03:38:25PM +0800, Kito Cheng wrote:
>> Missing entry for riscv_get_prefix_class :P
> Added declaration to ChangeLog.
>
>> All other place are using strcasecmp, so I think use strcasecmp here
>> would be more consistent.
> Done.
>
>> Mark as static, and need comment for that.
> Done.
>
>> Same as riscv_std_z_ext_strtab.
> Done.
>
>> Those two line can be removed, those two variable are no user out side
>> elfxx-riscv.c.
> Lines removed.
>
>> Argument name should omitted for function declaration.
> Omitted.
>
> ---

git interprets the stuff you put above the first "---" as the commit text and
the stuff below that a comment.

> Change -march parsing logic so we can pass "z" prefixed arguments. The
> code is originally taken from the riscv-bitmanip branch.
>
> bfd/ChangeLog:
>
> * bfd/elfnn-riscv.c (riscv_skip_prefix): New function to skip
> extension prefixes.
> (riscv_non_stop_ext_p): Unused function removed.
> (riscv_std_sv_ext_p): Same
> (riscv_non_std_sv_ext_p): Same
> (riscv_merge_arch_attr_info): Replace 3 calls to
> `riscv_merge_non_std_and_sv_ext' with single call to
> `riscv_merge_multi_letter_ext'
>
> * bfd/elfxx-riscv.c (riscv_parse_std_ext): Break if we
> encounter a 'z' prefix.
> (riscv_get_prefix_class): New function, return prefix class based
> on first few characters of input string.
> (riscv_parse_config): New structure to factor out minor differences
> in extension class parsing behaviour.
> (riscv_parse_sv_or_non_std_ext): Rename to...
> (riscv_parse_prefixed_ext), and parameterise with
> `riscv_parse_config`.
> (riscv_std_z_ext_strtab):
> (riscv_std_s_ext_strtab): String tables of known z/s extensions.
> (riscv_parse_subset): Delegate all non-single-letter parsing work
> to `riscv_parse_prefixed_ext'.
>
> * bfd/elfxx-riscv.h (riscv_isa_ext_class): New type.
> (riscv_get_prefix_class): Declare.
>
> gas/testsuite/ChangeLog:
>
> * march-ok-s.d, march-ok-sx.d, march-ok-s-with-version: sx is no
> longer valid and s exts must be known, so rename *ok* to *fail*.
> * march-fail-s.l, march-fail-sx.l, march-fail-sx-with-version.l:
> Expected error messages for above.
> ---
>  bfd/elfnn-riscv.c                             | 139 ++++++------
>  bfd/elfxx-riscv.c                             | 209 ++++++++++++++----
>  bfd/elfxx-riscv.h                             |  17 ++
>  .../gas/riscv/march-fail-s-with-version       |   2 +
>  ...-version.d => march-fail-s-with-version.d} |   1 +
>  .../gas/riscv/march-fail-s-with-version.l     |   2 +
>  .../riscv/{march-ok-s.d => march-fail-s.d}    |   1 +
>  gas/testsuite/gas/riscv/march-fail-s.l        |   2 +
>  .../riscv/{march-ok-sx.d => march-fail-sx.d}  |   3 +-
>  gas/testsuite/gas/riscv/march-fail-sx.l       |   2 +
>  10 files changed, 273 insertions(+), 105 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version
>  rename gas/testsuite/gas/riscv/{march-ok-s-with-version.d => march-fail-s-with-version.d} (68%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s-with-version.l
>  rename gas/testsuite/gas/riscv/{march-ok-s.d => march-fail-s.d} (75%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-s.l
>  rename gas/testsuite/gas/riscv/{march-ok-sx.d => march-fail-sx.d} (56%)
>  create mode 100644 gas/testsuite/gas/riscv/march-fail-sx.l
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 997f786602..138836b879 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2735,30 +2735,6 @@ riscv_std_ext_p (const char *name)
>    return (strlen (name) == 1) && (name[0] != 'x') && (name[0] != 's');
>  }
>
> -/* Predicator for non-standard extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 'x');
> -}
> -
> -/* Predicator for standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 2) && (name[0] == 's') && (name[1] != 'x');
> -}
> -
> -/* Predicator for non-standard supervisor extension.  */
> -
> -static bfd_boolean
> -riscv_non_std_sv_ext_p (const char *name)
> -{
> -  return (strlen (name) >= 3) && (name[0] == 's') && (name[1] == 'x');
> -}
> -
>  /* Error handler when version mis-match.  */
>
>  static void
> @@ -2884,53 +2860,95 @@ riscv_merge_std_ext (bfd *ibfd,
>    return TRUE;
>  }
>
> -/* Merge non-standard and supervisor extensions.
> -   Return Value:
> -     Return FALSE if failed to merge.
> +static const char *
> +riscv_skip_prefix (const char *ext, riscv_isa_ext_class_t c)
> +{
> +  switch (c)
> +    {
> +    case RV_ISA_CLASS_X: return &ext[1];
> +    case RV_ISA_CLASS_S: return &ext[1];
> +    case RV_ISA_CLASS_Z: return &ext[1];
> +    default: return ext;
> +    }
> +}
>
> -   Arguments:
> -     `bfd`: bfd handler.
> -     `in_arch`: Raw arch string for input object.
> -     `out_arch`: Raw arch string for output object.
> -     `pin`: subset list for input object, and it'll skip all merged subset after
> -            merge.
> -     `pout`: Like `pin`, but for output object. */
> +/* Compare prefixed extension names canonically.  */
> +
> +static int
> +riscv_prefix_cmp (const char *a, const char *b)
> +{
> +  riscv_isa_ext_class_t ca = riscv_get_prefix_class (a);
> +  riscv_isa_ext_class_t cb = riscv_get_prefix_class (b);
> +
> +  /* Extension name without prefix  */
> +  const char *anp = riscv_skip_prefix (a, ca);
> +  const char *bnp = riscv_skip_prefix (b, cb);
> +
> +  if (ca == cb)
> +    return strcasecmp (anp, bnp);
> +
> +  return (int)ca - (int)cb;
> +}
>
>  static bfd_boolean
> -riscv_merge_non_std_and_sv_ext (bfd *ibfd,
> - riscv_subset_t **pin,
> - riscv_subset_t **pout,
> - bfd_boolean (*predicate_func) (const char *))
> +riscv_merge_multi_letter_ext (bfd *ibfd,
> +      riscv_subset_t **pin,
> +      riscv_subset_t **pout)
>  {
>    riscv_subset_t *in = *pin;
>    riscv_subset_t *out = *pout;
> +  riscv_subset_t *tail;
>
> -  for (in = *pin; in != NULL && predicate_func (in->name); in = in->next)
> -    riscv_add_subset (&merged_subsets, in->name, in->major_version,
> -      in->minor_version);
> +  int cmp;
>
> -  for (out = *pout; out != NULL && predicate_func (out->name); out = out->next)
> +  while (in && out)
>      {
> -      riscv_subset_t *find_ext =
> - riscv_lookup_subset (&merged_subsets, out->name);
> -      if (find_ext != NULL)
> +      cmp = riscv_prefix_cmp (in->name, out->name);
> +
> +      if (cmp < 0)
> + {
> +  /* `in' comes before `out', append `in' and increment.  */
> +  riscv_add_subset (&merged_subsets, in->name, in->major_version,
> +    in->minor_version);
> +  in = in->next;
> + }
> +      else if (cmp > 0)
> + {
> +  /* `out' comes before `in', append `out' and increment.  */
> +  riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +    out->minor_version);
> +  out = out->next;
> + }
> +      else
>   {
> -  /* Check version is same or not. */
> -  /* TODO: Allow different merge policy.  */
> -  if ((find_ext->major_version != out->major_version)
> -      || (find_ext->minor_version != out->minor_version))
> +  /* Both present, check version and increment both.  */
> +  if ((in->major_version != out->major_version)
> +      || (in->minor_version != out->minor_version))
>      {
> -      riscv_version_mismatch (ibfd, find_ext, out);
> +      riscv_version_mismatch (ibfd, in, out);
>        return FALSE;
>      }
> +
> +  riscv_add_subset (&merged_subsets, out->name, out->major_version,
> +    out->minor_version);
> +  out = out->next;
> +  in = in->next;
>   }
> -      else
> - riscv_add_subset (&merged_subsets, out->name,
> -  out->major_version, out->minor_version);
>      }
>
> -  *pin = in;
> -  *pout = out;
> +  if (in || out) {
> +    /* If we're here, either `in' or `out' is running longer than
> +       the other. So, we need to append the corresponding tail.  */
> +    tail = in ? in : out;
> +
> +    while (tail)
> +      {
> + riscv_add_subset (&merged_subsets, tail->name, tail->major_version,
> +  tail->minor_version);
> + tail = tail->next;
> +      }
> +  }
> +
>    return TRUE;
>  }
>
> @@ -2989,14 +3007,9 @@ riscv_merge_arch_attr_info (bfd *ibfd, char *in_arch, char *out_arch)
>    /* Merge standard extension.  */
>    if (!riscv_merge_std_ext (ibfd, in_arch, out_arch, &in, &out))
>      return NULL;
> -  /* Merge non-standard extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_ext_p))
> -    return NULL;
> -  /* Merge standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_std_sv_ext_p))
> -    return NULL;
> -  /* Merge non-standard supervisor extension.  */
> -  if (!riscv_merge_non_std_and_sv_ext (ibfd, &in, &out, riscv_non_std_sv_ext_p))
> +
> +  /* Merge all non-single letter extensions with single call.  */
> +  if (!riscv_merge_multi_letter_ext (ibfd, &in, &out))
>      return NULL;
>
>    if (xlen_in != xlen_out)
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 245717f70f..fffbc4a92f 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -1193,7 +1193,7 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>      {
>        char subset[2] = {0, 0};
>
> -      if (*p == 'x' || *p == 's')
> +      if (*p == 'x' || *p == 's' || *p == 'z')
>   break;
>
>        if (*p == '_')
> @@ -1237,28 +1237,58 @@ riscv_parse_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> -/* Parsing function for non-standard and supervisor extensions.
> +/* Classify the argument 'arch' into one of riscv_isa_ext_class_t.  */
>
> -   Return Value:
> -     Points to the end of extensions.
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *arch)
> +{
> +  switch (*arch)
> +    {
> +    case 's':
> +      return RV_ISA_CLASS_S;
>
> -   Arguments:
> -     `rps`: Hooks and status for parsing subset.
> -     `march`: Full arch string.
> -     `p`: Curent parsing position.
> -     `ext_type`: What kind of extensions, 'x', 's' or 'sx'.
> -     `ext_type_str`: Full name for kind of extension.  */
> +    case 'x': return RV_ISA_CLASS_X;
> +    case 'z': return RV_ISA_CLASS_Z;
> +    default: return RV_ISA_CLASS_UNKNOWN;
> +    }
> +}
> +
> +/* Structure describing parameters to use when parsing a particular
> +   riscv_isa_ext_class_t. One of these should be provided for each
> +   possible class, except RV_ISA_CLASS_UNKNOWN.  */
> +
> +typedef struct riscv_parse_config
> +{
> +  /* Class of the extension. */
> +  riscv_isa_ext_class_t class;
> +
> +  /* Lower-case prefix string for error printing
> +     and internal parser usage, e.g. "z", "x".  */
> +  const char *prefix;
> +
> +  /* Predicate which is used for checking whether
> +     this is a "known" extension. For 'x',
> +     it always returns true (since they are by
> +     definition non-standard and cannot be known.  */
> +  bfd_boolean (*ext_valid_p) (const char *);
> +} riscv_parse_config_t;
> +
> +/* Parse a generic prefixed extension.
> +   march: The full architecture string as passed in by "-march=...".
> +   p: Point from which to start parsing the -march string.
> +   config: What class of extensions to parse, predicate funcs,
> +   and strings to use in error reporting.  */
>
>  static const char *
> -riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
> -       const char *march,
> -       const char *p,
> -       const char *ext_type,
> -       const char *ext_type_str)
> +riscv_parse_prefixed_ext (riscv_parse_subset_t *rps,
> +  const char *march,
> +  const char *p,
> +  const riscv_parse_config_t *config)
>  {
>    unsigned major_version = 0;
>    unsigned minor_version = 0;
> -  size_t ext_type_len = strlen (ext_type);
> +  const char *last_name;
> +  riscv_isa_ext_class_t class;
>
>    while (*p)
>      {
> @@ -1268,12 +1298,9 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>    continue;
>   }
>
> -      if (strncmp (p, ext_type, ext_type_len) != 0)
> - break;
> -
> -      /* It's non-standard supervisor extension if it prefix with sx.  */
> -      if ((ext_type[0] == 's') && (ext_type_len == 1)
> -  && (*(p + 1) == 'x'))
> +      /* Assert that the current extension specifier matches our parsing class.  */
> +      class = riscv_get_prefix_class (p);
> +      if (class != config->class)
>   break;
>
>        char *subset = xstrdup (p);
> @@ -1294,6 +1321,43 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>
>        *q = '\0';
>
> +      /* Check that the name is valid.
> + For 'x', anything goes but it cannot simply be 'x'.
> + For 'z', it must be known from a list and also cannot simply be 'z'.
> + For 's', it must be known from a list and also *can* simply be 's'.  */
> +
> +      /* Check that the extension name is well-formed.  */
> +      if (!config->ext_valid_p (subset))
> + {
> +  rps->error_handler
> +    ("-march=%s: Invalid or unknown %s ISA extension: '%s'",
> +     march, config->prefix, subset);
> +  free (subset);
> +  return NULL;
> + }
> +
> +      /* Check that the last item is not the same as this.  */
> +      last_name = rps->subset_list->tail->name;
> +
> +      if (!strcasecmp (last_name, subset))
> + {
> +  rps->error_handler ("-march=%s: Duplicate %s ISA extension: \'%s\'",
> +      march, config->prefix, subset);
> +  free (subset);
> +  return NULL;
> + }
> +
> +      /* Check that we are in alphabetical order within the subset.  */
> +      if (!strncasecmp (last_name, config->prefix, 1)
> +  && strcasecmp (last_name, subset) > 0)
> + {
> +  rps->error_handler ("-march=%s: %s ISA extension not in alphabetical order: "
> +      "\'%s\' must come before \'%s\'.",
> +      march, config->prefix, subset, last_name);
> +  free (subset);
> +  return NULL;
> + }
> +
>        riscv_add_subset (rps->subset_list, subset, major_version, minor_version);
>        free (subset);
>        p += end_of_version - subset;
> @@ -1301,7 +1365,7 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>        if (*p != '\0' && *p != '_')
>   {
>    rps->error_handler ("-march=%s: %s must seperate with _",
> -      march, ext_type_str);
> +      march, config->prefix);
>    return NULL;
>   }
>      }
> @@ -1309,6 +1373,81 @@ riscv_parse_sv_or_non_std_ext (riscv_parse_subset_t *rps,
>    return p;
>  }
>
> +/* List of Z-class extensions that binutils should know about.
> +   Whether or not a particular entry is in this list will
> +   dictate if gas/ld will accept its presence in the -march
> +   string.
> +
> +   Example: To add an extension called "Zbb" (bitmanip base extension),
> +   add "zbb" string to the list (all lowercase).
> +
> +   Keep this list alphabetically ordered.  */
> +
> +static const char * const riscv_std_z_ext_strtab[] =
> +  {
> +    NULL
> +  };

There are a bunch of defined Z extensions that we could use now as the proof of
concept -- Zifencei is the one I'd go for, as that'd actually get some use.

> +
> +/* Same as `riscv_std_z_ext_strtab', but for S-class extensions.  */
> +
> +static const char * const riscv_std_s_ext_strtab[] =
> +  {
> +    NULL
> +  };
> +
> +static bfd_boolean
> +riscv_multi_letter_ext_valid_p (const char *ext,
> + const char *const *known_exts)
> +{
> +  for (size_t i = 0; known_exts[i]; ++i)
> +    {
> +      if (!strcmp (ext, known_exts[i]))
> + return TRUE;
> +    }
> +
> +  return FALSE;
> +}
> +
> +/* Predicator function for x-prefixed extensions.
> +   Anything goes, except the literal 'x'.  */
> +
> +static bfd_boolean
> +riscv_ext_x_valid_p (const char *arg)
> +{
> +  if (!strcasecmp (arg, "x"))
> +    return FALSE;
> +
> +  return TRUE;

This allows any X extension through but then goes ahead to not provide any
support for those extensions.  I can understand providing some scaffolding to
users to turn on their custom extensions, but we should fail quickly on any
extensions that aren't supported.

> +}
> +
> +/* Predicator functions for z-prefixed extensions.
> +   Only known z-extensions are permitted.  */
> +
> +static bfd_boolean
> +riscv_ext_z_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_z_ext_strtab);
> +}
> +
> +/* Predicator function for 's' prefixed extensions.
> +   Must be either literal 's', or a known s-prefixed extension.  */
> +
> +static bfd_boolean
> +riscv_ext_s_valid_p (const char *arg)
> +{
> +  return riscv_multi_letter_ext_valid_p (arg, riscv_std_s_ext_strtab);
> +}
> +
> +/* Parsing order that is needed for bitmanip.  */
> +
> +static const riscv_parse_config_t parse_config[] =
> +{
> +   {RV_ISA_CLASS_S, "s", riscv_ext_s_valid_p},
> +   {RV_ISA_CLASS_Z, "z", riscv_ext_z_valid_p},
> +   {RV_ISA_CLASS_X, "x", riscv_ext_x_valid_p},
> +   {RV_ISA_CLASS_UNKNOWN, NULL, NULL}
> +};
> +
>  /* Function for parsing arch string.
>
>     Return Value:
> @@ -1347,26 +1486,14 @@ riscv_parse_subset (riscv_parse_subset_t *rps,
>    if (p == NULL)
>      return FALSE;
>
> -  /* Parsing non-standard extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> - rps, arch, p, "x", "non-standard extension");
> +  /* Parse the different classes of extensions in the specified order.  */
>
> -  if (p == NULL)
> -    return FALSE;
> +  for (size_t i = 0; i < ARRAY_SIZE (parse_config); ++i) {
> +    p = riscv_parse_prefixed_ext (rps, arch, p, &parse_config[i]);
>
> -  /* Parsing supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> - rps, arch, p, "s", "supervisor extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> -
> -  /* Parsing non-standard supervisor extension.  */
> -  p = riscv_parse_sv_or_non_std_ext (
> - rps, arch, p, "sx", "non-standard supervisor extension");
> -
> -  if (p == NULL)
> -    return FALSE;
> +    if (p == NULL)
> +      return FALSE;
> +  }
>
>    if (*p != '\0')
>      {
> diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
> index 19f7bd2ecc..113132897b 100644
> --- a/bfd/elfxx-riscv.h
> +++ b/bfd/elfxx-riscv.h
> @@ -86,3 +86,20 @@ riscv_release_subset_list (riscv_subset_list_t *);
>
>  extern char *
>  riscv_arch_str (unsigned, const riscv_subset_list_t *);
> +
> +/* ISA extension name class. E.g. "zbb" corresponds to RV_ISA_CLASS_Z,
> +   "xargs" corresponds to RV_ISA_CLASS_X, etc.  Order is important
> +   here.  */
> +
> +typedef enum riscv_isa_ext_class
> +  {
> +   RV_ISA_CLASS_S,
> +   RV_ISA_CLASS_Z,
> +   RV_ISA_CLASS_X,
> +   RV_ISA_CLASS_UNKNOWN
> +  } riscv_isa_ext_class_t;
> +
> +/* Classify the argument 'ext' into one of riscv_isa_ext_class_t.  */
> +
> +riscv_isa_ext_class_t
> +riscv_get_prefix_class (const char *);
> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version b/gas/testsuite/gas/riscv/march-fail-s-with-version
> new file mode 100644
> index 0000000000..a514d4aec7
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> \ No newline at end of file
> diff --git a/gas/testsuite/gas/riscv/march-ok-s-with-version.d b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> similarity index 68%
> rename from gas/testsuite/gas/riscv/march-ok-s-with-version.d
> rename to gas/testsuite/gas/riscv/march-fail-s-with-version.d
> index 6296a15c95..9881c2a0e0 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s-with-version.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo3p4
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s-with-version.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s-with-version.l b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s-with-version.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-s.d b/gas/testsuite/gas/riscv/march-fail-s.d
> similarity index 75%
> rename from gas/testsuite/gas/riscv/march-ok-s.d
> rename to gas/testsuite/gas/riscv/march-fail-s.d
> index 7daa0a11d0..ebc8377aaf 100644
> --- a/gas/testsuite/gas/riscv/march-ok-s.d
> +++ b/gas/testsuite/gas/riscv/march-fail-s.d
> @@ -1,5 +1,6 @@
>  #as: -march=rv32isfoo
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-s.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-s.l b/gas/testsuite/gas/riscv/march-fail-s.l
> new file mode 100644
> index 0000000000..6b1f957276
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-s.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sfoo'
> diff --git a/gas/testsuite/gas/riscv/march-ok-sx.d b/gas/testsuite/gas/riscv/march-fail-sx.d
> similarity index 56%
> rename from gas/testsuite/gas/riscv/march-ok-sx.d
> rename to gas/testsuite/gas/riscv/march-fail-sx.d
> index e2172f2348..144a85c2fb 100644
> --- a/gas/testsuite/gas/riscv/march-ok-sx.d
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.d
> @@ -1,5 +1,6 @@
> -#as: -march=rv32isfoo_sxbar
> +#as: -march=rv32i_sxbar
>  #objdump: -dr
>  #source: empty.s
> +#error_output: march-fail-sx.l
>
>  .*:     file format elf32-littleriscv
> diff --git a/gas/testsuite/gas/riscv/march-fail-sx.l b/gas/testsuite/gas/riscv/march-fail-sx.l
> new file mode 100644
> index 0000000000..b8ead71a3b
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/march-fail-sx.l
> @@ -0,0 +1,2 @@
> +Assembler messages:
> +.*: Invalid or unknown s ISA extension: 'sxbar'

Otherwise looks good, thanks!
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Kito Cheng-2
Hi Maxim:

LGTM, just need Palmer or Jim to approve :)

Hi Palmer:

>> +static bfd_boolean
>> +riscv_ext_x_valid_p (const char *arg)
>> +{
>> +  if (!strcasecmp (arg, "x"))
>> +    return FALSE;
>> +
>> +  return TRUE;
>
> This allows any X extension through but then goes ahead to not provide any
> support for those extensions.  I can understand providing some scaffolding to
> users to turn on their custom extensions, but we should fail quickly on any
> extensions that aren't supported.

We bypass any X extension now, and one concern is we can't testing any
X parsing/combine logic if we reject that.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Kito Cheng-2
Hi Palmer:

Ping, do you mind accept this patch and then we'll send follow up
patch for Zifence and Zicsr?

On Mon, Dec 23, 2019 at 3:21 PM Kito Cheng <[hidden email]> wrote:

>
> Hi Maxim:
>
> LGTM, just need Palmer or Jim to approve :)
>
> Hi Palmer:
>
> >> +static bfd_boolean
> >> +riscv_ext_x_valid_p (const char *arg)
> >> +{
> >> +  if (!strcasecmp (arg, "x"))
> >> +    return FALSE;
> >> +
> >> +  return TRUE;
> >
> > This allows any X extension through but then goes ahead to not provide any
> > support for those extensions.  I can understand providing some scaffolding to
> > users to turn on their custom extensions, but we should fail quickly on any
> > extensions that aren't supported.
>
> We bypass any X extension now, and one concern is we can't testing any
> X parsing/combine logic if we reject that.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Jim Wilson-2
On Wed, Jan 8, 2020 at 10:20 PM Kito Cheng <[hidden email]> wrote:
> Ping, do you mind accept this patch and then we'll send follow up
> patch for Zifence and Zicsr?

This is at the top of my list now and I will be looking at it today.
I'd like to get this patch in before the next binutils release as this
problem has been pending for too long.

Jim
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Sourceware - binutils list mailing list
In reply to this post by Kito Cheng-2
On Wed, 08 Jan 2020 22:20:36 PST (-0800), Kito Cheng wrote:
> Hi Palmer:
>
> Ping, do you mind accept this patch and then we'll send follow up
> patch for Zifence and Zicsr?

Sorry, I must have missed the original email

>
> On Mon, Dec 23, 2019 at 3:21 PM Kito Cheng <[hidden email]> wrote:
>>
>> Hi Maxim:
>>
>> LGTM, just need Palmer or Jim to approve :)
>>
>> Hi Palmer:
>>
>> >> +static bfd_boolean
>> >> +riscv_ext_x_valid_p (const char *arg)
>> >> +{
>> >> +  if (!strcasecmp (arg, "x"))
>> >> +    return FALSE;
>> >> +
>> >> +  return TRUE;
>> >
>> > This allows any X extension through but then goes ahead to not provide any
>> > support for those extensions.  I can understand providing some scaffolding to
>> > users to turn on their custom extensions, but we should fail quickly on any
>> > extensions that aren't supported.
>>
>> We bypass any X extension now, and one concern is we can't testing any
>> X parsing/combine logic if we reject that.

I still don't think it's a good idea to allow any X extension through.  If you
really want to test X extension handling then we should define some X extension
that's unlikely to have a name conflict and have that extension do something --
maybe "Xbinutils_example_extension", which only has some sort of
"Xbinutils_example_instruction" instruction in it.  Without that then you're
not really testing X extension parsing, as there won't be any behavior change
as a result of passing in the X extension so you can't tell if it's actually
enabled.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] RISC-V: Change -march parsing.

Jim Wilson-2
In reply to this post by Jim Wilson-2
On Thu, Jan 9, 2020 at 11:40 AM Jim Wilson <[hidden email]> wrote:
> On Wed, Jan 8, 2020 at 10:20 PM Kito Cheng <[hidden email]> wrote:
> > Ping, do you mind accept this patch and then we'll send follow up
> > patch for Zifence and Zicsr?

I finally got back to this for real.

I see a curious comment
+        For 's', it must be known from a list and also *can* simply be 's'.  *\
except the code does not allow s by itself, and I don't see anything
in the ISA spec that allows s by itself.  It isn't obvious what the
intent here was, so I left this alone for now, but I consider this a
bug that needs to be fixed, either by fixing the code to match the
comment, or the comment to match the code.  I suspect the latter is
the correct fix.  Maybe this is left over from when we still supported
sx?

I also noticed that there are now H* extensions documented in the ISA
manual, but I think these were added after this discussion started, so
this can be fixed later.  It should be easy to add H support same as
s/x/z support.

Otherwise this looks OK to me.

I noticed 3 functions were missing comments; I added them.  I fixed a
couple of lines that were too long.  There was a comment
+/* Parsing order that is needed for bitmanip.  */
I changed it to
/* Parsing order that is specified by the ISA manual.  */
I rewrote the ChangeLog entries.  The gas entries were written for the
wrong directory, as gas testsuite ChangeLog entries go in the toplevel
gas dir.  The bfd entries were missing some of the function changes,
had unnecessary quotes around function names, and had a few typos.
None of these changes affect how the patch works.

I'm approving the patch, and committed my slightly modified version of it.

Jim