[PATCH] [ARC] Fix handling of cpu=... disassembler option value

[PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

There is a bug in handling of cpu=... disassembler option in case there are
other options after it, for example, `cpu=EM,dsp'.  In this case `EM,dsp' is
treated as an option value, and strcasecmp reports is as non-equal to "EM".
This could have been fixed by using strncasecmp and passing length of
cpu_types[i].name as size argument, or using strcasestr, but that would cause
another problem as value `em4,dsp' would be equal to `em' cpy_type.name in
those cases.  Therefore the right solution is to extract value of cpu option
fully and pass it as an argument to parse_cpu_option.

opcodes/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-dis.c (parse_disassembler_options): Properly handle cpu=...
	  option when there is an option after it.
---
 opcodes/arc-dis.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index edd0c07..0121682 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -844,8 +844,12 @@ parse_disassembler_options (const char *options)
       /* A CPU option?  Cannot use STRING_COMMA_LEN because strncmp is also a
 	 preprocessor macro.  */
       if (strncmp (options, "cpu=", 4) == 0)
-	/* Strip leading `cpu=`.  */
-	enforced_isa_mask = parse_cpu_option (options + 4);
+	{
+	  /* Strip leading `cpu=`.  */
+	  char *options_copy = strdup (options + 4);
+	  enforced_isa_mask = parse_cpu_option (strtok (options_copy, ","));
+	  free (options_copy);
+	}
       else
 	parse_option (options);
 
-- 
2.8.3

Re: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Pedro Alves]

On 06/15/2017 03:59 PM, Anton Kolesov wrote:
> There is a bug in handling of cpu=... disassembler option in case there are
> other options after it, for example, `cpu=EM,dsp'.  In this case `EM,dsp' is
> treated as an option value, and strcasecmp reports is as non-equal to "EM".
> This could have been fixed by using strncasecmp and passing length of
> cpu_types[i].name as size argument, or using strcasestr, but that would cause
> another problem as value `em4,dsp' would be equal to `em' cpy_type.name in
> those cases.  Therefore the right solution is to extract value of cpu option
> fully and pass it as an argument to parse_cpu_option.

Couldn't this use disassembler_options_cmp / FOR_EACH_DISASSEMBLER_OPTION?
See e.g. ppc-dis.c:ppc_parse_cpu for example.

Thanks,
Pedro Alves

RE: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

Hi Pedro,

> On 06/15/2017 03:59 PM, Anton Kolesov wrote:
> > There is a bug in handling of cpu=... disassembler option in case
> > there are other options after it, for example, `cpu=EM,dsp'.  In this
> > case `EM,dsp' is treated as an option value, and strcasecmp reports is as
> non-equal to "EM".
> > This could have been fixed by using strncasecmp and passing length of
> > cpu_types[i].name as size argument, or using strcasestr, but that
> > would cause another problem as value `em4,dsp' would be equal to `em'
> > cpy_type.name in those cases.  Therefore the right solution is to
> > extract value of cpu option fully and pass it as an argument to
> parse_cpu_option.
>
> Couldn't this use disassembler_options_cmp /
> FOR_EACH_DISASSEMBLER_OPTION?
> See e.g. ppc-dis.c:ppc_parse_cpu for example.

FOR_EACH_DISASSEMBLER_OPTION advances pointer to the beginning of next option,
but it doesn't create a new string with \0 at the end. So, for example, this
code:

  const char* option;
  FOR_EACH_DISASSEMBLER_OPTION (option, "fpuda,fpud,fpus")
    {
      printf ("option: %s\n", option);
    }

will print:

  option: fpuda,fpud,fpus
  option: fpud,fpus
  option: fpus

As a result this macro can be used to improve existing code in arc-dis.c,
however it doesn't address the issue that this particular patch is trying to
fix. I can make a separate patch that would change ARC code to use
FOR_EACH_DISASSEMBLER_OPTION.

Anton


>
> Thanks,
> Pedro Alves

Re: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Pedro Alves]

On 06/16/2017 12:44 PM, Anton Kolesov wrote:

