diff mbox series

sim, sim/m32c, sim/rl78: Use getopt_long

Message ID 24e83e920d728237c4efe6f4720643d6fbbf1084.1666113214.git.research_trasio@irq.a4lg.com
State Committed
Headers
Series sim, sim/m32c, sim/rl78: Use getopt_long |

Commit Message

Tsukasa OI Oct. 18, 2022, 5:13 p.m. UTC 
  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(-)


base-commit: 04ea6b63141c43d9e96999e16917358088556fdd

Comments

Tom Tromey Oct. 21, 2022, 4:27 p.m. UTC | #1 
>>>>> 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
Tsukasa OI Oct. 24, 2022, 7:51 a.m. UTC | #2 
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
>
Tsukasa OI Oct. 25, 2022, 5:53 a.m. UTC | #3 
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
>>
Tom Tromey Oct. 25, 2022, 4:54 p.m. UTC | #4 
> 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
Tsukasa OI Oct. 25, 2022, 5:17 p.m. UTC | #5 
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
Tom Tromey Oct. 25, 2022, 7:42 p.m. UTC | #6 
> 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
diff mbox series

Patch

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)
 	{