[PATCH] RISC-V: Change -march parsing.
Kito Cheng
kito.cheng@gmail.com
Mon Dec 9 07:38:00 GMT 2019
Hi Maxim:
Thanks your patch, it's LGTM, just some minor review comment :)
On Wed, Dec 4, 2019 at 7:34 PM Maxim Blinov <maxim.blinov@embecosm.com> 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
>
More information about the Binutils
mailing list