>> Couldn't this use disassembler_options_cmp /
>> FOR_EACH_DISASSEMBLER_OPTION?
>> See e.g. ppc-dis.c:ppc_parse_cpu for example.
> 
> FOR_EACH_DISASSEMBLER_OPTION advances pointer to the beginning of next option,
> but it doesn't create a new string with \0 at the end. So, for example, this
> code:
> 
>   const char* option;
>   FOR_EACH_DISASSEMBLER_OPTION (option, "fpuda,fpud,fpus")
>     {
>       printf ("option: %s\n", option);
>     }
> 
> will print:
> 
>   option: fpuda,fpud,fpus
>   option: fpud,fpus
>   option: fpus
> 
> As a result this macro can be used to improve existing code in arc-dis.c,
> however it doesn't address the issue that this particular patch is trying to
> fix. 

Hmm, the description you originally send only talked about parsing the
option value incorrectly.  If you change arc-dis.c:parse_cpu_option
to use disassembler_options_cmp for comparing options, then I think
it should fix the original issue you described?  Guess the issue then would
be that disassembler_options_cmp is case sensitive.  I wonder whether
architectures really want to be different here..

Oh well.

IMO, it's arguable whether to consider printing the whole string starting at
what failed to be parsed as a bug.  I find it fine.  ppc-dis.c:powerpc_init_dialect
also prints the whole string starting from the invalid option, AFAICS.
(And GDB does that in many other cases too.)

Actually I find it a bug that this code even prints to stderr directly
in the first place.  That'll be the wrong thing to do when this code
is being called by GDB -- a gdb Python script can disassemble with output
redirected to a string, for example.

Thanks,
Pedro Alves

Re: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Peter Bergner]

On 6/16/17 6:44 AM, Anton Kolesov wrote:
> FOR_EACH_DISASSEMBLER_OPTION advances pointer to the beginning of next option,
> but it doesn't create a new string with \0 at the end. So, for example, this
> code:
> 
>   const char* option;
>   FOR_EACH_DISASSEMBLER_OPTION (option, "fpuda,fpud,fpus")
>     {
>       printf ("option: %s\n", option);
>     }

That is true, but disassembler_options_cmp() treats ',' the same as '\0'
so disassembler_options_cmp ("fpuda", "fpuda,fpud,fpus") will return
that the strings match.  That means you don't have to copy the next
option into a separate string just so you can terminate the string with
\0 before using strcmp().  Like ARC, ppc has problems using strncmp due
to multiple options having the same prefix string.  So a loop like:

    const char* option;
    FOR_EACH_DISASSEMBLER_OPTION (option, "fpuda,fpud,fpus")
      {
        if (disassembler_options_cmp (option, "fpuda") == 0)
          printf ("I found 'fpuda'\n");
        else if (disassembler_options_cmp (option, "fpud") == 0)
          printf ("I found 'fpud'\n");
        else if (disassembler_options_cmp (option, "fpus") == 0)
          printf ("I found 'fpus'\n");
      }

will print:

  I found 'fpuda'
  I found 'fpud'
  I found 'fpus'

Peter

[PATCH v2 2/2] [ARC] Use FOR_EACH_DISASSEMBLER_OPTION to iterate over options

[Anton Kolesov]

This patch updates arc-dis.c:parse_disassembler_options to use a macro
FOR_EACH_DISASSEMBLER_OPTION, which has been introduced in [1], instead of a
homegrown solution to split option string.

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=65b48a81

opcodes/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	arc-dis.c (parse_disassembler_options): Use
	FOR_EACH_DISASSEMBLER_OPTION.
---
 opcodes/arc-dis.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index dc9f5d7..c1faf0e 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -823,6 +823,8 @@ parse_cpu_option (const char *option)
 static void
 parse_disassembler_options (const char *options)
 {
+  const char *option;
+
   if (options == NULL)
     return;
 
@@ -832,25 +834,15 @@ parse_disassembler_options (const char *options)
      CPU when new options are being parsed.  */
   enforced_isa_mask = ARC_OPCODE_NONE;
 
-  while (*options)
+  FOR_EACH_DISASSEMBLER_OPTION (option, options)
     {
-      /* Skip empty options.  */
-      if (*options == ',')
-	{
-	  ++ options;
-	  continue;
-	}
-
       /* A CPU option?  Cannot use STRING_COMMA_LEN because strncmp is also a
 	 preprocessor macro.  */
-      if (strncmp (options, "cpu=", 4) == 0)
+      if (strncmp (option, "cpu=", 4) == 0)
 	/* Strip leading `cpu=`.  */
-	enforced_isa_mask = parse_cpu_option (options + 4);
+	enforced_isa_mask = parse_cpu_option (option + 4);
       else
-	parse_option (options);
-
-      while (*options != ',' && *options != '\0')
-	++ options;
+	parse_option (option);
     }
 }
 
