Re: [PATCH v3 4/5] sim: Check known getopt definition existence

Re: [PATCH v3 4/5] sim: Check known getopt definition existence

[Tsukasa OI]

On 2022/10/13 1:28, Tom de Vries wrote:
> On 10/6/22 08:43, Tsukasa OI via Gdb-patches wrote:
>> Clang generates a warning if there is a function declaration/definition
>> with zero arguments.  Such declarations/definitions without a
>> prototype (an
>> argument list) are deprecated forms of indefinite arguments
>> ("-Wdeprecated-non-prototype").  On the default configuration, it
>> causes a
>> build failure (unless "--disable-werror" is specified).
>>
>> include/getopt.h defines some getopt function definitions but one of them
>> has a form "extern int getopt ();".  If this form is selected in
>> include/getopt.h, Clang generates a warning and the build fails by
>> default.
>>
>> In really old environments, this getopt definition with no arguments is
>> necessary (because the definition may change between environments).
>> However, this definition is now a cause of problems on modern
>> environments.
>>
>> A good news is, this definition is not always selected (e.g. if used by
>> binutils/*.c).  This is because configuration scripts of binutils, gas,
>> gprof and ld tries to find known definition of getopt function is used
>> and
>> defines HAVE_DECL_GETOPT macro.  If this macro is defined when
>> getopt.h is
>> included, a good form of getopt is used and Clang won't generate
>> warnings.
>>
>> This commit adds a modified portion of ld/configure.ac to find the known
>> getopt definition.  If we could find one (and we *will* in most modern
>> environments), we don't need to rely on the deprecated definition.
> 
> I'm guessing this cause the build breakage on buildbot gdb-centos-x86_64 .
> 
> https://builder.sourceware.org/buildbot/#/builders/71/builds/1392
> 
> Thanks,
> - Tom

Hi Tom,

I finally found a cause.  First of all, this is reproduced with
following configurations (examples are selected to minimize time to
reproduce):

-   CentOS 7 (x86_64; with GNU libc 2.17)
-   "make all-sim" with following configurations (for example):
    -   --target=m32c-unknown-elf
    -   --target=rl78-unknown-elf

And it was true that my commit (as you pointed out) was the initial bad
commit but the real cause was much complex than I expected.  The reason
I could not initially reproduce the issue was because it required GNU
libc <= 2.25 to reproduce.

An interesting fact is... standard unistd.h on GNU libc <= 2.25 includes
<getopt.h> (with __need_getopt macro defined) but it actually includes
$(srcdir)/include/getopt.h, not getopt.h in GNU libc.  Since my change
defined HAVE_DECL_GETOPT to 1 on GNU libc-based environment and
declaration of the getopt function is suppressed, causing an error.

On GNU libc >= 2.26, unistd.h includes <bits/getopt_posix.h> without
defining __need_getopt and that's why this error is suppressed on newer
GNU libc-based environments.


Possible reason why this bug is missed is, there are not so many getopt
callers (many calls getopt_long or getopt_long_only, not getopt).

True getopt callers on Binutils/GDB/GCC are following:

-   M32C simulator (affected by my change)
-   RL78 simulator (affected by my change)
-   gprofng (getopt declaration not checked by gprofng/configure)

... yes, GCC (entirely) and the most of Binutils components do not
depend on getopt function anymore.  This fact explains why this bug is
not discovered so long.


The true fix to this is going to be applied to include/getopt.h
(Binutils) to detect this include path on GNU libc <= 2.25.

Aside from this, some sim source files need to be changed (to minimize
problems even further).  I'll submit these patchsets soon.

Thanks,
Tsukasa

> 
> 
>> ---
>>   sim/config.h.in  |  3 +++
>>   sim/configure    | 32 ++++++++++++++++++++++++++++++++
>>   sim/configure.ac | 10 ++++++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/sim/config.h.in b/sim/config.h.in
>> index 84c363c0aec..9a94b289e46 100644
>> --- a/sim/config.h.in
>> +++ b/sim/config.h.in
>> @@ -41,6 +41,9 @@
>>   /* Define to 1 if you have the `chmod' function. */
>>   #undef HAVE_CHMOD
>>   +/* Is the prototype for getopt in <unistd.h> in the expected
>> format? */
>> +#undef HAVE_DECL_GETOPT
>> +
>>   /* Define to 1 if you have the declaration of `tzname', and to 0 if
>> you don't.
>>      */
>>   #undef HAVE_DECL_TZNAME
>> diff --git a/sim/configure b/sim/configure
>> index 75d1935df38..dac7f085be1 100755
>> --- a/sim/configure
>> +++ b/sim/configure
>> @@ -16428,6 +16428,38 @@ $as_echo "${WARN_CFLAGS} ${WERROR_CFLAGS}"
>> >&6; }
>>   fi
>>     +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for a known
>> getopt prototype in unistd.h" >&5
>> +$as_echo_n "checking for a known getopt prototype in unistd.h... "
>> >&6; }
>> +if ${sim_cv_decl_getopt_unistd_h+:} false; then :
>> +  $as_echo_n "(cached) " >&6
>> +else
>> +  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>> +/* end confdefs.h.  */
>> +#include <unistd.h>
>> +int
>> +main ()
>> +{
>> +extern int getopt (int, char *const*, const char *);
>> +  ;
>> +  return 0;
>> +}
>> +_ACEOF
>> +if ac_fn_c_try_compile "$LINENO"; then :
>> +  sim_cv_decl_getopt_unistd_h=yes
>> +else
>> +  sim_cv_decl_getopt_unistd_h=no
>> +fi
>> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
>> +fi
>> +
>> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
>> $sim_cv_decl_getopt_unistd_h" >&5
>> +$as_echo "$sim_cv_decl_getopt_unistd_h" >&6; }
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +
>> +$as_echo "#define HAVE_DECL_GETOPT 1" >>confdefs.h
>> +
>> +fi
>> +
>>       diff --git a/sim/configure.ac b/sim/configure.ac
>> index 66a1020efe0..be0cfdbea32 100644
>> --- a/sim/configure.ac
>> +++ b/sim/configure.ac
>> @@ -177,6 +177,16 @@ SIM_AC_OPTION_STDIO
>>   SIM_AC_OPTION_TRACE
>>   SIM_AC_OPTION_WARNINGS
>>   +AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
>> +AC_CACHE_VAL(sim_cv_decl_getopt_unistd_h,
>> +[AC_COMPILE_IFELSE([AC_LANG_PROGRAM([#include <unistd.h>], [extern
>> int getopt (int, char *const*, const char *);])],
>> +sim_cv_decl_getopt_unistd_h=yes, sim_cv_decl_getopt_unistd_h=no)])
>> +AC_MSG_RESULT($sim_cv_decl_getopt_unistd_h)
>> +if test $sim_cv_decl_getopt_unistd_h = yes; then
>> +  AC_DEFINE([HAVE_DECL_GETOPT], 1,
>> +        [Is the prototype for getopt in <unistd.h> in the expected
>> format?])
>> +fi
>> +
>>   dnl These are unfortunate.  They are conditionally called by other
>> sim macros
>>   dnl but always used by common/Make-common.in.  So we have to subst
>> here even
>>   dnl when the rest of the code is in the respective macros.  Once we
>> merge the
> 

