[PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tsukasa OI]

Because of Binutils/GCC hack, getopt on GNU libc (2.25 or before) is
currently unusable on sim, causing a regression on CentOS 7.  Getting rid of
include/getopt.h is the best solution to avoid hacking but as a short-term
solution, this commit replaces calls to getopt to call getopt_long.
---
 sim/igen/igen.c | 6 ++++--
 sim/m32c/main.c | 5 ++++-
 sim/rl78/main.c | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/sim/igen/igen.c b/sim/igen/igen.c
index ba856401fa9..22cfd30ec43 100644
--- a/sim/igen/igen.c
+++ b/sim/igen/igen.c
@@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
+  struct option dummy_longopts = { 0 };
   lf *standard_out =
     lf_open ("-", "stdout", lf_omit_references, lf_is_text, "igen");
 
@@ -1162,8 +1163,9 @@ main (int argc, char **argv, char **envp)
       printf ("  -t <output-file>      output itable\n");
     }
 
-  while ((ch = getopt (argc, argv,
-		       "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x"))
+  while ((ch = getopt_long (argc, argv,
+			    "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x",
+			    &dummy_longopts, NULL))
 	 != -1)
     {
 #if 0  /* For debugging.  */
diff --git a/sim/m32c/main.c b/sim/m32c/main.c
index 958ca27ab2b..5560adea60a 100644
--- a/sim/m32c/main.c
+++ b/sim/m32c/main.c
@@ -29,6 +29,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <setjmp.h>
 #include <signal.h>
 #include <sys/types.h>
+#include <getopt.h>
 
 #ifdef HAVE_SYS_SOCKET_H
 #ifdef HAVE_NETINET_IN_H
@@ -135,12 +136,14 @@ main (int argc, char **argv)
 #ifdef HAVE_networking
   char *console_port_s = 0;
 #endif
+  struct option dummy_longopts = { 0 };
 
   setbuf (stdout, 0);
 
   in_gdb = 0;
 
-  while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
+  while ((o = getopt_long (argc, argv, "tc:vdm:C", &dummy_longopts, NULL))
+	 != -1)
     switch (o)
       {
       case 't':
diff --git a/sim/rl78/main.c b/sim/rl78/main.c
index c9459c7bc78..436f370a338 100644
--- a/sim/rl78/main.c
+++ b/sim/rl78/main.c
@@ -64,10 +64,12 @@ main (int argc, char **argv)
   int save_trace;
   bfd *prog;
   int rc;
+  struct option dummy_longopts = { 0 };
 
   xmalloc_set_program_name (argv[0]);
 
-  while ((o = getopt (argc, argv, "tvdr:D:M:")) != -1)
+  while ((o = getopt_long (argc, argv, "tvdr:D:M:", &dummy_longopts, NULL))
+	 != -1)
     {
       switch (o)
 	{

base-commit: 04ea6b63141c43d9e96999e16917358088556fdd
-- 
2.34.1

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tom Tromey]

>>>>> Tsukasa OI via Gdb-patches <gdb-patches@sourceware.org> writes:

> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or before) is
> currently unusable on sim, causing a regression on CentOS 7.  Getting rid of
> include/getopt.h is the best solution to avoid hacking

I don't think we want to do remove this file.  getopt_long is required
by a bunch of programs in the tree, and is part of libiberty -- but it's
not in the traditional getopt.h.

Using getopt_long seems like an improvement though.

> but as a short-term
> solution, this commit replaces calls to getopt to call getopt_long.

I notice you didn't update sim/ppc... why is that?
If those don't have the problem then perhaps we can just use that
solution, whatever it is.

Tom

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tsukasa OI]

On 2022/10/22 1:27, Tom Tromey wrote:
>>>>>> Tsukasa OI via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or before) is
>> currently unusable on sim, causing a regression on CentOS 7.  Getting rid of
>> include/getopt.h is the best solution to avoid hacking
> 
> I don't think we want to do remove this file.  getopt_long is required
> by a bunch of programs in the tree, and is part of libiberty -- but it's
> not in the traditional getopt.h.
> 
> Using getopt_long seems like an improvement though.

Glibc (2.25 or before; not too old to drop support) includes <getopt.h>
from <unistd.h> but this causes <unistd.h> to include Binutils/GCC's
include/getopt.h.  It means, using the filename "getopt.h" in an include
path (-I) itself is a risk.  The only reason it didn't cause a problem
is most of Binutils/GCC components used getopt_long and
getopt_long_only, not plain getopt.

After I started checking getopt in sim/configure (to reduce build
failure on Clang), it became a problem on sim because some sim files
actually use plain getopt.

c.f. Following thread under binutils
<https://sourceware.org/pipermail/binutils/2022-October/123543.html>

> 
>> but as a short-term
>> solution, this commit replaces calls to getopt to call getopt_long.
> 
> I notice you didn't update sim/ppc... why is that?

Because I simply missed that (I grepped "getopt (" and two occurrences
under sim/ppc uses "getopt(" without space between "getopt" and "(" ).

Thanks for spotting that and I'll fix it soon.

Thanks,
Tsukasa

> If those don't have the problem then perhaps we can just use that
> solution, whatever it is.
> 
> Tom
> 

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tsukasa OI]