-- 
2.8.3

[PATCH v2 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

Changes in V2:

* Use disassembler_options_cmp to compare string instead of using strtok, which
  required string duplication.
* Add a second patch that introduces usage of FOR_EACH_DISASSEMBLER_OPTION to
  ARC.

---

There is a bug in handling of cpu=... disassembler option in case there are
other options after it, for example, `cpu=EM,dsp'.  In this case `EM,dsp' is
treated as an option value, and strcasecmp reports is as non-equal to "EM".
This is fixed by using disassembler_options_cmp function, which compares string
treating `,' the same way as `\0'.

This function also solves a problem with option order in parse_option.
Previously, if several option had same prefix (e.g. fpud, fpuda), then the
longer one should have been compared first, otherwise when longer option is
passed it would be treated as a short one, because

  CONST_STRNEQ ("fpud", "fpuda")

would be true.  The order of options was correct for ARC, so there were no
bugs per se, but with disassembler_option_cmp there is no risk of such a bug
being introduced in the future.

opcodes/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-dis.c (parse_option): Use disassembler_options_cmp to compare
	  disassembler option strings.
	  (parse_cpu_option): Likewise.
---
 opcodes/arc-dis.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index edd0c07..dc9f5d7 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -740,16 +740,16 @@ operand_iterator_next (struct arc_operand_iterator *iter,
 static void
 parse_option (const char *option)
 {
-  if (CONST_STRNEQ (option, "dsp"))
+  if (!disassembler_options_cmp (option, "dsp"))
     add_to_decodelist (DSP, NONE);
 
-  else if (CONST_STRNEQ (option, "spfp"))
+  else if (!disassembler_options_cmp (option, "spfp"))
     add_to_decodelist (FLOAT, SPX);
 
-  else if (CONST_STRNEQ (option, "dpfp"))
+  else if (!disassembler_options_cmp (option, "dpfp"))
     add_to_decodelist (FLOAT, DPX);
 
-  else if (CONST_STRNEQ (option, "quarkse_em"))
+  else if (!disassembler_options_cmp (option, "quarkse_em"))
     {
       add_to_decodelist (FLOAT, DPX);
       add_to_decodelist (FLOAT, SPX);
@@ -757,16 +757,16 @@ parse_option (const char *option)
       add_to_decodelist (FLOAT, QUARKSE2);
     }
 
-  else if (CONST_STRNEQ (option, "fpuda"))
+  else if (!disassembler_options_cmp (option, "fpuda"))
     add_to_decodelist (FLOAT, DPA);
 
-  else if (CONST_STRNEQ (option, "fpus"))
+  else if (!disassembler_options_cmp (option, "fpus"))
     {
       add_to_decodelist (FLOAT, SP);
       add_to_decodelist (FLOAT, CVT);
     }
 
-  else if (CONST_STRNEQ (option, "fpud"))
+  else if (!disassembler_options_cmp (option, "fpud"))
     {
       add_to_decodelist (FLOAT, DP);
       add_to_decodelist (FLOAT, CVT);
@@ -808,7 +808,7 @@ parse_cpu_option (const char *option)
 
   for (i = 0; cpu_types[i].name; ++i)
     {
-      if (!strcasecmp (cpu_types[i].name, option))
+      if (!disassembler_options_cmp (cpu_types[i].name, option))
 	{
 	  return cpu_types[i].flags;
 	}
-- 
2.8.3

RE: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

Hi Pedro,

> >
> > As a result this macro can be used to improve existing code in
> > arc-dis.c, however it doesn't address the issue that this particular
> > patch is trying to fix.
>
> Hmm, the description you originally send only talked about parsing the
> option value incorrectly.  If you change arc-dis.c:parse_cpu_option to use
> disassembler_options_cmp for comparing options, then I think it should fix
> the original issue you described?  Guess the issue then would be that
> disassembler_options_cmp is case sensitive.  I wonder whether
> architectures really want to be different here..

Right, this will change behaviour for ARC cpu= options - they now will be case
sensitive. Which should be ok, most options that I see in other places
(including arc-dis.c) are case sensitive, so making this case-insensitive was
not a good decision on my side to start with, I think.

>
> Actually I find it a bug that this code even prints to stderr directly in the first
> place.  That'll be the wrong thing to do when this code is being called by GDB
> -- a gdb Python script can disassemble with output redirected to a string, for
> example.

I don't see where it could print right now though - according to the name and
comment disassemble_info->memory_error_func is used for cases when there is an
error reading from memory, not an error parsing options.  To address this
problem, I think a new field should be added to disassembler_info - either a
function that prints to desired error stream or pointer to error stream itself.

Anton

Re: [PATCH] [ARC] Fix handling of cpu=... disassembler option value

[Pedro Alves]

On 06/19/2017 04:17 PM, Anton Kolesov wrote:
> Hi Pedro,

Hi Anton,

>> Actually I find it a bug that this code even prints to stderr directly in the first
>> place.  That'll be the wrong thing to do when this code is being called by GDB
>> -- a gdb Python script can disassemble with output redirected to a string, for
>> example.
> 
> I don't see where it could print right now though - according to the name and
> comment disassemble_info->memory_error_func is used for cases when there is an
> error reading from memory, not an error parsing options.  To address this
> problem, I think a new field should be added to disassembler_info - either a
> function that prints to desired error stream or pointer to error stream itself.

I think it'd have to be a function.  GDB's streams are not FILE pointers;
they're ui_file pointers, which are wrappers around FILE and/or other
kinds of streams [gdb/ui-file.h].

Maybe a better change would be something around making the
"disassemble_init_XXX" routines return/propagate the options parse error
indication up to the caller.  Currently if you pass down an invalid
option you get that "Unrecognized option" sent to stderr, but then
we try disassembling anyway.

TBC, this is not ARC specific and I'm not suggesting that you
should fix this as a sort of prerequisite.

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Pedro Alves]

On 06/19/2017 03:40 PM, Anton Kolesov wrote:
> Changes in V2:
> 
> * Use disassembler_options_cmp to compare string instead of using strtok, which
>   required string duplication.
> * Add a second patch that introduces usage of FOR_EACH_DISASSEMBLER_OPTION to
>   ARC.

Thanks for doing this.

Both patches look good to me (but note I'm not an opcodes maintainer).

From the nit department:

- I'm not sure on coding standard on the bfd side, but on the gdb side
  you'd write "disassembler_options_cmp (...) == 0" instead of 
  !disassembler_options_cmp, since the return value isn't a boolean.

- Spurious leading space after TAB in the ChangeLog entry.

It's kind of a shame IMHO to fix bugs like these without adding
regression tests, to prevent regressions (and to prove it
works :-)).  Could something be easily added to
testsuite/binutils-all/arc/ perhaps?

Both ARM and Power test these FOR_EACH_DISASSEMBLER_OPTION-related paths
using GDB's testsuite.  Grep for "set disassembler-options" under
gdb/testsuite/gdb.arch/.  But IIUC, ARC doesn't support
"set disassembler-options" yet, right?

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Peter Bergner]

On 6/20/17 5:32 AM, Pedro Alves wrote:
> - I'm not sure on coding standard on the bfd side, but on the gdb side
>   you'd write "disassembler_options_cmp (...) == 0" instead of 
>   !disassembler_options_cmp, since the return value isn't a boolean.

It's the same on bfd side too, so "... == 0" is preferred.



> Both ARM and Power test these FOR_EACH_DISASSEMBLER_OPTION-related paths
> using GDB's testsuite.  Grep for "set disassembler-options" under
> gdb/testsuite/gdb.arch/.  But IIUC, ARC doesn't support
> "set disassembler-options" yet, right?

When I added the "set disassembler-options ..." support, I included
support for it in PPC, ARM and S390.  ARC doesn't support it, but from
a quick glance, it also doesn't look too hard to add.

Peter

RE: [PATCH v2 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

> 
> On 6/20/17 5:32 AM, Pedro Alves wrote:
> > - I'm not sure on coding standard on the bfd side, but on the gdb side
> >   you'd write "disassembler_options_cmp (...) == 0" instead of
> >   !disassembler_options_cmp, since the return value isn't a boolean.
> 
> It's the same on bfd side too, so "... == 0" is preferred.

Will fix that.

> 
> > Both ARM and Power test these FOR_EACH_DISASSEMBLER_OPTION-
> related
> > paths using GDB's testsuite.  Grep for "set disassembler-options"
> > under gdb/testsuite/gdb.arch/.  But IIUC, ARC doesn't support "set
> > disassembler-options" yet, right?
> 
> When I added the "set disassembler-options ..." support, I included support
> for it in PPC, ARM and S390.  ARC doesn't support it, but from a quick glance,
> it also doesn't look too hard to add.

I'll look into adding test case to binutils-objdump tests. GDB testsuite should
test only the GDB part of this option setting, I think.

As I've realized, this patch uncovers an issue with my previous GDB patch,
which added the usage of those disassembler option. That patch uses BFD machine
from XML target description as CPU model for disassembler option, but they are
not really the same. For modern ARCs BFD machine could be "ARCv2", "EM" or
"HS"; but CPU models are much more fine grained, as they distinguish various
configurations of ARC EM and ARC HS. While there is a cpu models "em" and "hs"
in ARC assembler, they are actually the least capable so they support the least
set of instructions for a given CPU model. Initially I wanted this "cpu="
disassembler option to select the BFD machine, so it would be directly mapped
to what comes from target description, but our binutils developer asked instead
to use actual CPU models there (since our assembler already had those CPU models
defined and it makes sense that assembler and disassembler options should
match), but I've missed that this also affects they GDB part as well.
Currently with case-insensitive option match disassembler would at least "EM"
and "HS" machines without a warning, achieving the main goal of those patches,
but with case-sensitive match I'd need an additional GDB patch Target
description doesn't really provide information about an exact cpu model for
ARC, so in GDB I need to pass to disassembler the most feature rich CPU model
for a specified BFD machine.

Anton

[PATCH v3 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Anton Kolesov]

Changes in V3:

* Compare disassembler_options_cmp result with integer, rather than treat it as
  a null.
* Add testcases to binutils/testsuite/binutils-all/arc/objdump.exp.

Changes in V2:

* Use disassembler_options_cmp to compare string instead of using strtok, which
  required string duplication.
* Add a second patch that introduces usage of FOR_EACH_DISASSEMBLER_OPTION to
  ARC.

---

There is a bug in handling of cpu=... disassembler option in case there are
other options after it, for example, `cpu=EM,dsp'.  In this case `EM,dsp' is
treated as an option value, and strcasecmp reports is as non-equal to "EM".
This is fixed by using disassembler_options_cmp function, which compares string
treating `,' the same way as `\0'.

This function also solves a problem with option order in parse_option.
Previously, if several option had same prefix (e.g. fpud, fpuda), then the
longer one should have been compared first, otherwise when longer option is
passed it would be treated as a short one, because

  CONST_STRNEQ ("fpud", "fpuda")

would be true.  The order of options was correct for ARC, so there were no
bugs per se, but with disassembler_option_cmp there is no risk of such a bug
being introduced in the future.

opcodes/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-dis.c (parse_option): Use disassembler_options_cmp to compare
	disassembler option strings.
	(parse_cpu_option): Likewise.

binutils/ChangeLog

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* testsuite/binutils-all/arc/double_store.s: New file.
	* testsuite/binutils-all/arc/objdump.exp: Tests for disassembler
	options.
	(do_objfile): New function.
	(check_assembly): Likewise.
---
 binutils/testsuite/binutils-all/arc/double_store.s |  6 ++
 binutils/testsuite/binutils-all/arc/objdump.exp    | 73 +++++++++++++++++-----
 opcodes/arc-dis.c                                  | 16 ++---
 3 files changed, 70 insertions(+), 25 deletions(-)
 create mode 100644 binutils/testsuite/binutils-all/arc/double_store.s

diff --git a/binutils/testsuite/binutils-all/arc/double_store.s b/binutils/testsuite/binutils-all/arc/double_store.s
new file mode 100644
index 0000000..794d7e8
--- /dev/null
+++ b/binutils/testsuite/binutils-all/arc/double_store.s
@@ -0,0 +1,6 @@
+.cpu HS
+.section .text
+.global __start
+__start:
+    std r0r1, [r3]
+
diff --git a/binutils/testsuite/binutils-all/arc/objdump.exp b/binutils/testsuite/binutils-all/arc/objdump.exp
index ca36431..2037b2b 100644
--- a/binutils/testsuite/binutils-all/arc/objdump.exp
+++ b/binutils/testsuite/binutils-all/arc/objdump.exp
@@ -25,31 +25,70 @@ if {[which $OBJDUMP] == 0} then {
 
 send_user "Version [binutil_version $OBJDUMP]"
 
-###########################
-# Set up the test of dsp.s
-###########################
+# Helper functions
 
-if {![binutils_assemble $srcdir/$subdir/dsp.s tmpdir/dsp.o]} then {
-    return
+# Create object file from the assembly source.
+proc do_objfile { srcfile } {
+    global srcdir
+    global subdir
+
+    set objfile [regsub -- "\.s$" $srcfile ".o"]
+
+    if {![binutils_assemble $srcdir/$subdir/$srcfile tmpdir/$objfile]} then {
+	return
+    }
+
+    if [is_remote host] {
+	set objfile [remote_download host tmpdir/$objfile]
+    } else {
+	set objfile tmpdir/$objfile
+    }
+
+    return $objfile
 }
 
-if [is_remote host] {
-    set objfile [remote_download host tmpdir/dsp.o]
-} else {
-    set objfile tmpdir/dsp.o
+# Ensure that disassembler output includes EXPECTED lines.
+proc check_assembly { testname objfile expected { disas_flags "" } } {
+    global OBJDUMP
+    global OBJDUMPFLAGS
+
+    set got [binutils_run $OBJDUMP "$OBJDUMPFLAGS --disassemble $disas_flags \
+	$objfile"]
+
+    if [regexp $expected $got] then {
+	pass $testname
+    } else {
+	fail $testname
+    }
 }
 
 # Make sure that a warning message is generated (because the disassembly does
 # not match the assembled instructions, which has happened because the user
 # has not specified a -M option on the disassembler command line, and so the
 # disassembler has had to guess as the instruction class in use).
-
-set got [binutils_run $OBJDUMP "$OBJDUMPFLAGS --disassemble $objfile"]
-
 set want "Warning: disassembly.*vmac2hnfr\[ \t\]*r0,r2,r4.*dmulh12.f\[ \t\]*r0,r2,r4.*dmulh11.f"
+check_assembly "Warning test" [do_objfile dsp.s] $want
+
+set double_store_hs_expected {std\s*r0r1,\[r3\]}
+set objfile [do_objfile double_store.s]
+check_assembly "arc double_store default -M" $objfile \
+    $double_store_hs_expected
+check_assembly "arc double_store -Mcpu=hs" $objfile \
+    $double_store_hs_expected "-Mcpu=hs"
+check_assembly "arc double_store -Mcpu=hs38_linux" $objfile \
+    $double_store_hs_expected "-Mcpu=hs38_linux"
+set double_store_em_expected ".long 0x1b000006"
+check_assembly "arc double_store -Mcpu=em" $objfile \
+    $double_store_em_expected "-Mcpu=em"
+check_assembly "arc double_store -Mcpu=em4_dmips" $objfile \
+    $double_store_em_expected "-Mcpu=em4_dmips"
+# Test to ensure that only value up to the next `,' is checked.  There used to
+# be a bug, where whole `em,fpus' was compared against known CPU values, and
+# that comparison would fail.  When this bug is present, whole cpu= option will
+# be ignored and instruction will be disassembled as ARC HS.
+check_assembly "arc double_store -Mcpu=em,fpus" $objfile \
+    $double_store_em_expected "-Mcpu=em,fpus"
+# Make sure that the last cpu= value is used.
+check_assembly "arc double_store -Mcpu=hs,cpu=em" $objfile \
+    $double_store_em_expected "-Mcpu=hs,cpu=em"
 
-if [regexp $want $got] then {
-    pass "Warning test"
-} else {
-    fail "Warning test"
-}
diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index edd0c07..b46424a 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -740,16 +740,16 @@ operand_iterator_next (struct arc_operand_iterator *iter,
 static void
 parse_option (const char *option)
 {
-  if (CONST_STRNEQ (option, "dsp"))
+  if (disassembler_options_cmp (option, "dsp") == 0)
     add_to_decodelist (DSP, NONE);
 
-  else if (CONST_STRNEQ (option, "spfp"))
+  else if (disassembler_options_cmp (option, "spfp") == 0)
     add_to_decodelist (FLOAT, SPX);
 
-  else if (CONST_STRNEQ (option, "dpfp"))
+  else if (disassembler_options_cmp (option, "dpfp") == 0)
     add_to_decodelist (FLOAT, DPX);
 
-  else if (CONST_STRNEQ (option, "quarkse_em"))
+  else if (disassembler_options_cmp (option, "quarkse_em") == 0)
     {
       add_to_decodelist (FLOAT, DPX);
       add_to_decodelist (FLOAT, SPX);
@@ -757,16 +757,16 @@ parse_option (const char *option)
       add_to_decodelist (FLOAT, QUARKSE2);
     }
 
-  else if (CONST_STRNEQ (option, "fpuda"))
+  else if (disassembler_options_cmp (option, "fpuda") == 0)
     add_to_decodelist (FLOAT, DPA);
 
-  else if (CONST_STRNEQ (option, "fpus"))
+  else if (disassembler_options_cmp (option, "fpus") == 0)
     {
       add_to_decodelist (FLOAT, SP);
       add_to_decodelist (FLOAT, CVT);
     }
 
-  else if (CONST_STRNEQ (option, "fpud"))
+  else if (disassembler_options_cmp (option, "fpud") == 0)
     {
       add_to_decodelist (FLOAT, DP);
       add_to_decodelist (FLOAT, CVT);
@@ -808,7 +808,7 @@ parse_cpu_option (const char *option)
 
   for (i = 0; cpu_types[i].name; ++i)
     {
-      if (!strcasecmp (cpu_types[i].name, option))
+      if (!disassembler_options_cmp (cpu_types[i].name, option))
 	{
 	  return cpu_types[i].flags;
 	}
-- 
2.8.3

[PATCH v3 2/2] [ARC] Use FOR_EACH_DISASSEMBLER_OPTION to iterate over options

[Anton Kolesov]

This patch updates arc-dis.c:parse_disassembler_options to use a macro
FOR_EACH_DISASSEMBLER_OPTION, which has been introduced in [1], instead of a
homegrown solution to split option string.

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=65b48a81

opcodes/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>

	* arc-dis.c (parse_disassembler_options): Use
	FOR_EACH_DISASSEMBLER_OPTION.
---
 opcodes/arc-dis.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/opcodes/arc-dis.c b/opcodes/arc-dis.c
index b46424a..addd75c 100644
--- a/opcodes/arc-dis.c
+++ b/opcodes/arc-dis.c
@@ -823,6 +823,8 @@ parse_cpu_option (const char *option)
 static void
 parse_disassembler_options (const char *options)
 {
+  const char *option;
+
   if (options == NULL)
     return;
 
@@ -832,25 +834,15 @@ parse_disassembler_options (const char *options)
      CPU when new options are being parsed.  */
   enforced_isa_mask = ARC_OPCODE_NONE;
 
-  while (*options)
+  FOR_EACH_DISASSEMBLER_OPTION (option, options)
     {
-      /* Skip empty options.  */
-      if (*options == ',')
-	{
-	  ++ options;
-	  continue;
-	}
-
       /* A CPU option?  Cannot use STRING_COMMA_LEN because strncmp is also a
 	 preprocessor macro.  */
-      if (strncmp (options, "cpu=", 4) == 0)
+      if (strncmp (option, "cpu=", 4) == 0)
 	/* Strip leading `cpu=`.  */
-	enforced_isa_mask = parse_cpu_option (options + 4);
+	enforced_isa_mask = parse_cpu_option (option + 4);
       else
-	parse_option (options);
-
-      while (*options != ',' && *options != '\0')
-	++ options;
+	parse_option (option);
     }
 }
 
-- 
2.8.3

Re: [PATCH v3 1/2] [ARC] Fix handling of cpu=... disassembler option value

[Nick Clifton]

Hi Anton,

> opcodes/ChangeLog:
> 
> yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>
> 
> 	* arc-dis.c (parse_option): Use disassembler_options_cmp to compare
> 	disassembler option strings.
> 	(parse_cpu_option): Likewise.
> 
> binutils/ChangeLog
> 
> yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>
> 
> 	* testsuite/binutils-all/arc/double_store.s: New file.
> 	* testsuite/binutils-all/arc/objdump.exp: Tests for disassembler
> 	options.
> 	(do_objfile): New function.
> 	(check_assembly): Likewise.
 Approved - please apply.

Cheers
  Nick

Re: [PATCH v3 2/2] [ARC] Use FOR_EACH_DISASSEMBLER_OPTION to iterate over options

[Nick Clifton]

Hi Anton,

> yyyy-mm-dd  Anton Kolesov  <Anton.Kolesov@synopsys.com>
> 
> 	* arc-dis.c (parse_disassembler_options): Use
> 	FOR_EACH_DISASSEMBLER_OPTION.

Approved - please apply.

Cheers
  Nick