[PATCH] include: Declare getopt function on old GNU libc

[Tsukasa OI]

On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
defined.  That <getopt.h> is intended to be a part of GNU libc but
<unistd.h> actually includes include/getopt.h in this project.

If HAVE_DECL_GETOPT is defined to 1 and include/getopt.h is included from
GNU libc's <unistd.h>, declaration of getopt is suppressed, causing errors
on getopt callers.  This issue is possibly hidden so long because there are
not so many true getopt callers in Binutils, GDB and GCC.  Still, this issue
needs to be fixed for following components:

-   Binutils: gprofng
    (not currently affected due to the configuration script but will be)
-   GDB (sim): M32C simulator
-   GDB (sim): RL78 simulator

To avoid not defining proper getopt declaration, we have to check
__need_getopt macro to detect this include path.  With this commit, even if
HAVE_DECL_GETOPT is 1, getopt is declared if:

-   The standard C library is GNU libc and
-   __need_getopt macro is defined (<unistd.h> includes <getopt.h> to
    declare getopt function).

include/ChangeLog:

	* getopt.h: Detect special include path on GNU libc 2.25 or older
	to prevent not declaring getopt function when necessary.
---
 include/getopt.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/getopt.h b/include/getopt.h
index d8103f97483..e941b811ace 100644
--- a/include/getopt.h
+++ b/include/getopt.h
@@ -104,7 +104,14 @@ struct option
    declaration without arguments.  If it is 0, we checked and failed
    to find the declaration so provide a fully prototyped one.  If it
    is 1, we found it so don't provide any declaration at all.  */