On 2022/10/24 16:51, Tsukasa OI wrote:
> On 2022/10/22 1:27, Tom Tromey wrote:
>>>>>>> Tsukasa OI via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>>> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or before) is
>>> currently unusable on sim, causing a regression on CentOS 7.  Getting rid of
>>> include/getopt.h is the best solution to avoid hacking
>>
>> I don't think we want to do remove this file.  getopt_long is required
>> by a bunch of programs in the tree, and is part of libiberty -- but it's
>> not in the traditional getopt.h.
>>
>> Using getopt_long seems like an improvement though.
> 
> Glibc (2.25 or before; not too old to drop support) includes <getopt.h>
> from <unistd.h> but this causes <unistd.h> to include Binutils/GCC's
> include/getopt.h.  It means, using the filename "getopt.h" in an include
> path (-I) itself is a risk.  The only reason it didn't cause a problem
> is most of Binutils/GCC components used getopt_long and
> getopt_long_only, not plain getopt.
> 
> After I started checking getopt in sim/configure (to reduce build
> failure on Clang), it became a problem on sim because some sim files
> actually use plain getopt.
> 
> c.f. Following thread under binutils
> <https://sourceware.org/pipermail/binutils/2022-October/123543.html>
> 
>>
>>> but as a short-term
>>> solution, this commit replaces calls to getopt to call getopt_long.
>>
>> I notice you didn't update sim/ppc... why is that?
> 
> Because I simply missed that (I grepped "getopt (" and two occurrences
> under sim/ppc uses "getopt(" without space between "getopt" and "(" ).
> 
> Thanks for spotting that and I'll fix it soon.
> 
> Thanks,
> Tsukasa

There was another reason (I should have tested the branch before
submission and I wondered why I missed those getopt calls): for sim/ppc
files, it includes <getopt.h> before including project's config.h,
making getopt declarations not suppressed.

Replacing getopt with getopt_long will make this fact not relevant and
I'll just replace the function without modifying #include order (that
should be handled as a part of future "build with Clang" patchset).

Regards,
Tsukasa

> 
>> If those don't have the problem then perhaps we can just use that
>> solution, whatever it is.
>>
>> Tom
>>

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tom Tromey]

> Glibc (2.25 or before; not too old to drop support) includes <getopt.h>
> from <unistd.h> but this causes <unistd.h> to include Binutils/GCC's
> include/getopt.h.  It means, using the filename "getopt.h" in an include
> path (-I) itself is a risk.  The only reason it didn't cause a problem
> is most of Binutils/GCC components used getopt_long and
> getopt_long_only, not plain getopt.

What exactly was the problem?
It seems to me that we'd want to use the libiberty getopt.h, because
we're also linking against the linking getopt.

Tom

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tsukasa OI]

On 2022/10/26 1:54, Tom Tromey wrote:
>> Glibc (2.25 or before; not too old to drop support) includes <getopt.h>
>> from <unistd.h> but this causes <unistd.h> to include Binutils/GCC's
>> include/getopt.h.  It means, using the filename "getopt.h" in an include
>> path (-I) itself is a risk.  The only reason it didn't cause a problem
>> is most of Binutils/GCC components used getopt_long and
>> getopt_long_only, not plain getopt.
> 
> What exactly was the problem?
> It seems to me that we'd want to use the libiberty getopt.h, because
> we're also linking against the linking getopt.
> 
> Tom
> 

I've described it in the commit message of PATCH v2.
<https://sourceware.org/pipermail/gdb-patches/2022-October/193115.html>

If there's other questions, feel free to ask me.

Tsukasa

Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long

[Tom Tromey]

> I've described it in the commit message of PATCH v2.
> <https://sourceware.org/pipermail/gdb-patches/2022-October/193115.html>

I see, thank you.  This makes me appreciate your view better.

