public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 2/2] gas/arc: Make .cpu directive case-insensitive
  2016-04-16 16:02 [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Andrew Burgess
  2016-04-16 16:02 ` [PATCH 1/2] gas/arc: Support NPS400 in " Andrew Burgess
@ 2016-04-16 16:02 ` Andrew Burgess
  2016-04-17  8:35 ` [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Claudiu Zissulescu
  2 siblings, 0 replies; 17+ messages in thread
From: Andrew Burgess @ 2016-04-16 16:02 UTC (permalink / raw)
  To: binutils; +Cc: Claudiu.Zissulescu, Cupertino.Miranda, Andrew Burgess

gas/ChangeLog:

	* config/tc-arc.c (arc_option): Make .cpu directive
	case-insensitive.
---
 gas/ChangeLog       |  5 +++++
 gas/config/tc-arc.c | 16 ++++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 169b05c..4f4e970 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -878,26 +878,26 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 
   if (!mach_type_specified_p)
     {
-      if ((!strcmp ("ARC600", cpu))
-	  || (!strcmp ("ARC601", cpu))
-	  || (!strcmp ("A6", cpu)))
+      if ((!strcasecmp ("ARC600", cpu))
+	  || (!strcasecmp ("ARC601", cpu))
+	  || (!strcasecmp ("A6", cpu)))
 	{
 	  md_parse_option (OPTION_MCPU, "arc600");
 	}
-      else if ((!strcmp ("ARC700", cpu))
-	       || (!strcmp ("A7", cpu)))
+      else if ((!strcasecmp ("ARC700", cpu))
+	       || (!strcasecmp ("A7", cpu)))
 	{
 	  md_parse_option (OPTION_MCPU, "arc700");
 	}
-      else if (!strcmp ("EM", cpu))
+      else if (!strcasecmp ("EM", cpu))
 	{
 	  md_parse_option (OPTION_MCPU, "arcem");
 	}
-      else if (!strcmp ("HS", cpu))
+      else if (!strcasecmp ("HS", cpu))
 	{
 	  md_parse_option (OPTION_MCPU, "archs");
 	}
-      else if (!strcmp ("NPS400", cpu))
+      else if (!strcasecmp ("NPS400", cpu))
 	{
 	  md_parse_option (OPTION_MCPU, "nps400");
 	}
-- 
2.6.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/2] gas/arc: Support NPS400 in .cpu directive
  2016-04-16 16:02 [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Andrew Burgess
@ 2016-04-16 16:02 ` Andrew Burgess
  2016-04-16 16:02 ` [PATCH 2/2] gas/arc: Make .cpu directive case-insensitive Andrew Burgess
  2016-04-17  8:35 ` [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Claudiu Zissulescu
  2 siblings, 0 replies; 17+ messages in thread
From: Andrew Burgess @ 2016-04-16 16:02 UTC (permalink / raw)
  To: binutils; +Cc: Claudiu.Zissulescu, Cupertino.Miranda, Andrew Burgess

gas/ChangeLog:

	* config/tc-arc.c (arc_option): Allow NPS400 in .cpu directive.
---
 gas/ChangeLog       | 4 ++++
 gas/config/tc-arc.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 17e0b9a..169b05c 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -897,6 +897,10 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 	{
 	  md_parse_option (OPTION_MCPU, "archs");
 	}
+      else if (!strcmp ("NPS400", cpu))
+	{
+	  md_parse_option (OPTION_MCPU, "nps400");
+	}
       else
 	as_fatal (_("could not find the architecture"));
 
-- 
2.6.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
@ 2016-04-16 16:02 Andrew Burgess
  2016-04-16 16:02 ` [PATCH 1/2] gas/arc: Support NPS400 in " Andrew Burgess
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Andrew Burgess @ 2016-04-16 16:02 UTC (permalink / raw)
  To: binutils; +Cc: Claudiu.Zissulescu, Cupertino.Miranda, Andrew Burgess

Two patches that I've pushed relating to adding support for nps400 to
the ARC assembler .cpu directive.  The first patch adds support for
nps400 while the second makes the .cpu directive case-insensitive.

I've pushed both of these fixes as obvious.

---

Andrew Burgess (2):
  gas/arc: Support NPS400 in .cpu directive
  gas/arc: Make .cpu directive case-insensitive

 gas/ChangeLog       |  9 +++++++++
 gas/config/tc-arc.c | 18 +++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.6.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-16 16:02 [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Andrew Burgess
  2016-04-16 16:02 ` [PATCH 1/2] gas/arc: Support NPS400 in " Andrew Burgess
  2016-04-16 16:02 ` [PATCH 2/2] gas/arc: Make .cpu directive case-insensitive Andrew Burgess
@ 2016-04-17  8:35 ` Claudiu Zissulescu
  2016-04-17 20:35   ` Andrew Burgess
  2 siblings, 1 reply; 17+ messages in thread
From: Claudiu Zissulescu @ 2016-04-17  8:35 UTC (permalink / raw)
  To: Andrew Burgess, Nick Clifton
  Cc: Binutils, Claudiu Zissulescu, Cupertino Miranda

Hi,

I am not sure if those two patches are obvious. One is changing the
semantic of a pseudo-op. I would expect a bit of discussion here, as
in our ARC backend R0 is a symbol while r0 is a register. Hence, we
are case sensitive. And the other one adds (doesn't fix) a new
feature.

Nick should be the one if I am right or not.

Cheers,
Claudiu


On Sat, Apr 16, 2016 at 6:01 PM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> Two patches that I've pushed relating to adding support for nps400 to
> the ARC assembler .cpu directive.  The first patch adds support for
> nps400 while the second makes the .cpu directive case-insensitive.
>
> I've pushed both of these fixes as obvious.
>
> ---
>
> Andrew Burgess (2):
>   gas/arc: Support NPS400 in .cpu directive
>   gas/arc: Make .cpu directive case-insensitive
>
>  gas/ChangeLog       |  9 +++++++++
>  gas/config/tc-arc.c | 18 +++++++++++-------
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> --
> 2.6.4
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-17  8:35 ` [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Claudiu Zissulescu
@ 2016-04-17 20:35   ` Andrew Burgess
  2016-04-17 21:36     ` Claudiu Zissulescu
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2016-04-17 20:35 UTC (permalink / raw)
  To: Claudiu Zissulescu
  Cc: Nick Clifton, Binutils, Claudiu Zissulescu, Cupertino Miranda

* Claudiu Zissulescu <claziss@gmail.com> [2016-04-17 10:35:16 +0200]:

> Hi,
> 
> I am not sure if those two patches are obvious. One is changing the
> semantic of a pseudo-op. I would expect a bit of discussion here, as
> in our ARC backend R0 is a symbol while r0 is a register. Hence, we
> are case sensitive. And the other one adds (doesn't fix) a new
> feature.

I apologise if I have overstepped the mark with these commits.

The change in case sensitivity brings the '.cpu' directive into line
with the -mcpu command line option.

Claudiu, you make it clear in the above that you _could_ object to
this commit, however, even after reading the above a couple of times,
it's not clear if you are actually objecting or not.  If you don't
like the commit then please make an objection, the commit can always
be reverted.

You are right that adding support for nps400 to the .cpu directive is
not a bug fix, however, the code is trivial, follow the existing
pattern of code in that function, and given that nps400 has been
accepted I am curious under what situation you think that that part of
the commit would be objected to in any way?

Again, I apologise if anyone feels I was too forward in pushing these
patches, I certainly did not intend to cause any offence.

Thanks,
Andrew




> 
> Nick should be the one if I am right or not.
> 
> Cheers,
> Claudiu
> 
> 
> On Sat, Apr 16, 2016 at 6:01 PM, Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > Two patches that I've pushed relating to adding support for nps400 to
> > the ARC assembler .cpu directive.  The first patch adds support for
> > nps400 while the second makes the .cpu directive case-insensitive.
> >
> > I've pushed both of these fixes as obvious.
> >
> > ---
> >
> > Andrew Burgess (2):
> >   gas/arc: Support NPS400 in .cpu directive
> >   gas/arc: Make .cpu directive case-insensitive
> >
> >  gas/ChangeLog       |  9 +++++++++
> >  gas/config/tc-arc.c | 18 +++++++++++-------
> >  2 files changed, 20 insertions(+), 7 deletions(-)
> >
> > --
> > 2.6.4
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-17 20:35   ` Andrew Burgess
@ 2016-04-17 21:36     ` Claudiu Zissulescu
  2016-04-17 22:31       ` Andrew Burgess
  0 siblings, 1 reply; 17+ messages in thread
From: Claudiu Zissulescu @ 2016-04-17 21:36 UTC (permalink / raw)
  To: Andrew Burgess
  Cc: Nick Clifton, Binutils, Claudiu Zissulescu, Cupertino Miranda

Hi Andrew,

I will repeat what Nick told me about the obvious commits (you can
trace it in binutils mailing list):

"The only exception is if the patch can be considered to be "obvious",
in which case you
can check it in without prior approval, but you must still post the
patch to the list,
and tell people that you are committing an obvious fix.  The exact
definition of obvious
in this context is a bit nebulous, but I consider it to mean not
"legally significant"[1],
not adding a new feature, and one to which any seasoned programmer
would say "oh yes,
that is obvious".

[1] http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant"

I understand your patches are simple and can be considered obvious,
but as far as I can read what Nick says, we still need to go through
the reviewing process. My understanding of obvious patches are things
like spelling, typos and fixing simple warnings or so.

Anyhow, coming back to your two commits, adding the nps4xx to .cpu
directive is ok with me. The one which ignores case sensitivity, I am
in doubts, as it firstly changes the established semantic of the .cpu
pseudo-op. Then, introduces an uncertainty  how a cpu name is spelled.
Finally, it seems it is a common practice for other processors as well
to use case sensitivity match in this case. Though, I am not 100%
against this latest patch, I would like to debate the pros and cons
for such a change, although it may look trivial, the decision may
affect us years to come.

Best,
Claudiu


On Sun, Apr 17, 2016 at 10:35 PM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Claudiu Zissulescu <claziss@gmail.com> [2016-04-17 10:35:16 +0200]:
>
>> Hi,
>>
>> I am not sure if those two patches are obvious. One is changing the
>> semantic of a pseudo-op. I would expect a bit of discussion here, as
>> in our ARC backend R0 is a symbol while r0 is a register. Hence, we
>> are case sensitive. And the other one adds (doesn't fix) a new
>> feature.
>
> I apologise if I have overstepped the mark with these commits.
>
> The change in case sensitivity brings the '.cpu' directive into line
> with the -mcpu command line option.
>
> Claudiu, you make it clear in the above that you _could_ object to
> this commit, however, even after reading the above a couple of times,
> it's not clear if you are actually objecting or not.  If you don't
> like the commit then please make an objection, the commit can always
> be reverted.
>
> You are right that adding support for nps400 to the .cpu directive is
> not a bug fix, however, the code is trivial, follow the existing
> pattern of code in that function, and given that nps400 has been
> accepted I am curious under what situation you think that that part of
> the commit would be objected to in any way?
>
> Again, I apologise if anyone feels I was too forward in pushing these
> patches, I certainly did not intend to cause any offence.
>
> Thanks,
> Andrew
>
>
>
>
>>
>> Nick should be the one if I am right or not.
>>
>> Cheers,
>> Claudiu
>>
>>
>> On Sat, Apr 16, 2016 at 6:01 PM, Andrew Burgess
>> <andrew.burgess@embecosm.com> wrote:
>> > Two patches that I've pushed relating to adding support for nps400 to
>> > the ARC assembler .cpu directive.  The first patch adds support for
>> > nps400 while the second makes the .cpu directive case-insensitive.
>> >
>> > I've pushed both of these fixes as obvious.
>> >
>> > ---
>> >
>> > Andrew Burgess (2):
>> >   gas/arc: Support NPS400 in .cpu directive
>> >   gas/arc: Make .cpu directive case-insensitive
>> >
>> >  gas/ChangeLog       |  9 +++++++++
>> >  gas/config/tc-arc.c | 18 +++++++++++-------
>> >  2 files changed, 20 insertions(+), 7 deletions(-)
>> >
>> > --
>> > 2.6.4
>> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-17 21:36     ` Claudiu Zissulescu
@ 2016-04-17 22:31       ` Andrew Burgess
  2016-04-18 11:07         ` Nick Clifton
  0 siblings, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2016-04-17 22:31 UTC (permalink / raw)
  To: Claudiu Zissulescu
  Cc: Nick Clifton, Binutils, Claudiu Zissulescu, Cupertino Miranda

* Claudiu Zissulescu <claziss@gmail.com> [2016-04-17 23:36:08 +0200]:

> I will repeat what Nick told me about the obvious commits (you can
> trace it in binutils mailing list):
> 
> "The only exception is if the patch can be considered to be "obvious",
> in which case you
> can check it in without prior approval, but you must still post the
> patch to the list,
> and tell people that you are committing an obvious fix.  The exact
> definition of obvious
> in this context is a bit nebulous, but I consider it to mean not
> "legally significant"[1],
> not adding a new feature, and one to which any seasoned programmer
> would say "oh yes,
> that is obvious".
> 
> [1] http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant"
> 
> I understand your patches are simple and can be considered obvious,
> but as far as I can read what Nick says, we still need to go through
> the reviewing process. My understanding of obvious patches are things
> like spelling, typos and fixing simple warnings or so.

Thank you for taking the time to explain all of this too me, you have
taught me a valuable lesson.

> Anyhow, coming back to your two commits, adding the nps4xx to .cpu
> directive is ok with me.

I have taken the liberty of leaving this commit in the tree.  I'll
keep my fingers crossed that a global maintainer doesn't object :-)

>                          The one which ignores case sensitivity, I am
> in doubts, as it firstly changes the established semantic of the .cpu
> pseudo-op. Then, introduces an uncertainty  how a cpu name is spelled.
> Finally, it seems it is a common practice for other processors as well
> to use case sensitivity match in this case. Though, I am not 100%
> against this latest patch, I would like to debate the pros and cons
> for such a change, although it may look trivial, the decision may
> affect us years to come.

I have reverted this change, and pushed the revert as obvious ;-)

Once again, thank you for your time, and I apologise to you, and any
other binutils developers for the offence I have caused.

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-17 22:31       ` Andrew Burgess
@ 2016-04-18 11:07         ` Nick Clifton
  2016-04-19 18:52           ` Andrew Burgess
  2016-04-19 18:54           ` Andrew Burgess
  0 siblings, 2 replies; 17+ messages in thread
From: Nick Clifton @ 2016-04-18 11:07 UTC (permalink / raw)
  To: Andrew Burgess, Claudiu Zissulescu
  Cc: Binutils, Claudiu Zissulescu, Cupertino Miranda

Hi Andrew,

> I have taken the liberty of leaving this commit in the tree.  I'll
> keep my fingers crossed that a global maintainer doesn't object :-)

Actually I do object, sorry. :-{

There are problems with both of the patches:

[PATCH 1/2] gas/arc: Support NPS400 in .cpu directive

  The problem here is not the patch itself, it is OK, but rather
  that it does not go far enough.  The patch adds support for the
  NPS400 to the .cpu directive, but it does not update the 
  documentation in gas/doc/c-arc.texi detailing the acceptable
  arguments to the .cpu directive.

  [A patch to update the documentation as indicated is pre-approved].

  Further I would suggest that this code can be improved.  Since
  there already is a table of known ARC architecture variants in tc-arc.c
  (cpu_types[], line 420), it would make sense to change the code in
  arc_option() to scan this table.  That way when a new architecture is
  added in the future the programmer will not have to remember to 
  update the arc_option() function as well.

  Additionally the md_show_usage() function could be extended to list
  exactly which architectures are supported by the -mcpu= command line
  option; again by scanning the cpu_types table.  Other targets already
  do this sort of thing.

  [The patch can stay in, but please do consider the above advice].


[PATCH 2/2] gas/arc: Make .cpu directive case-insensitive

  This is definitely not "obvious" because you are changing the 
  functionality of the assembler.  It might be trivial, but it
  certainly is not obvious.

  As Claudiu pointed out removing case-sensitivity does have its
  problems.  If the two of you can agree that it is the right thing
  to do, then I would be OK with the patch - although you would also
  need to update the documentation in gas/doc/c-arc.texi to explicitly
  state that the parameter to the .cpu directive is case insensitive.

  So, first of all, please revert this patch.  Then if you still feel
  that case insensitivity is important, please present your arguments
  on this list.  Assuming that agreement can be reached, please submit
  a revised patch for review afterwards.

Cheers
  Nick


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-18 11:07         ` Nick Clifton
@ 2016-04-19 18:52           ` Andrew Burgess
  2016-04-20  9:18             ` Nick Clifton
  2016-04-19 18:54           ` Andrew Burgess
  1 sibling, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2016-04-19 18:52 UTC (permalink / raw)
  To: Nick Clifton
  Cc: Claudiu Zissulescu, Binutils, Claudiu Zissulescu, Cupertino Miranda

* Nick Clifton <nickc@redhat.com> [2016-04-18 12:07:44 +0100]:

> Hi Andrew,
> 
> > I have taken the liberty of leaving this commit in the tree.  I'll
> > keep my fingers crossed that a global maintainer doesn't object :-)
> 
> Actually I do object, sorry. :-{
> 
> There are problems with both of the patches:
> 
> [PATCH 1/2] gas/arc: Support NPS400 in .cpu directive
> 
>   The problem here is not the patch itself, it is OK, but rather
>   that it does not go far enough.  The patch adds support for the
>   NPS400 to the .cpu directive, but it does not update the 
>   documentation in gas/doc/c-arc.texi detailing the acceptable
>   arguments to the .cpu directive.
> 
>   [A patch to update the documentation as indicated is
>   pre-approved].

I've attached the documentation patch below.  I've added an extra xref
beyond what you preapproved, so ...

OK?

Thanks,
Andrew

---

gas/doc/arc: Add nps400 references into the documentation

Add nps400 to the list of acceptable values for the -mcpu command line
switch, and to the .cpu directive.

I've added an extra cross reference from -mcpu to .cpu to improve
navigation of the documentation.

gas/ChangeLog:

	* doc/c-arc.texi (ARC Options): Add nps400 to list of valus for
	-mcpu.  Add cross reference to .cpu directive from -mcpu option.
	(ARC Directives): Add NPS400 to .cpu directive list.
---
 gas/ChangeLog      |  6 ++++++
 gas/doc/c-arc.texi | 10 ++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/gas/doc/c-arc.texi b/gas/doc/c-arc.texi
index a3e18cf..36d3ec0 100644
--- a/gas/doc/c-arc.texi
+++ b/gas/doc/c-arc.texi
@@ -56,6 +56,9 @@ Assemble for ARC 601.  Alias: @code{-mARC601}.
 @cindex @code{mARC700} command line option, ARC
 Assemble for ARC 700.  Aliases: @code{-mA7}, @code{-mARC700}.
 
+@item nps400
+Assemble for NPS400.
+
 @item arcem
 @cindex @code{mEM} command line option, ARC
 Assemble for ARC EM.  Aliases: @code{-mEM}
@@ -66,8 +69,8 @@ Assemble for ARC HS.  Aliases: @code{-mHS}, @code{-mav2hs}.
 
 @end table
 
-Note: the @code{.cpu} directive can to be used to select a core
-variant from within assembly code.
+Note: the @code{.cpu} directive (@pxref{ARC Directives}) can
+to be used to select a core variant from within assembly code.
 
 @cindex @code{-EB} command line option, ARC
 @item -EB
@@ -350,6 +353,9 @@ Assemble for the ARC600 instruction set.
 @item ARC700
 Assemble for the ARC700 instruction set.
 
+@item NPS400
+Assemble for the NPS400 instruction set.
+
 @item EM
 Assemble for the ARC EM instruction set.
 
-- 
2.5.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-18 11:07         ` Nick Clifton
  2016-04-19 18:52           ` Andrew Burgess
@ 2016-04-19 18:54           ` Andrew Burgess
  2016-04-20  9:07             ` Claudiu Zissulescu
  2016-04-20  9:23             ` Nick Clifton
  1 sibling, 2 replies; 17+ messages in thread
From: Andrew Burgess @ 2016-04-19 18:54 UTC (permalink / raw)
  To: Nick Clifton
  Cc: Claudiu Zissulescu, Binutils, Claudiu Zissulescu, Cupertino Miranda

* Nick Clifton <nickc@redhat.com> [2016-04-18 12:07:44 +0100]:

> Hi Andrew,
> 
> > I have taken the liberty of leaving this commit in the tree.  I'll
> > keep my fingers crossed that a global maintainer doesn't object :-)
> 
> Actually I do object, sorry. :-{
> 
> There are problems with both of the patches:
> 
> [PATCH 1/2] gas/arc: Support NPS400 in .cpu directive
> 
>   The problem here is not the patch itself, it is OK, but rather
>   that it does not go far enough.  The patch adds support for the
>   NPS400 to the .cpu directive, but it does not update the 
>   documentation in gas/doc/c-arc.texi detailing the acceptable
>   arguments to the .cpu directive.
> 
>   [A patch to update the documentation as indicated is pre-approved].
> 
>   Further I would suggest that this code can be improved.  Since
>   there already is a table of known ARC architecture variants in tc-arc.c
>   (cpu_types[], line 420), it would make sense to change the code in
>   arc_option() to scan this table.  That way when a new architecture is
>   added in the future the programmer will not have to remember to 
>   update the arc_option() function as well.
> 
>   Additionally the md_show_usage() function could be extended to list
>   exactly which architectures are supported by the -mcpu= command line
>   option; again by scanning the cpu_types table.  Other targets already
>   do this sort of thing.
> 
>   [The patch can stay in, but please do consider the above advice].

Below is a patch that does the things you mention above:

  - Uses the same table to drive -mcpu and .cpu
  - Uses the same table to generate the cpu list in the docs.

OK?

Thanks,
Andrew

---

arc/gas: Unify -mcpu option and .cpu directive handling

This change makes use of the single cpu_types table for handling the
.cpu directive and the -mcpu command line option.  At the same time the
help text for -mcpu is extended to include the list of possible cpu
types, generated from the table.

gas/ChangeLog:

	* config/tc-arc.c (MAX_NUMBER_OF_ALIASES): Define.
	(struct cpu_type): Add aliases member, convert string points to
	NULL instead of 0.
	(cpu_types): Fill in the aliases field.
	(arc_handle_cpu_option_string): New function.
	(arc_option): Use arc_handle_cpu_option_string to process the
	option.
	(arc_show_cpu_list): New function.
	(md_show_usage): Call arc_show_cpu_list.  Convert help text to
	lower case to match all other help text.  Remove use of tabs,
	inline with global help text.
---
 gas/ChangeLog       |  14 ++++++
 gas/config/tc-arc.c | 123 +++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 97 insertions(+), 40 deletions(-)

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 169b05c..c556f54 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -408,10 +408,14 @@ static struct hash_control *arc_reg_hash;
 /* The hash table of aux register symbols.  */
 static struct hash_control *arc_aux_hash;
 
+/* Maximum number of CPU aliases in the cpu_type table.  */
+#define MAX_NUMBER_OF_ALIASES 2
+
 /* A table of CPU names and opcode sets.  */
 static const struct cpu_type
 {
   const char *name;
+  const char *alises [MAX_NUMBER_OF_ALIASES + 1];
   unsigned flags;
   int mach;
   unsigned eflags;
@@ -419,17 +423,18 @@ static const struct cpu_type
 }
   cpu_types[] =
 {
-  { "arc600", ARC_OPCODE_ARC600,  bfd_mach_arc_arc600,
-    E_ARC_MACH_ARC600,  0x00},
-  { "arc700", ARC_OPCODE_ARC700,  bfd_mach_arc_arc700,
-    E_ARC_MACH_ARC700,  0x00},
-  { "nps400", ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
+  { "ARC600", { "ARC601", "A6", NULL },
+    ARC_OPCODE_ARC600,  bfd_mach_arc_arc600, E_ARC_MACH_ARC600,  0x00},
+  { "ARC700", { "A7", NULL },
+    ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00},
+  { "NPS400", { NULL },
+    ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
     E_ARC_MACH_NPS400,  0x00},
-  { "arcem",  ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2EM, ARC_CD},
-  { "archs",  ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2HS, ARC_CD},
-  { 0, 0, 0, 0, 0 }
+  { "ARCEM", { "EM", NULL },
+    ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2EM, ARC_CD},
+  { "ARCHS", { "HS", NULL },
+    ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2HS, ARC_CD},
+  { NULL, { NULL }, 0, 0, 0, 0 }
 };
 
 /* Used by the arc_reloc_op table.  Order is important.  */
@@ -861,6 +866,40 @@ arc_lcomm (int ignore)
     symbol_get_bfdsym (symbolP)->flags |= BSF_OBJECT;
 }
 
+/* Take the string passed to the .cpu directive and configure the assembler
+   as though the -mcpu option was used.  Raises an error if the string is
+   not a valid cpu name.  */
+
+static void
+arc_handle_cpu_option_string (const char *cpu)
+{
+  int i;
+
+  for (i = 0; cpu_types[i].name != NULL; ++i)
+    {
+      if (!strcmp (cpu_types[i].name, cpu))
+        {
+          md_parse_option (OPTION_MCPU, cpu_types[i].name);
+          return;
+        }
+      else
+        {
+          int j;
+
+          for (j = 0; cpu_types[i].alises[j] != NULL; ++j)
+            {
+              if (!strcmp (cpu_types[i].alises[j], cpu))
+                {
+                  md_parse_option (OPTION_MCPU, cpu_types[i].name);
+                  return;
+                }
+            }
+        }
+    }
+
+  as_fatal (_("could not find the architecture"));
+}
+
 /* Select the cpu we're assembling for.  */
 
 static void
@@ -878,31 +917,7 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 
   if (!mach_type_specified_p)
     {
-      if ((!strcmp ("ARC600", cpu))
-	  || (!strcmp ("ARC601", cpu))
-	  || (!strcmp ("A6", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc600");
-	}
-      else if ((!strcmp ("ARC700", cpu))
-	       || (!strcmp ("A7", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc700");
-	}
-      else if (!strcmp ("EM", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "arcem");
-	}
-      else if (!strcmp ("HS", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "archs");
-	}
-      else if (!strcmp ("NPS400", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "nps400");
-	}
-      else
-	as_fatal (_("could not find the architecture"));
+      arc_handle_cpu_option_string (cpu);
 
       if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, mach))
 	as_fatal (_("could not set architecture and machine"));
@@ -3238,21 +3253,49 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
   return 1;
 }
 
+/* Display the list of cpu names for use in the help text.  */
+static void
+arc_show_cpu_list (FILE *stream)
+{
+  int i, offset;
+  static const char *spaces = "                          ";
+
+  fprintf (stream, spaces);
+  offset = strlen (spaces);
+  for (i = 0; cpu_types[i].name != NULL; ++i)
+    {
+      bfd_boolean last = (cpu_types[i + 1].name == NULL);
+
+      /* If displaying the new cpu name string, and the ', ' (for all but
+         the last one) will take us past a target width of 80 characters,
+         then it's time for a new line.  */
+      if (offset + strlen (cpu_types[i].name) + (last ? 0 : 2) > 80)
+        {
+          fprintf (stream, "\n%s", spaces);
+          offset = strlen (spaces);
+        }
+
+      fprintf (stream, "%s%s", cpu_types[i].name, (last ? "\n" : ", "));
+      offset += strlen (cpu_types [i].name) + (last ? 0 : 2);
+    }
+}
+
 void
 md_show_usage (FILE *stream)
 {
   fprintf (stream, _("ARC-specific assembler options:\n"));
 
-  fprintf (stream, "  -mcpu=<cpu name>\t  assemble for CPU <cpu name>\n");
-  fprintf (stream,
-	   "  -mcode-density\t  enable code density option for ARC EM\n");
-
+  fprintf (stream, "\
+  -mcpu=<cpu name>        assemble for CPU <cpu name>, one of:\n");
+  arc_show_cpu_list (stream);
+  fprintf (stream, "\
+  -mcode-density          enable code density option for ARC EM\n");
   fprintf (stream, _("\
   -EB                     assemble code for a big-endian cpu\n"));
   fprintf (stream, _("\
   -EL                     assemble code for a little-endian cpu\n"));
   fprintf (stream, _("\
-  -mrelax                  Enable relaxation\n"));
+  -mrelax                 enable relaxation\n"));
 
 }
 
-- 
2.5.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-19 18:54           ` Andrew Burgess
@ 2016-04-20  9:07             ` Claudiu Zissulescu
  2016-04-20  9:25               ` Nick Clifton
  2016-04-20 11:24               ` Andrew Burgess
  2016-04-20  9:23             ` Nick Clifton
  1 sibling, 2 replies; 17+ messages in thread
From: Claudiu Zissulescu @ 2016-04-20  9:07 UTC (permalink / raw)
  To: Andrew Burgess, Nick Clifton
  Cc: Claudiu Zissulescu, Binutils, Cupertino Miranda

Hi Andrew,

> +/* Maximum number of CPU aliases in the cpu_type table.  */
> +#define MAX_NUMBER_OF_ALIASES 2
> +
>  /* A table of CPU names and opcode sets.  */
>  static const struct cpu_type
>  {
>    const char *name;
> +  const char *alises [MAX_NUMBER_OF_ALIASES + 1];

Instead of this list of aliases, I would list all the alternatives in cpu_types[]. On the expense of more used memory, we will get rid of the MAX_NUMBER _OF_ALIASES and the need to maintain it whenever we want to add a new cpu alias.  Also the support routines will look much cleaner, we will have a single scanning loop instead of two. Please take it as recommendation. Regardless the decision you take, the patch seems  alright to me.

Best,
Claudiu

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-19 18:52           ` Andrew Burgess
@ 2016-04-20  9:18             ` Nick Clifton
  0 siblings, 0 replies; 17+ messages in thread
From: Nick Clifton @ 2016-04-20  9:18 UTC (permalink / raw)
  To: Andrew Burgess
  Cc: Claudiu Zissulescu, Binutils, Claudiu Zissulescu, Cupertino Miranda

Hi Andrew,

> gas/ChangeLog:
> 
> 	* doc/c-arc.texi (ARC Options): Add nps400 to list of valus for
> 	-mcpu.  Add cross reference to .cpu directive from -mcpu option.
> 	(ARC Directives): Add NPS400 to .cpu directive list.

Approved - please apply.  Thanks for taking the time to do this.

Cheers
  Nick

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-19 18:54           ` Andrew Burgess
  2016-04-20  9:07             ` Claudiu Zissulescu
@ 2016-04-20  9:23             ` Nick Clifton
  1 sibling, 0 replies; 17+ messages in thread
From: Nick Clifton @ 2016-04-20  9:23 UTC (permalink / raw)
  To: Andrew Burgess
  Cc: Claudiu Zissulescu, Binutils, Claudiu Zissulescu, Cupertino Miranda

Hi Andrew,

> gas/ChangeLog:
> 
> 	* config/tc-arc.c (MAX_NUMBER_OF_ALIASES): Define.
> 	(struct cpu_type): Add aliases member, convert string points to
> 	NULL instead of 0.
> 	(cpu_types): Fill in the aliases field.
> 	(arc_handle_cpu_option_string): New function.
> 	(arc_option): Use arc_handle_cpu_option_string to process the
> 	option.
> 	(arc_show_cpu_list): New function.
> 	(md_show_usage): Call arc_show_cpu_list.  Convert help text to
> 	lower case to match all other help text.  Remove use of tabs,
> 	inline with global help text.

Approved - please apply.

Note:

> +  const char *alises [MAX_NUMBER_OF_ALIASES + 1];

You could set the this just [MAX_NUMBERR_OF_ALIASES] and then ...

> +          for (j = 0; cpu_types[i].alises[j] != NULL; ++j)

test for i < MAX_NUMBER_OF_ALIASES as well as a NULL alias.

Which would save a very small amount of space and is probably 
not worth it.  But it just struck me as odd that you would create
an array with MAX + 1 elements in it.

Cheers
  Nick

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-20  9:07             ` Claudiu Zissulescu
@ 2016-04-20  9:25               ` Nick Clifton
  2016-04-20 11:24               ` Andrew Burgess
  1 sibling, 0 replies; 17+ messages in thread
From: Nick Clifton @ 2016-04-20  9:25 UTC (permalink / raw)
  To: Claudiu Zissulescu, Andrew Burgess
  Cc: Claudiu Zissulescu, Binutils, Cupertino Miranda

Hi Andrew, Hi Claudiu,

  [Note to self - read all emails first, *then* reply...]

> Instead of this list of aliases, I would list all the alternatives in cpu_types[].

This would be a much better idea.

Cheers
  Nick

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-20  9:07             ` Claudiu Zissulescu
  2016-04-20  9:25               ` Nick Clifton
@ 2016-04-20 11:24               ` Andrew Burgess
  2016-04-20 12:01                 ` Claudiu Zissulescu
  1 sibling, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2016-04-20 11:24 UTC (permalink / raw)
  To: Claudiu Zissulescu, Nick Clifton; +Cc: binutils, Cupertino Miranda

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-04-20 09:07:18 +0000]:

> Hi Andrew,
> 
> > +/* Maximum number of CPU aliases in the cpu_type table.  */
> > +#define MAX_NUMBER_OF_ALIASES 2
> > +
> >  /* A table of CPU names and opcode sets.  */
> >  static const struct cpu_type
> >  {
> >    const char *name;
> > +  const char *alises [MAX_NUMBER_OF_ALIASES + 1];
> 
> Instead of this list of aliases, I would list all the alternatives
> in cpu_types[]. On the expense of more used memory, we will get rid
> of the MAX_NUMBER _OF_ALIASES and the need to maintain it whenever
> we want to add a new cpu alias.  Also the support routines will look
> much cleaner, we will have a single scanning loop instead of
> two. Please take it as recommendation. Regardless the decision you
> take, the patch seems alright to me.

* Nick Clifton <nickc@redhat.com> [2016-04-20 10:25:13 +0100]:

> 
> > Instead of this list of aliases, I would list all the alternatives
> > in cpu_types[].
> 
> This would be a much better idea.

I disagree and think it's a much worse idea.  My concern was never
about memory usage, my concern is data duplication, how long until we
have a bug where one alias is configured slightly differently to
another alias.  Much better, I still think, to have one list of names
and one set of configuration, and link those together.

The concern about the support routines being "cleaner" is valid, but
then, that's why they are support routines, to hide the grubby
detail.  Still, in deference I have merged the name field and the
aliases into a single list, this makes the support routines slightly
easier on the eyes, but we still fundamentally have two loops one over
table entries and one over aliases within the entry.

I actually preferred the code as it was before really, the first
"name" is special, which is why I originally left it as a separate
field; it's the only name accepted in 'arc_select_cpu' (so -mcpu=TYPE
on the command line does not like aliases) while the handling of .cpu
directive is the only place that aliases are accepted.  Though this
specialness is not something I would choose I originally left it in
just to avoid changing the behaviour.

Still, here's a revised patch with the name / aliases merged into a
single list for consideration, though if this patch can't be merged in
the current form then it's certainly not a blocker for me.

Thanks,
Andrew

---

arc/gas: Unify -mcpu option and .cpu directive handling

This change makes use of the single cpu_types table for handling the
.cpu directive and the -mcpu command line option.  At the same time the
help text for -mcpu is extended to include the list of possible cpu
types, generated from the table.

gas/ChangeLog:

	* config/tc-arc.c (MAX_CPU_NAMES): Define.
	(struct cpu_type): Change names field to a list.
	(cpu_types): Add all aliases to the names list.
	(NUMBER_OF_CPU_TYPES): Define.
	(arc_select_cpu): Updates to reflect changes in cpu_types table.
	(arc_handle_cpu_option_string): New function.
	(arc_option): Use arc_handle_cpu_option_string to process the
	option.
	(arc_show_cpu_list): New function.
	(md_show_usage): Call arc_show_cpu_list.  Convert help text to
	lower case to match all other help text.  Remove use of tabs,
	inline with global help text.
---
 gas/ChangeLog       |  15 ++++++
 gas/config/tc-arc.c | 128 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 97 insertions(+), 46 deletions(-)

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 169b05c..0d11209 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -408,10 +408,13 @@ static struct hash_control *arc_reg_hash;
 /* The hash table of aux register symbols.  */
 static struct hash_control *arc_aux_hash;
 
+/* Maximum number of CPU aliases in the cpu_type table.  */
+#define MAX_CPU_NAMES 3
+
 /* A table of CPU names and opcode sets.  */
 static const struct cpu_type
 {
-  const char *name;
+  const char *names [MAX_CPU_NAMES];
   unsigned flags;
   int mach;
   unsigned eflags;
@@ -419,19 +422,21 @@ static const struct cpu_type
 }
   cpu_types[] =
 {
-  { "arc600", ARC_OPCODE_ARC600,  bfd_mach_arc_arc600,
-    E_ARC_MACH_ARC600,  0x00},
-  { "arc700", ARC_OPCODE_ARC700,  bfd_mach_arc_arc700,
-    E_ARC_MACH_ARC700,  0x00},
-  { "nps400", ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
+  { { "ARC600", "ARC601", "A6" },
+    ARC_OPCODE_ARC600,  bfd_mach_arc_arc600, E_ARC_MACH_ARC600,  0x00},
+  { { "ARC700", "A7" },
+    ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00},
+  { { "NPS400" },
+    ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
     E_ARC_MACH_NPS400,  0x00},
-  { "arcem",  ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2EM, ARC_CD},
-  { "archs",  ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2HS, ARC_CD},
-  { 0, 0, 0, 0, 0 }
+  { { "ARCEM", "EM" },
+    ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2EM, ARC_CD},
+  { { "ARCHS", "HS" },
+    ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2HS, ARC_CD},
 };
 
+#define NUMBER_OF_CPU_TYPES (sizeof (cpu_types) / sizeof (cpu_types[0]))
+
 /* Used by the arc_reloc_op table.  Order is important.  */
 #define O_gotoff  O_md1     /* @gotoff relocation.  */
 #define O_gotpc   O_md2     /* @gotpc relocation.  */
@@ -745,14 +750,14 @@ static void
 arc_select_cpu (const char *arg)
 {
   int cpu_flags = 0;
-  int i;
+  unsigned int i;
 
-  for (i = 0; cpu_types[i].name; ++i)
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
     {
-      if (!strcasecmp (cpu_types[i].name, arg))
+      if (!strcasecmp (cpu_types[i].names[0], arg))
         {
           arc_target = cpu_types[i].flags;
-          arc_target_name = cpu_types[i].name;
+          arc_target_name = cpu_types[i].names[0];
           arc_features = cpu_types[i].features;
           arc_mach_type = cpu_types[i].mach;
           cpu_flags = cpu_types[i].eflags;
@@ -760,8 +765,9 @@ arc_select_cpu (const char *arg)
         }
     }
 
-  if (!cpu_types[i].name)
+  if (i == NUMBER_OF_CPU_TYPES)
     as_fatal (_("unknown architecture: %s\n"), arg);
+
   gas_assert (cpu_flags != 0);
   arc_eflag = (arc_eflag & ~EF_ARC_MACH_MSK) | cpu_flags;
 }
@@ -861,6 +867,32 @@ arc_lcomm (int ignore)
     symbol_get_bfdsym (symbolP)->flags |= BSF_OBJECT;
 }
 
+/* Take the string passed to the .cpu directive and configure the assembler
+   as though the -mcpu option was used.  Raises an error if the string is
+   not a valid cpu name.  */
+
+static void
+arc_handle_cpu_option_string (const char *cpu)
+{
+  unsigned int i;
+
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
+    {
+      int j;
+
+      for (j = 0; j < MAX_CPU_NAMES && cpu_types[i].names[j] != NULL; ++j)
+        {
+          if (!strcmp (cpu_types[i].names[j], cpu))
+            {
+              md_parse_option (OPTION_MCPU, cpu_types[i].names[0]);
+              return;
+            }
+        }
+    }
+
+  as_fatal (_("could not find the architecture"));
+}
+
 /* Select the cpu we're assembling for.  */
 
 static void
@@ -878,31 +910,7 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 
   if (!mach_type_specified_p)
     {
-      if ((!strcmp ("ARC600", cpu))
-	  || (!strcmp ("ARC601", cpu))
-	  || (!strcmp ("A6", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc600");
-	}
-      else if ((!strcmp ("ARC700", cpu))
-	       || (!strcmp ("A7", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc700");
-	}
-      else if (!strcmp ("EM", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "arcem");
-	}
-      else if (!strcmp ("HS", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "archs");
-	}
-      else if (!strcmp ("NPS400", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "nps400");
-	}
-      else
-	as_fatal (_("could not find the architecture"));
+      arc_handle_cpu_option_string (cpu);
 
       if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, mach))
 	as_fatal (_("could not set architecture and machine"));
@@ -3238,21 +3246,49 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
   return 1;
 }
 
+/* Display the list of cpu names for use in the help text.  */
+static void
+arc_show_cpu_list (FILE *stream)
+{
+  unsigned int i, offset;
+  static const char *spaces = "                          ";
+
+  fprintf (stream, spaces);
+  offset = strlen (spaces);
+  for (i = 0; i < NUMBER_OF_CPU_TYPES; ++i)
+    {
+      bfd_boolean last = (i + 1 == NUMBER_OF_CPU_TYPES);
+
+      /* If displaying the new cpu name string, and the ', ' (for all but
+         the last one) will take us past a target width of 80 characters,
+         then it's time for a new line.  */
+      if (offset + strlen (cpu_types[i].names[0]) + (last ? 0 : 2) > 80)
+        {
+          fprintf (stream, "\n%s", spaces);
+          offset = strlen (spaces);
+        }
+
+      fprintf (stream, "%s%s", cpu_types[i].names[0], (last ? "\n" : ", "));
+      offset += strlen (cpu_types [i].names[0]) + (last ? 0 : 2);
+    }
+}
+
 void
 md_show_usage (FILE *stream)
 {
   fprintf (stream, _("ARC-specific assembler options:\n"));
 
-  fprintf (stream, "  -mcpu=<cpu name>\t  assemble for CPU <cpu name>\n");
-  fprintf (stream,
-	   "  -mcode-density\t  enable code density option for ARC EM\n");
-
+  fprintf (stream, "\
+  -mcpu=<cpu name>        assemble for CPU <cpu name>, one of:\n");
+  arc_show_cpu_list (stream);
+  fprintf (stream, "\
+  -mcode-density          enable code density option for ARC EM\n");
   fprintf (stream, _("\
   -EB                     assemble code for a big-endian cpu\n"));
   fprintf (stream, _("\
   -EL                     assemble code for a little-endian cpu\n"));
   fprintf (stream, _("\
-  -mrelax                  Enable relaxation\n"));
+  -mrelax                 enable relaxation\n"));
 
 }
 
-- 
2.5.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-20 11:24               ` Andrew Burgess
@ 2016-04-20 12:01                 ` Claudiu Zissulescu
  2016-04-20 12:58                   ` Andrew Burgess
  0 siblings, 1 reply; 17+ messages in thread
From: Claudiu Zissulescu @ 2016-04-20 12:01 UTC (permalink / raw)
  To: Andrew Burgess, Nick Clifton; +Cc: binutils, Cupertino Miranda

 
> I disagree and think it's a much worse idea.  My concern was never
> about memory usage, my concern is data duplication, how long until we
> have a bug where one alias is configured slightly differently to
> another alias

This is one of the reason why macros have been introduced. For example:
#define A7_CPU_TYPE(NAME) { #NAME,  ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00}

Then you just enumerate it in the structure:
A7_CPU_TYPE (ARC700),
A7_CPU_TYPE (A7),

So on, so forth...

Attached is a patch that shows this concept, though it needs to be validated:

diff --git a/gas/config/tc-arc.c b/gas/config/tc-arc.c
index 169b05c..07e78df 100644
--- a/gas/config/tc-arc.c
+++ b/gas/config/tc-arc.c
@@ -408,6 +408,16 @@ static struct hash_control *arc_reg_hash;
 /* The hash table of aux register symbols.  */
 static struct hash_control *arc_aux_hash;
 
+#define A6_CPU_TYPE(NAME)						\
+  { #NAME, ARC_OPCODE_ARC600,  bfd_mach_arc_arc600, E_ARC_MACH_ARC600,  0x00},
+#define A7_CPU_TYPE(NAME)						\
+  { #NAME, ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00},
+#define EM_CPU_TYPE(NAME)						\
+  { #NAME, ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2EM, ARC_CD},
+#define HS_CPU_TYPE(NAME)						\
+  { #NAME, ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2, EF_ARC_CPU_ARCV2HS, ARC_CD},
+
+
 /* A table of CPU names and opcode sets.  */
 static const struct cpu_type
 {
@@ -419,17 +429,18 @@ static const struct cpu_type
 }
   cpu_types[] =
 {
-  { "arc600", ARC_OPCODE_ARC600,  bfd_mach_arc_arc600,
-    E_ARC_MACH_ARC600,  0x00},
-  { "arc700", ARC_OPCODE_ARC700,  bfd_mach_arc_arc700,
-    E_ARC_MACH_ARC700,  0x00},
-  { "nps400", ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
+  A6_CPU_TYPE(ARC600)
+  A6_CPU_TYPE(ARC601)
+  A6_CPU_TYPE(ARC6)
+  A7_CPU_TYPE(ARC700)
+  A7_CPU_TYPE(A7)
+  { "NPS400", ARC_OPCODE_ARC700 | ARC_OPCODE_NPS400, bfd_mach_arc_nps400,
     E_ARC_MACH_NPS400,  0x00},
-  { "arcem",  ARC_OPCODE_ARCv2EM, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2EM, ARC_CD},
-  { "archs",  ARC_OPCODE_ARCv2HS, bfd_mach_arc_arcv2,
-    EF_ARC_CPU_ARCV2HS, ARC_CD},
-  { 0, 0, 0, 0, 0 }
+  EM_CPU_TYPE(ARCEM)
+  EM_CPU_TYPE(EM)
+  HS_CPU_TYPE(ARCHS)
+  HS_CPU_TYPE(HS)
+  { NULL, 0, 0, 0, 0 }
 };
 
 /* Used by the arc_reloc_op table.  Order is important.  */
@@ -861,6 +872,27 @@ arc_lcomm (int ignore)
     symbol_get_bfdsym (symbolP)->flags |= BSF_OBJECT;
 }
 
+/* Take the string passed to the .cpu directive and configure the assembler
+   as though the -mcpu option was used.  Raises an error if the string is
+   not a valid cpu name.  */
+
+static void
+arc_handle_cpu_option_string (const char *cpu)
+{
+  int i;
+
+  for (i = 0; cpu_types[i].name != NULL; ++i)
+    {
+      if (!strcmp (cpu_types[i].name, cpu))
+        {
+          md_parse_option (OPTION_MCPU, cpu_types[i].name);
+          return;
+        }
+    }
+
+  as_fatal (_("could not find the architecture"));
+}
+
 /* Select the cpu we're assembling for.  */
 
 static void
@@ -878,31 +910,7 @@ arc_option (int ignore ATTRIBUTE_UNUSED)
 
   if (!mach_type_specified_p)
     {
-      if ((!strcmp ("ARC600", cpu))
-	  || (!strcmp ("ARC601", cpu))
-	  || (!strcmp ("A6", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc600");
-	}
-      else if ((!strcmp ("ARC700", cpu))
-	       || (!strcmp ("A7", cpu)))
-	{
-	  md_parse_option (OPTION_MCPU, "arc700");
-	}
-      else if (!strcmp ("EM", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "arcem");
-	}
-      else if (!strcmp ("HS", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "archs");
-	}
-      else if (!strcmp ("NPS400", cpu))
-	{
-	  md_parse_option (OPTION_MCPU, "nps400");
-	}
-      else
-	as_fatal (_("could not find the architecture"));
+      arc_handle_cpu_option_string (cpu);
 
       if (!bfd_set_arch_mach (stdoutput, bfd_arch_arc, mach))
 	as_fatal (_("could not set architecture and machine"));
@@ -3238,21 +3246,49 @@ md_parse_option (int c, const char *arg ATTRIBUTE_UNUSED)
   return 1;
 }
 
+/* Display the list of cpu names for use in the help text.  */
+static void
+arc_show_cpu_list (FILE *stream)
+{
+  int i, offset;
+  static const char *spaces = "                          ";
+
+  fprintf (stream, "%s", spaces);
+  offset = strlen (spaces);
+  for (i = 0; cpu_types[i].name != NULL; ++i)
+    {
+      bfd_boolean last = (cpu_types[i + 1].name == NULL);
+
+      /* If displaying the new cpu name string, and the ', ' (for all but
+         the last one) will take us past a target width of 80 characters,
+         then it's time for a new line.  */
+      if (offset + strlen (cpu_types[i].name) + (last ? 0 : 2) > 80)
+        {
+          fprintf (stream, "\n%s", spaces);
+          offset = strlen (spaces);
+        }
+
+      fprintf (stream, "%s%s", cpu_types[i].name, (last ? "\n" : ", "));
+      offset += strlen (cpu_types [i].name) + (last ? 0 : 2);
+    }
+}
+
 void
 md_show_usage (FILE *stream)
 {
   fprintf (stream, _("ARC-specific assembler options:\n"));
 
-  fprintf (stream, "  -mcpu=<cpu name>\t  assemble for CPU <cpu name>\n");
-  fprintf (stream,
-	   "  -mcode-density\t  enable code density option for ARC EM\n");
-
+  fprintf (stream, "\
+  -mcpu=<cpu name>        assemble for CPU <cpu name>, one of:\n");
+  arc_show_cpu_list (stream);
+  fprintf (stream, "\
+  -mcode-density          enable code density option for ARC EM\n");
   fprintf (stream, _("\
   -EB                     assemble code for a big-endian cpu\n"));
   fprintf (stream, _("\
   -EL                     assemble code for a little-endian cpu\n"));
   fprintf (stream, _("\
-  -mrelax                  Enable relaxation\n"));
+  -mrelax                 enable relaxation\n"));
 
 }

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive
  2016-04-20 12:01                 ` Claudiu Zissulescu
@ 2016-04-20 12:58                   ` Andrew Burgess
  0 siblings, 0 replies; 17+ messages in thread
From: Andrew Burgess @ 2016-04-20 12:58 UTC (permalink / raw)
  To: Claudiu Zissulescu; +Cc: Nick Clifton, binutils, Cupertino Miranda

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-04-20 12:00:58 +0000]:

>  
> > I disagree and think it's a much worse idea.  My concern was never
> > about memory usage, my concern is data duplication, how long until we
> > have a bug where one alias is configured slightly differently to
> > another alias
> 
> This is one of the reason why macros have been introduced. For example:
> #define A7_CPU_TYPE(NAME) { #NAME,  ARC_OPCODE_ARC700,  bfd_mach_arc_arc700, E_ARC_MACH_ARC700,  0x00}
> 
> Then you just enumerate it in the structure:
> A7_CPU_TYPE (ARC700),
> A7_CPU_TYPE (A7),
> 
> So on, so forth...
> 
> Attached is a patch that shows this concept, though it needs to be
> validated:

Feel free to move forward with your version this patch as you clearly
prefer this approach.  You might want to consider if the help text is
clearer with all possible aliases listed, or if it is clearer to list
just a single name for each variant, clearly I favour the latter, but
I've been wrong before :)

Thanks,
Andrew

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-04-20 12:58 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-16 16:02 [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Andrew Burgess
2016-04-16 16:02 ` [PATCH 1/2] gas/arc: Support NPS400 in " Andrew Burgess
2016-04-16 16:02 ` [PATCH 2/2] gas/arc: Make .cpu directive case-insensitive Andrew Burgess
2016-04-17  8:35 ` [PATCH 0/2] [PUSHED/OBV] gas/arc: Add nps400 support to .cpu directive Claudiu Zissulescu
2016-04-17 20:35   ` Andrew Burgess
2016-04-17 21:36     ` Claudiu Zissulescu
2016-04-17 22:31       ` Andrew Burgess
2016-04-18 11:07         ` Nick Clifton
2016-04-19 18:52           ` Andrew Burgess
2016-04-20  9:18             ` Nick Clifton
2016-04-19 18:54           ` Andrew Burgess
2016-04-20  9:07             ` Claudiu Zissulescu
2016-04-20  9:25               ` Nick Clifton
2016-04-20 11:24               ` Andrew Burgess
2016-04-20 12:01                 ` Claudiu Zissulescu
2016-04-20 12:58                   ` Andrew Burgess
2016-04-20  9:23             ` Nick Clifton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).