-#if !HAVE_DECL_GETOPT
+/* On GNU libc <= 2,25, <unistd.h> includes <getopt.h> with __need_getopt
+   macro defined.  That <getopt.h> is intended to be a part of GNU libc
+   but <unistd.h> actually includes THIS getopt.h.  If HAVE_DECL_GETOPT is
+   defined to 1 and this file is included from GNU libc's <unistd.h>,
+   declaration of getopt is suppressed, causing errors on getopt callers.
+   To avoid not defining proper getopt declaration, we have to check
+   __need_getopt macro when built with GNU libc to detect this include path.  */
+#if !HAVE_DECL_GETOPT || (defined (__GNU_LIBRARY__) && defined (__need_getopt))
 #if defined (__GNU_LIBRARY__) || defined (HAVE_DECL_GETOPT)
 /* Many other libraries have conflicting prototypes for getopt, with
    differences in the consts, in unistd.h.  To avoid compilation

base-commit: 927b2f4caf46e5ca49684c9a52a9786425c60fa2
-- 
2.34.1

Re: [PATCH] include: Declare getopt function on old GNU libc

[Pedro Alves]

On 2022-10-13 11:11 a.m., Tsukasa OI via Binutils wrote:
> On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
> defined.  That <getopt.h> is intended to be a part of GNU libc but
> <unistd.h> actually includes include/getopt.h in this project.

Messy.

Do we really still need this getopt.h header?

gnulib, at:

 https://www.gnu.org/software/gnulib/manual/html_node/getopt_002eh.html

says:

  "This header file is missing on some platforms: AIX 5.1, HP-UX 11, MSVC 14."

AIX 5.1 is from 2001.
HP-UX 11 is from 1997.
We don't support building with MSVC AFAIK.

Can't we just get rid of it?

Re: [PATCH] include: Declare getopt function on old GNU libc

[Tom de Vries]

On 10/13/22 12:11, Tsukasa OI wrote:
> On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
> defined.  That <getopt.h> is intended to be a part of GNU libc but
> <unistd.h> actually includes include/getopt.h in this project.
> 
> If HAVE_DECL_GETOPT is defined to 1 and include/getopt.h is included from
> GNU libc's <unistd.h>, declaration of getopt is suppressed, causing errors
> on getopt callers.  This issue is possibly hidden so long because there are
> not so many true getopt callers in Binutils, GDB and GCC.  Still, this issue
> needs to be fixed for following components:
> 
> -   Binutils: gprofng
>      (not currently affected due to the configuration script but will be)
> -   GDB (sim): M32C simulator
> -   GDB (sim): RL78 simulator
> 
> To avoid not defining proper getopt declaration, we have to check
> __need_getopt macro to detect this include path.  With this commit, even if
> HAVE_DECL_GETOPT is 1, getopt is declared if:
> 
> -   The standard C library is GNU libc and
> -   __need_getopt macro is defined (<unistd.h> includes <getopt.h> to
>      declare getopt function).
> 

I've put this into the gdb try-buildbot, and it fixes the centos 
regression : 
https://builder.sourceware.org/buildbot/#/builders/118/builds/17 .

I didn't get emails to that effect for some reason, but visual 
inspection at https://builder.sourceware.org/ of the try build bots at 
gdb shows all green.

Thanks,
- Tom

> include/ChangeLog:
> 
> 	* getopt.h: Detect special include path on GNU libc 2.25 or older
> 	to prevent not declaring getopt function when necessary.
> ---
>   include/getopt.h | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/getopt.h b/include/getopt.h
> index d8103f97483..e941b811ace 100644
> --- a/include/getopt.h
> +++ b/include/getopt.h
> @@ -104,7 +104,14 @@ struct option
>      declaration without arguments.  If it is 0, we checked and failed
>      to find the declaration so provide a fully prototyped one.  If it
>      is 1, we found it so don't provide any declaration at all.  */
> -#if !HAVE_DECL_GETOPT
> +/* On GNU libc <= 2,25, <unistd.h> includes <getopt.h> with __need_getopt
> +   macro defined.  That <getopt.h> is intended to be a part of GNU libc
> +   but <unistd.h> actually includes THIS getopt.h.  If HAVE_DECL_GETOPT is
> +   defined to 1 and this file is included from GNU libc's <unistd.h>,
> +   declaration of getopt is suppressed, causing errors on getopt callers.
> +   To avoid not defining proper getopt declaration, we have to check
> +   __need_getopt macro when built with GNU libc to detect this include path.  */
> +#if !HAVE_DECL_GETOPT || (defined (__GNU_LIBRARY__) && defined (__need_getopt))
>   #if defined (__GNU_LIBRARY__) || defined (HAVE_DECL_GETOPT)
>   /* Many other libraries have conflicting prototypes for getopt, with
>      differences in the consts, in unistd.h.  To avoid compilation
> 
> base-commit: 927b2f4caf46e5ca49684c9a52a9786425c60fa2

Re: [PATCH] include: Declare getopt function on old GNU libc

[Tsukasa OI]

On 2022/10/13 20:59, Pedro Alves wrote:
> On 2022-10-13 11:11 a.m., Tsukasa OI via Binutils wrote:
>> On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
>> defined.  That <getopt.h> is intended to be a part of GNU libc but
>> <unistd.h> actually includes include/getopt.h in this project.
> 
> Messy.

Well, I have to agree.

> 
> Do we really still need this getopt.h header?
> 
> gnulib, at:
> 
>  https://www.gnu.org/software/gnulib/manual/html_node/getopt_002eh.html
> 
> says:
> 
>   "This header file is missing on some platforms: AIX 5.1, HP-UX 11, MSVC 14."
> 
> AIX 5.1 is from 2001.
> HP-UX 11 is from 1997.
> We don't support building with MSVC AFAIK.
> 
> Can't we just get rid of it?
> 

I feel this is too unsafe to do that (unless you assume getopt_long and
getopt_long_only are always available on the system).

Even if this patch is unacceptable, I don't want to revert previous
changes to sim/configure{,.ac} either (it's necessary to prevent a build
failure with Clang).  It's also a hack but to stop using getopt
(entirely) may be an option, changing following files:

-   sim/igen/igen.c
-   sim/m32c/main.c
-   sim/rl78/main.c

I mean, we could replace getopt with getopt_long plus dummy longopts.
This way, CentOS (7) regression will (also) be gone.

What do you think?

Thanks,
Tsukasa

Re: [PATCH] include: Declare getopt function on old GNU libc

[Pedro Alves]

On 2022-10-16 2:12 p.m., Tsukasa OI wrote:
> On 2022/10/13 20:59, Pedro Alves wrote:
>> On 2022-10-13 11:11 a.m., Tsukasa OI via Binutils wrote:
>>> On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
>>> defined.  That <getopt.h> is intended to be a part of GNU libc but
>>> <unistd.h> actually includes include/getopt.h in this project.
>>
>> Messy.
> 
> Well, I have to agree.
> 
>>
>> Do we really still need this getopt.h header?
>>
>> gnulib, at:
>>
>>  https://www.gnu.org/software/gnulib/manual/html_node/getopt_002eh.html
>>
>> says:
>>
>>   "This header file is missing on some platforms: AIX 5.1, HP-UX 11, MSVC 14."
>>
>> AIX 5.1 is from 2001.
>> HP-UX 11 is from 1997.
>> We don't support building with MSVC AFAIK.
>>
>> Can't we just get rid of it?
>>
> 
> I feel this is too unsafe to do that

Why?  Are you aware of any host system that people actually build binutils on
that doesn't have a proper getopt declaration?

> (unless you assume getopt_long and
> getopt_long_only are always available on the system).

getopt is supposed to be declared in unistd.h.  The .c files that
use getopt (not the GNU extensions) could just switch to including
that one.

Even if there is such a system, I would think that a better fix would
be to rename include/getopt.h to something else, and have that header include
<getopt.h> and/or do whatever else needed to pick the right declarations on the system.
A standard header replacement that doesn't #include_next the original is just asking
for trouble, like we've run into...

> 
> Even if this patch is unacceptable, I don't want to revert previous
> changes to sim/configure{,.ac} either (it's necessary to prevent a build
> failure with Clang).  

It's only necessary because of this hacky include/getopt.h header existing, no?.
Why would you want to keep the configure.ac bits if the hacky header with the
unprototyped declaration doesn't exist any more?

Thanks,
Pedro Alves

> It's also a hack but to stop using getopt
> (entirely) may be an option, changing following files:
> 
> -   sim/igen/igen.c
> -   sim/m32c/main.c
> -   sim/rl78/main.c
> 
> I mean, we could replace getopt with getopt_long plus dummy longopts.
> This way, CentOS (7) regression will (also) be gone.
> 
> What do you think?
> 
> Thanks,
> Tsukasa
> 

Re: [PATCH] include: Declare getopt function on old GNU libc

[Tsukasa OI]

On 2022/10/17 20:52, Pedro Alves wrote:
> On 2022-10-16 2:12 p.m., Tsukasa OI wrote:
>> On 2022/10/13 20:59, Pedro Alves wrote:
>>> On 2022-10-13 11:11 a.m., Tsukasa OI via Binutils wrote:
>>>> On GNU libc <= 2.25, <unistd.h> includes <getopt.h> with __need_getopt macro
>>>> defined.  That <getopt.h> is intended to be a part of GNU libc but
>>>> <unistd.h> actually includes include/getopt.h in this project.
>>>
>>> Messy.
>>
>> Well, I have to agree.
>>
>>>
>>> Do we really still need this getopt.h header?
>>>
>>> gnulib, at:
>>>
>>>  https://www.gnu.org/software/gnulib/manual/html_node/getopt_002eh.html
>>>
>>> says:
>>>
>>>   "This header file is missing on some platforms: AIX 5.1, HP-UX 11, MSVC 14."
>>>
>>> AIX 5.1 is from 2001.
>>> HP-UX 11 is from 1997.
>>> We don't support building with MSVC AFAIK.
>>>
>>> Can't we just get rid of it?
>>>
>>
>> I feel this is too unsafe to do that
> 
> Why?  Are you aware of any host system that people actually build binutils on
> that doesn't have a proper getopt declaration?

No.  I'm worrying because currently included getopt.h contains
declarations of getopt_long and getopt_long_only.  Those declarations
are required (not necessarily in <getopt.h> though) because if the
system does not have getopt_long and/or getopt_long_only, libiberty
implementation is (and should be) used.

>> (unless you assume getopt_long and
>> getopt_long_only are always available on the system).

That's why I said the sentence above.

> 
> getopt is supposed to be declared in unistd.h.  The .c files that
> use getopt (not the GNU extensions) could just switch to including
> that one.
> 
> Even if there is such a system, I would think that a better fix would
> be to rename include/getopt.h to something else, and have that header include
> <getopt.h> and/or do whatever else needed to pick the right declarations on the system.
> A standard header replacement that doesn't #include_next the original is just asking
> for trouble, like we've run into...

I also agree that.  In fact, that was my first idea to deal with this
problem.  As a long-term solution, I support your idea.

But, as a short-term solution, there are a few obstacles and intentions:

1.  The fact that include/getopt.h is shared by both Binutils and GCC
    projects so that we must sync Binutils and GCC.
    That would be far beyond my capacity to handle.  My messy patch also
    changed include/getopt.h but I thought I can explain the changes
    to GCC people (since the changes were that small).
2.  I don't want to disrupt any release schedule.  The changes will not
    be small and spans through multiple subprojects.
3.  For me, it's fine as long as...
    a.  CentOS 7 regression is resolved and
    b.  Build problem with Clang is fewer than before

Replacing getopt with getopt_long won't be large and relatively
self-explanatory.  I will propose sim workaround as a short-term solution.
_At the same time_, we could discuss the possibility of getting rid of
"include/getopt.h" entirely.

Thanks,
Tsukasa

> 
>>
>> Even if this patch is unacceptable, I don't want to revert previous
>> changes to sim/configure{,.ac} either (it's necessary to prevent a build
>> failure with Clang).  
> 
> It's only necessary because of this hacky include/getopt.h header existing, no?.
> Why would you want to keep the configure.ac bits if the hacky header with the
> unprototyped declaration doesn't exist any more?
> 
> Thanks,
> Pedro Alves
> 
>> It's also a hack but to stop using getopt
>> (entirely) may be an option, changing following files:
>>
>> -   sim/igen/igen.c
>> -   sim/m32c/main.c
>> -   sim/rl78/main.c
>>
>> I mean, we could replace getopt with getopt_long plus dummy longopts.
>> This way, CentOS (7) regression will (also) be gone.
>>
>> What do you think?
>>
>> Thanks,
>> Tsukasa
>>
>