libiberty is funny because it requires certain configure checks and
definitions but doesn't provide a simple way to do them.  gdb has
libiberty.m4 but it is incomplete -- for example it doesn't do this
getopt check.

Something more robust would probably end up looking like gnulib... which
does have a getopt module, but which we don't use.  (And binutils
doesn't use gnulib at all.)

Anyway.  Switching to getopt_long seems like a fine workaround.
Programs should be using it anyway so that --help and --version can be
implemented.

Tom

[PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Tsukasa OI]

Because of Binutils/GCC hack, getopt on GNU libc (2.25 or earlier) is
currently unusable on sim, causing a regression on CentOS 7.

This is caused as follows:

1.  If HAVE_DECL_GETOPT is defined (getopt with known prototype is
    declared), a declaration of getopt in "include/getopt.h" is suppressed.
    The author started to define HAVE_DECL_GETOPT in sim with the commit
    340aa4f6872c ("sim: Check known getopt definition existence").
2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> to declare
    getopt function (only, not getopt_long or getopt_long_only) but it
    causes <unistd.h> to include Binutils/GCC's "include/getopt.h".
3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
    declare getopt by including <getopt.h>, "include/getopt.h" does not
    define one, causing getopt function unusable.

Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
best solution to avoid hacking but as a short-term solution, this commit
replaces getopt with getopt_long under sim/.
---
 sim/igen/igen.c | 6 ++++--
 sim/m32c/main.c | 5 ++++-
 sim/ppc/dgen.c  | 6 ++++--
 sim/ppc/igen.c  | 9 ++++++---
 sim/rl78/main.c | 4 +++-
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/sim/igen/igen.c b/sim/igen/igen.c
index ba856401fa9..22cfd30ec43 100644
--- a/sim/igen/igen.c
+++ b/sim/igen/igen.c
@@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
+  struct option dummy_longopts = { 0 };
   lf *standard_out =
     lf_open ("-", "stdout", lf_omit_references, lf_is_text, "igen");
 
@@ -1162,8 +1163,9 @@ main (int argc, char **argv, char **envp)
       printf ("  -t <output-file>      output itable\n");
     }
 
