* [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long [not found] <3b7e769f-b5e9-4049-786f-d00d997f0280@irq.a4lg.com> @ 2022-10-18 17:13 ` Tsukasa OI 2022-10-21 16:27 ` Tom Tromey 2022-10-25 6:27 ` [PATCH v2] sim, sim/{m32c,ppc,rl78}: " Tsukasa OI 0 siblings, 2 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-18 17:13 UTC (permalink / raw) To: Tsukasa OI, Tom de Vries, Andrew Burgess, Mike Frysinger; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-18 17:13 ` [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long Tsukasa OI @ 2022-10-21 16:27 ` Tom Tromey 2022-10-24 7:51 ` Tsukasa OI 2022-10-25 6:27 ` [PATCH v2] sim, sim/{m32c,ppc,rl78}: " Tsukasa OI 1 sibling, 1 reply; 14+ messages in thread From: Tom Tromey @ 2022-10-21 16:27 UTC (permalink / raw) To: Tsukasa OI via Gdb-patches Cc: Tsukasa OI, Tom de Vries, Andrew Burgess, Mike Frysinger >>>>> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-21 16:27 ` Tom Tromey @ 2022-10-24 7:51 ` Tsukasa OI 2022-10-25 5:53 ` Tsukasa OI 2022-10-25 16:54 ` Tom Tromey 0 siblings, 2 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-24 7:51 UTC (permalink / raw) To: Tom Tromey, gdb-patches 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-24 7:51 ` Tsukasa OI @ 2022-10-25 5:53 ` Tsukasa OI 2022-10-25 16:54 ` Tom Tromey 1 sibling, 0 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-25 5:53 UTC (permalink / raw) To: Tom Tromey, gdb-patches 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 >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-24 7:51 ` Tsukasa OI 2022-10-25 5:53 ` Tsukasa OI @ 2022-10-25 16:54 ` Tom Tromey 2022-10-25 17:17 ` Tsukasa OI 1 sibling, 1 reply; 14+ messages in thread From: Tom Tromey @ 2022-10-25 16:54 UTC (permalink / raw) To: Tsukasa OI; +Cc: Tom Tromey, gdb-patches > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-25 16:54 ` Tom Tromey @ 2022-10-25 17:17 ` Tsukasa OI 2022-10-25 19:42 ` Tom Tromey 0 siblings, 1 reply; 14+ messages in thread From: Tsukasa OI @ 2022-10-25 17:17 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long 2022-10-25 17:17 ` Tsukasa OI @ 2022-10-25 19:42 ` Tom Tromey 0 siblings, 0 replies; 14+ messages in thread From: Tom Tromey @ 2022-10-25 19:42 UTC (permalink / raw) To: Tsukasa OI; +Cc: Tom Tromey, gdb-patches > 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-18 17:13 ` [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long Tsukasa OI 2022-10-21 16:27 ` Tom Tromey @ 2022-10-25 6:27 ` Tsukasa OI 2022-10-26 8:59 ` Mike Frysinger 2022-10-26 10:59 ` [PATCH v3] " Tsukasa OI 1 sibling, 2 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-25 6:27 UTC (permalink / raw) To: Tsukasa OI, Tom de Vries, Andrew Burgess, Mike Frysinger, Pedro Alves Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-25 6:27 ` [PATCH v2] sim, sim/{m32c,ppc,rl78}: " Tsukasa OI @ 2022-10-26 8:59 ` Mike Frysinger 2022-10-26 10:57 ` Tsukasa OI 2022-10-26 10:59 ` [PATCH v3] " Tsukasa OI 1 sibling, 1 reply; 14+ messages in thread From: Mike Frysinger @ 2022-10-26 8:59 UTC (permalink / raw) To: Tsukasa OI; +Cc: Tom de Vries, Andrew Burgess, Pedro Alves, gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-26 8:59 ` Mike Frysinger @ 2022-10-26 10:57 ` Tsukasa OI 2022-10-26 16:01 ` Mike Frysinger 0 siblings, 1 reply; 14+ messages in thread From: Tsukasa OI @ 2022-10-26 10:57 UTC (permalink / raw) To: gdb-patches, Mike Frysinger 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-26 10:57 ` Tsukasa OI @ 2022-10-26 16:01 ` Mike Frysinger 2022-10-27 1:29 ` Tsukasa OI 0 siblings, 1 reply; 14+ messages in thread From: Mike Frysinger @ 2022-10-26 16:01 UTC (permalink / raw) To: Tsukasa OI; +Cc: gdb-patches [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-26 16:01 ` Mike Frysinger @ 2022-10-27 1:29 ` Tsukasa OI 0 siblings, 0 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-27 1:29 UTC (permalink / raw) To: Mike Frysinger, gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-25 6:27 ` [PATCH v2] sim, sim/{m32c,ppc,rl78}: " Tsukasa OI 2022-10-26 8:59 ` Mike Frysinger @ 2022-10-26 10:59 ` Tsukasa OI 2022-10-27 1:23 ` [PATCH v4] " Tsukasa OI 1 sibling, 1 reply; 14+ messages in thread From: Tsukasa OI @ 2022-10-26 10:59 UTC (permalink / raw) To: Tsukasa OI, Mike Frysinger, Andrew Burgess, Tom de Vries, Pedro Alves Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] sim, sim/{m32c,ppc,rl78}: Use getopt_long 2022-10-26 10:59 ` [PATCH v3] " Tsukasa OI @ 2022-10-27 1:23 ` Tsukasa OI 0 siblings, 0 replies; 14+ messages in thread From: Tsukasa OI @ 2022-10-27 1:23 UTC (permalink / raw) To: Tsukasa OI, Mike Frysinger, Andrew Burgess, Tom de Vries, Pedro Alves Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-10-27 1:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <3b7e769f-b5e9-4049-786f-d00d997f0280@irq.a4lg.com> 2022-10-18 17:13 ` [PATCH] sim, sim/m32c, sim/rl78: Use getopt_long Tsukasa OI 2022-10-21 16:27 ` Tom Tromey 2022-10-24 7:51 ` Tsukasa OI 2022-10-25 5:53 ` Tsukasa OI 2022-10-25 16:54 ` Tom Tromey 2022-10-25 17:17 ` Tsukasa OI 2022-10-25 19:42 ` Tom Tromey 2022-10-25 6:27 ` [PATCH v2] sim, sim/{m32c,ppc,rl78}: " Tsukasa OI 2022-10-26 8:59 ` Mike Frysinger 2022-10-26 10:57 ` Tsukasa OI 2022-10-26 16:01 ` Mike Frysinger 2022-10-27 1:29 ` Tsukasa OI 2022-10-26 10:59 ` [PATCH v3] " Tsukasa OI 2022-10-27 1:23 ` [PATCH v4] " Tsukasa OI
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).