-  while ((ch = getopt (argc, argv,
-		       "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x"))
+  while ((ch = getopt_long (argc, argv,
+			    "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x",
+			    &dummy_longopts, NULL))
 	 != -1)
     {
 #if 0  /* For debugging.  */
diff --git a/sim/m32c/main.c b/sim/m32c/main.c
index 958ca27ab2b..5560adea60a 100644
--- a/sim/m32c/main.c
+++ b/sim/m32c/main.c
@@ -29,6 +29,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <setjmp.h>
 #include <signal.h>
 #include <sys/types.h>
+#include <getopt.h>
 
 #ifdef HAVE_SYS_SOCKET_H
 #ifdef HAVE_NETINET_IN_H
@@ -135,12 +136,14 @@ main (int argc, char **argv)
 #ifdef HAVE_networking
   char *console_port_s = 0;
 #endif
+  struct option dummy_longopts = { 0 };
 
   setbuf (stdout, 0);
 
   in_gdb = 0;
 
-  while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
+  while ((o = getopt_long (argc, argv, "tc:vdm:C", &dummy_longopts, NULL))
+	 != -1)
     switch (o)
       {
       case 't':
diff --git a/sim/ppc/dgen.c b/sim/ppc/dgen.c
index a1c1d56e8dc..da0b6446cfa 100644
--- a/sim/ppc/dgen.c
+++ b/sim/ppc/dgen.c
@@ -271,6 +271,7 @@ main(int argc,
 {
   lf_file_references file_references = lf_include_references;
   spreg_table *sprs = NULL;
+  struct option dummy_longopts = { 0 };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -284,8 +285,9 @@ main(int argc,
     printf("-L  Suppress cpp line numbering in output files\n");
   }
 
-
-  while ((ch = getopt(argc, argv, "hLsn:r:p:")) != -1) {
+  while ((ch = getopt_long (argc, argv, "hLsn:r:p:", &dummy_longopts, NULL))
+	 != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, ( optarg ? optarg : ""));
 #endif
diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
index 27b48638276..35d222df34b 100644
--- a/sim/ppc/igen.c
+++ b/sim/ppc/igen.c
@@ -351,6 +351,7 @@ main(int argc,
   filter *filters = NULL;
   insn_table *instructions = NULL;
   table_include *includes = NULL;
+  struct option dummy_longopts = { 0 };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -390,9 +391,11 @@ main(int argc,
     printf("  -f <output-file>      output support functions\n");
   }
 
-  while ((ch = getopt(argc, argv,
-		      "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:"))
-	 != -1) {
+  while (
+      (ch = getopt_long (argc, argv, "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:",
+			 &dummy_longopts, NULL))
+      != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, (optarg ? optarg : ""));
 #endif
diff --git a/sim/rl78/main.c b/sim/rl78/main.c
index c9459c7bc78..436f370a338 100644
--- a/sim/rl78/main.c
+++ b/sim/rl78/main.c
@@ -64,10 +64,12 @@ main (int argc, char **argv)
   int save_trace;
   bfd *prog;
   int rc;
+  struct option dummy_longopts = { 0 };
 
   xmalloc_set_program_name (argv[0]);
 
-  while ((o = getopt (argc, argv, "tvdr:D:M:")) != -1)
+  while ((o = getopt_long (argc, argv, "tvdr:D:M:", &dummy_longopts, NULL))
+	 != -1)
     {
       switch (o)
 	{

base-commit: a5a0a4fd0ff536a8dbd532864cfc095a306b678c
-- 
2.37.2

Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

On 25 Oct 2022 06:27, Tsukasa OI wrote:
> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or earlier) is
> currently unusable on sim, causing a regression on CentOS 7.
> 
> This is caused as follows:
> 
> 1.  If HAVE_DECL_GETOPT is defined (getopt with known prototype is
>     declared), a declaration of getopt in "include/getopt.h" is suppressed.
>     The author started to define HAVE_DECL_GETOPT in sim with the commit
>     340aa4f6872c ("sim: Check known getopt definition existence").
> 2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> to declare
>     getopt function (only, not getopt_long or getopt_long_only) but it
>     causes <unistd.h> to include Binutils/GCC's "include/getopt.h".
> 3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
>     declare getopt by including <getopt.h>, "include/getopt.h" does not
>     define one, causing getopt function unusable.
> 
> Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
> best solution to avoid hacking but as a short-term solution, this commit
> replaces getopt with getopt_long under sim/.
> ---
>  sim/igen/igen.c | 6 ++++--
>  sim/m32c/main.c | 5 ++++-
>  sim/ppc/dgen.c  | 6 ++++--
>  sim/ppc/igen.c  | 9 ++++++---
>  sim/rl78/main.c | 4 +++-
>  5 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/sim/igen/igen.c b/sim/igen/igen.c
> index ba856401fa9..22cfd30ec43 100644
> --- a/sim/igen/igen.c
> +++ b/sim/igen/igen.c
> @@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
>    char *real_file_name = NULL;
>    int is_header = 0;
>    int ch;
> +  struct option dummy_longopts = { 0 };

just call it "longopts" so we don't have to rename it in the future if we
decide to actually add long options.  comes up in the other files too.

otherwise lgtm.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Tsukasa OI]

On 2022/10/26 17:59, Mike Frysinger wrote:
> On 25 Oct 2022 06:27, Tsukasa OI wrote:
>> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or earlier) is
>> currently unusable on sim, causing a regression on CentOS 7.
>>
>> This is caused as follows:
>>
>> 1.  If HAVE_DECL_GETOPT is defined (getopt with known prototype is
>>     declared), a declaration of getopt in "include/getopt.h" is suppressed.
>>     The author started to define HAVE_DECL_GETOPT in sim with the commit
>>     340aa4f6872c ("sim: Check known getopt definition existence").
>> 2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> to declare
>>     getopt function (only, not getopt_long or getopt_long_only) but it
>>     causes <unistd.h> to include Binutils/GCC's "include/getopt.h".
>> 3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
>>     declare getopt by including <getopt.h>, "include/getopt.h" does not
>>     define one, causing getopt function unusable.
>>
>> Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
>> best solution to avoid hacking but as a short-term solution, this commit
>> replaces getopt with getopt_long under sim/.
>> ---
>>  sim/igen/igen.c | 6 ++++--
>>  sim/m32c/main.c | 5 ++++-
>>  sim/ppc/dgen.c  | 6 ++++--
>>  sim/ppc/igen.c  | 9 ++++++---
>>  sim/rl78/main.c | 4 +++-
>>  5 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/sim/igen/igen.c b/sim/igen/igen.c
>> index ba856401fa9..22cfd30ec43 100644
>> --- a/sim/igen/igen.c
>> +++ b/sim/igen/igen.c
>> @@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
>>    char *real_file_name = NULL;
>>    int is_header = 0;
>>    int ch;
>> +  struct option dummy_longopts = { 0 };
> 
> just call it "longopts" so we don't have to rename it in the future if we
> decide to actually add long options.  comes up in the other files too.
> 
> otherwise lgtm.
> -mike

To prepare actual long options, not just renaming, making them array of
struct option seems better.  Moving longopts out from the caller is
avoided for now (since it might not get big so that it requires option
definition outside a function).  That means...

Before: struct option dummy_longopts = { 0 };
After:  struct option longopts[] = { { 0 } };

I fixed like this and I'll submit PATCH v3 (with fix above and minor
commit message clarification) soon.  Finally, I can clean up the mess I
created.

Ah, a minor question to you, Mike.  Can I consider your "lgtm" as an
approval for specific area you are responsible?

Best regards,
Tsukasa

Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

On 26 Oct 2022 19:57, Tsukasa OI wrote:
> On 2022/10/26 17:59, Mike Frysinger wrote:
> > On 25 Oct 2022 06:27, Tsukasa OI wrote:
> >> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or earlier) is
> >> currently unusable on sim, causing a regression on CentOS 7.
> >>
> >> This is caused as follows:
> >>
> >> 1.  If HAVE_DECL_GETOPT is defined (getopt with known prototype is
> >>     declared), a declaration of getopt in "include/getopt.h" is suppressed.
> >>     The author started to define HAVE_DECL_GETOPT in sim with the commit
> >>     340aa4f6872c ("sim: Check known getopt definition existence").
> >> 2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> to declare
> >>     getopt function (only, not getopt_long or getopt_long_only) but it
> >>     causes <unistd.h> to include Binutils/GCC's "include/getopt.h".
> >> 3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
> >>     declare getopt by including <getopt.h>, "include/getopt.h" does not
> >>     define one, causing getopt function unusable.
> >>
> >> Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
> >> best solution to avoid hacking but as a short-term solution, this commit
> >> replaces getopt with getopt_long under sim/.
> >> ---
> >>  sim/igen/igen.c | 6 ++++--
> >>  sim/m32c/main.c | 5 ++++-
> >>  sim/ppc/dgen.c  | 6 ++++--
> >>  sim/ppc/igen.c  | 9 ++++++---
> >>  sim/rl78/main.c | 4 +++-
> >>  5 files changed, 21 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/sim/igen/igen.c b/sim/igen/igen.c
> >> index ba856401fa9..22cfd30ec43 100644
> >> --- a/sim/igen/igen.c
> >> +++ b/sim/igen/igen.c
> >> @@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
> >>    char *real_file_name = NULL;
> >>    int is_header = 0;
> >>    int ch;
> >> +  struct option dummy_longopts = { 0 };
> > 
> > just call it "longopts" so we don't have to rename it in the future if we
> > decide to actually add long options.  comes up in the other files too.
> > 
> > otherwise lgtm.
> > -mike
> 
> To prepare actual long options, not just renaming, making them array of
> struct option seems better.  Moving longopts out from the caller is
> avoided for now (since it might not get big so that it requires option
> definition outside a function).  That means...
> 
> Before: struct option dummy_longopts = { 0 };
> After:  struct option longopts[] = { { 0 } };
> 
> I fixed like this and I'll submit PATCH v3 (with fix above and minor
> commit message clarification) soon.  Finally, I can clean up the mess I
> created.

i would make it static const too in that case so it isn't constructed on
the stack ... it's just part of the rodata and loaded right away.

> Ah, a minor question to you, Mike.  Can I consider your "lgtm" as an
> approval for specific area you are responsible?

"lgtm" is equiv to "approved, please push when you're ready"
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Tsukasa OI]

On 2022/10/27 1:01, Mike Frysinger wrote:
> On 26 Oct 2022 19:57, Tsukasa OI wrote:
>> On 2022/10/26 17:59, Mike Frysinger wrote:
>>> On 25 Oct 2022 06:27, Tsukasa OI wrote:
>>>> Because of Binutils/GCC hack, getopt on GNU libc (2.25 or earlier) is
>>>> currently unusable on sim, causing a regression on CentOS 7.
>>>>
>>>> This is caused as follows:
>>>>
>>>> 1.  If HAVE_DECL_GETOPT is defined (getopt with known prototype is
>>>>     declared), a declaration of getopt in "include/getopt.h" is suppressed.
>>>>     The author started to define HAVE_DECL_GETOPT in sim with the commit
>>>>     340aa4f6872c ("sim: Check known getopt definition existence").
>>>> 2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> to declare
>>>>     getopt function (only, not getopt_long or getopt_long_only) but it
>>>>     causes <unistd.h> to include Binutils/GCC's "include/getopt.h".
>>>> 3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
>>>>     declare getopt by including <getopt.h>, "include/getopt.h" does not
>>>>     define one, causing getopt function unusable.
>>>>
>>>> Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
>>>> best solution to avoid hacking but as a short-term solution, this commit
>>>> replaces getopt with getopt_long under sim/.
>>>> ---
>>>>  sim/igen/igen.c | 6 ++++--
>>>>  sim/m32c/main.c | 5 ++++-
>>>>  sim/ppc/dgen.c  | 6 ++++--
>>>>  sim/ppc/igen.c  | 9 ++++++---
>>>>  sim/rl78/main.c | 4 +++-
>>>>  5 files changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/sim/igen/igen.c b/sim/igen/igen.c
>>>> index ba856401fa9..22cfd30ec43 100644
>>>> --- a/sim/igen/igen.c
>>>> +++ b/sim/igen/igen.c
>>>> @@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
>>>>    char *real_file_name = NULL;
>>>>    int is_header = 0;
>>>>    int ch;
>>>> +  struct option dummy_longopts = { 0 };
>>>
>>> just call it "longopts" so we don't have to rename it in the future if we
>>> decide to actually add long options.  comes up in the other files too.
>>>
>>> otherwise lgtm.
>>> -mike
>>
>> To prepare actual long options, not just renaming, making them array of
>> struct option seems better.  Moving longopts out from the caller is
>> avoided for now (since it might not get big so that it requires option
>> definition outside a function).  That means...
>>
>> Before: struct option dummy_longopts = { 0 };
>> After:  struct option longopts[] = { { 0 } };
>>
>> I fixed like this and I'll submit PATCH v3 (with fix above and minor
>> commit message clarification) soon.  Finally, I can clean up the mess I
>> created.
> 
> i would make it static const too in that case so it isn't constructed on
> the stack ... it's just part of the rodata and loaded right away.

That makes sense.  I submitted PATCH v4:
<https://sourceware.org/pipermail/gdb-patches/2022-October/193200.html>
and I will push this version in this weekend unless there's an objection
from someone.

> 
>> Ah, a minor question to you, Mike.  Can I consider your "lgtm" as an
>> approval for specific area you are responsible?
> 
> "lgtm" is equiv to "approved, please push when you're ready"
> -mike

Thanks!  I will push some of my "build with Clang" work with plain
"lgtm" response in this weekend.

Tsukasa

[PATCH v3] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Tsukasa OI]

Because of a Libiberty hack, getopt on GNU libc (2.25 or earlier) is
currently unusable on sim, causing a regression on CentOS 7.

This is caused as follows:

1.  If HAVE_DECL_GETOPT is defined (getopt declaration with known prototype
    is detected while configuration), a declaration of getopt in
    "include/getopt.h" is suppressed.
    The author started to define HAVE_DECL_GETOPT in sim with the commit
    340aa4f6872c ("sim: Check known getopt definition existence").
2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> with a
    special purpose macro defined to declare only getopt function but due
    to include path (not tested while configuration), it causes <unistd.h>
    to include Libiberty's "include/getopt.h".
3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
    declare getopt by including <getopt.h>, "include/getopt.h" does not do
    so, causing getopt function undeclared.

Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
best solution to avoid hacking but as a short-term solution, this commit
replaces getopt with getopt_long under sim/.
---
 sim/igen/igen.c | 6 ++++--
 sim/m32c/main.c | 5 ++++-
 sim/ppc/dgen.c  | 6 ++++--
 sim/ppc/igen.c  | 9 ++++++---
 sim/rl78/main.c | 4 +++-
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/sim/igen/igen.c b/sim/igen/igen.c
index ba856401fa9..06ee5fdfd10 100644
--- a/sim/igen/igen.c
+++ b/sim/igen/igen.c
@@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
+  struct option longopts[] = { { 0 } };
   lf *standard_out =
     lf_open ("-", "stdout", lf_omit_references, lf_is_text, "igen");
 
@@ -1162,8 +1163,9 @@ main (int argc, char **argv, char **envp)
       printf ("  -t <output-file>      output itable\n");
     }
 
-  while ((ch = getopt (argc, argv,
-		       "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x"))
+  while ((ch = getopt_long (argc, argv,
+			    "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x",
+			    longopts, NULL))
 	 != -1)
     {
 #if 0  /* For debugging.  */
diff --git a/sim/m32c/main.c b/sim/m32c/main.c
index 958ca27ab2b..5d6686d702e 100644
--- a/sim/m32c/main.c
+++ b/sim/m32c/main.c
@@ -29,6 +29,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <setjmp.h>
 #include <signal.h>
 #include <sys/types.h>
+#include <getopt.h>
 
 #ifdef HAVE_SYS_SOCKET_H
 #ifdef HAVE_NETINET_IN_H
@@ -135,12 +136,14 @@ main (int argc, char **argv)
 #ifdef HAVE_networking
   char *console_port_s = 0;
 #endif
+  struct option longopts[] = { { 0 } };
 
   setbuf (stdout, 0);
 
   in_gdb = 0;
 
-  while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
+  while ((o = getopt_long (argc, argv, "tc:vdm:C", longopts, NULL))
+	 != -1)
     switch (o)
       {
       case 't':
diff --git a/sim/ppc/dgen.c b/sim/ppc/dgen.c
index a1c1d56e8dc..277f3ab1d89 100644
--- a/sim/ppc/dgen.c
+++ b/sim/ppc/dgen.c
@@ -271,6 +271,7 @@ main(int argc,
 {
   lf_file_references file_references = lf_include_references;
   spreg_table *sprs = NULL;
+  struct option longopts[] = { { 0 } };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -284,8 +285,9 @@ main(int argc,
     printf("-L  Suppress cpp line numbering in output files\n");
   }
 
-
-  while ((ch = getopt(argc, argv, "hLsn:r:p:")) != -1) {
+  while ((ch = getopt_long (argc, argv, "hLsn:r:p:", longopts, NULL))
+	 != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, ( optarg ? optarg : ""));
 #endif
diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
index 27b48638276..8711394637a 100644
--- a/sim/ppc/igen.c
+++ b/sim/ppc/igen.c
@@ -351,6 +351,7 @@ main(int argc,
   filter *filters = NULL;
   insn_table *instructions = NULL;
   table_include *includes = NULL;
+  struct option longopts[] = { { 0 } };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -390,9 +391,11 @@ main(int argc,
     printf("  -f <output-file>      output support functions\n");
   }
 
-  while ((ch = getopt(argc, argv,
-		      "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:"))
-	 != -1) {
+  while (
+      (ch = getopt_long (argc, argv, "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:",
+			 longopts, NULL))
+      != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, (optarg ? optarg : ""));
 #endif
diff --git a/sim/rl78/main.c b/sim/rl78/main.c
index c9459c7bc78..bc8dd1a0697 100644
--- a/sim/rl78/main.c
+++ b/sim/rl78/main.c
@@ -64,10 +64,12 @@ main (int argc, char **argv)
   int save_trace;
   bfd *prog;
   int rc;
+  struct option longopts[] = { { 0 } };
 
   xmalloc_set_program_name (argv[0]);
 
-  while ((o = getopt (argc, argv, "tvdr:D:M:")) != -1)
+  while ((o = getopt_long (argc, argv, "tvdr:D:M:", longopts, NULL))
+	 != -1)
     {
       switch (o)
 	{

base-commit: 99033a63c7ba0997ef737392eef15337d6783078
-- 
2.37.2

[PATCH v4] sim, sim/{m32c,ppc,rl78}: Use getopt_long

[Tsukasa OI]

Because of a Libiberty hack, getopt on GNU libc (2.25 or earlier) is
currently unusable on sim, causing a regression on CentOS 7.

This is caused as follows:

1.  If HAVE_DECL_GETOPT is defined (getopt declaration with known prototype
    is detected while configuration), a declaration of getopt in
    "include/getopt.h" is suppressed.
    The author started to define HAVE_DECL_GETOPT in sim with the commit
    340aa4f6872c ("sim: Check known getopt definition existence").
2.  GNU libc (2.25 or earlier)'s <unistd.h> includes <getopt.h> with a
    special purpose macro defined to declare only getopt function but due
    to include path (not tested while configuration), it causes <unistd.h>
    to include Libiberty's "include/getopt.h".
3.  If both 1. and 2. are satisfied, despite that <unistd.h> tries to
    declare getopt by including <getopt.h>, "include/getopt.h" does not do
    so, causing getopt function undeclared.

Getting rid of "include/getopt.h" (e.g. renaming this header file) is the
best solution to avoid hacking but as a short-term solution, this commit
replaces getopt with getopt_long under sim/.
---
 sim/igen/igen.c | 6 ++++--
 sim/m32c/main.c | 5 ++++-
 sim/ppc/dgen.c  | 6 ++++--
 sim/ppc/igen.c  | 9 ++++++---
 sim/rl78/main.c | 4 +++-
 5 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/sim/igen/igen.c b/sim/igen/igen.c
index ba856401fa9..b0a07743c24 100644
--- a/sim/igen/igen.c
+++ b/sim/igen/igen.c
@@ -989,6 +989,7 @@ main (int argc, char **argv, char **envp)
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
+  static const struct option longopts[] = { { 0 } };
   lf *standard_out =
     lf_open ("-", "stdout", lf_omit_references, lf_is_text, "igen");
 
@@ -1162,8 +1163,9 @@ main (int argc, char **argv, char **envp)
       printf ("  -t <output-file>      output itable\n");
     }
 
-  while ((ch = getopt (argc, argv,
-		       "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x"))
+  while ((ch = getopt_long (argc, argv,
+			    "B:D:F:G:H:I:M:N:P:T:W:o:k:i:n:hc:d:e:m:r:s:t:f:x",
+			    longopts, NULL))
 	 != -1)
     {
 #if 0  /* For debugging.  */
diff --git a/sim/m32c/main.c b/sim/m32c/main.c
index 958ca27ab2b..5ed912ae7bc 100644
--- a/sim/m32c/main.c
+++ b/sim/m32c/main.c
@@ -29,6 +29,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <setjmp.h>
 #include <signal.h>
 #include <sys/types.h>
+#include <getopt.h>
 
 #ifdef HAVE_SYS_SOCKET_H
 #ifdef HAVE_NETINET_IN_H
@@ -135,12 +136,14 @@ main (int argc, char **argv)
 #ifdef HAVE_networking
   char *console_port_s = 0;
 #endif
+  static const struct option longopts[] = { { 0 } };
 
   setbuf (stdout, 0);
 
   in_gdb = 0;
 
-  while ((o = getopt (argc, argv, "tc:vdm:C")) != -1)
+  while ((o = getopt_long (argc, argv, "tc:vdm:C", longopts, NULL))
+	 != -1)
     switch (o)
       {
       case 't':
diff --git a/sim/ppc/dgen.c b/sim/ppc/dgen.c
index a1c1d56e8dc..caafe07a6aa 100644
--- a/sim/ppc/dgen.c
+++ b/sim/ppc/dgen.c
@@ -271,6 +271,7 @@ main(int argc,
 {
   lf_file_references file_references = lf_include_references;
   spreg_table *sprs = NULL;
+  static const struct option longopts[] = { { 0 } };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -284,8 +285,9 @@ main(int argc,
     printf("-L  Suppress cpp line numbering in output files\n");
   }
 
-
-  while ((ch = getopt(argc, argv, "hLsn:r:p:")) != -1) {
+  while ((ch = getopt_long (argc, argv, "hLsn:r:p:", longopts, NULL))
+	 != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, ( optarg ? optarg : ""));
 #endif
diff --git a/sim/ppc/igen.c b/sim/ppc/igen.c
index 27b48638276..445afb9ee00 100644
--- a/sim/ppc/igen.c
+++ b/sim/ppc/igen.c
@@ -351,6 +351,7 @@ main(int argc,
   filter *filters = NULL;
   insn_table *instructions = NULL;
   table_include *includes = NULL;
+  static const struct option longopts[] = { { 0 } };
   char *real_file_name = NULL;
   int is_header = 0;
   int ch;
@@ -390,9 +391,11 @@ main(int argc,
     printf("  -f <output-file>      output support functions\n");
   }
 
-  while ((ch = getopt(argc, argv,
-		      "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:"))
-	 != -1) {
+  while (
+      (ch = getopt_long (argc, argv, "F:EI:RSLJT:CB:H:N:o:k:i:n:hc:d:m:s:t:f:",
+			 longopts, NULL))
+      != -1)
+  {
 #if 0  /* For debugging.  */
     fprintf(stderr, "\t-%c %s\n", ch, (optarg ? optarg : ""));
 #endif
diff --git a/sim/rl78/main.c b/sim/rl78/main.c
index c9459c7bc78..ea6ddaaa6c3 100644
--- a/sim/rl78/main.c
+++ b/sim/rl78/main.c
@@ -64,10 +64,12 @@ main (int argc, char **argv)
   int save_trace;
   bfd *prog;
   int rc;
+  static const struct option longopts[] = { { 0 } };
 
   xmalloc_set_program_name (argv[0]);
 
-  while ((o = getopt (argc, argv, "tvdr:D:M:")) != -1)
+  while ((o = getopt_long (argc, argv, "tvdr:D:M:", longopts, NULL))
+	 != -1)
     {
       switch (o)
 	{

base-commit: ecb58b32cdeb9789f9bc7c2037cb001d9a729f83
-- 
2.37.2