Module/readline.c has suspicious macro

[sobolevn]

Bug report

readline has two identical function signatures under different macro branches:

cpython/Modules/readline.c

Lines 1019 to 1024 in f75cefd

	static int
	#if defined(_RL_FUNCTION_TYPEDEF)
	on_startup_hook(void)
	#else
	on_startup_hook(void)
	#endif

and

cpython/Modules/readline.c

Lines 1033 to 1039 in f75cefd

	#ifdef HAVE_RL_PRE_INPUT_HOOK
	static int
	#if defined(_RL_FUNCTION_TYPEDEF)
	on_pre_input_hook(void)
	#else
	on_pre_input_hook(void)
	#endif

Looks like that they became identical after a9305b5

Original issue when this macro was introduced: #64573
But, new versions of readline do not ship _RL_FUNCTION_TYPEDEF anymore: https://git.savannah.gnu.org/cgit/readline.git/tree/rltypedefs.h#n50

So, what should we do?

1. Remove #if defined
2. Revert a9305b5

[CorvinM]

If we care about support for old readline versions, then option 2 as the commit likely broke it.
If we don't then I'd say option 1 for just a bit of code cleanup.

It looks like readline library has always defined _RL_FUNCTION_TYPEDEF since at least version 4.2 (circa 2001)

[sobolevn]

cc @iritkatriel

[iritkatriel]

I made that commit to silence some warning. I don't have an opinion on this.

[erlend-aasland]

I currently get a compiler warning for readline.c on my Mac. The warning disappears if I revert a9305b5.

$ touch Modules/readline.c
$ make
gcc  -fno-strict-overflow -g -Og -Wall -fprofile-instr-generate -fcoverage-mapping -Og   -std=c11 -Werror=implicit-function-declaration -fvisibility=hidden  -I/Users/erlend.aasland/src/cpython.git/Include/internal -IObjects -IInclude -IPython -I. -I/Users/erlend.aasland/src/cpython.git/Include     -c /Users/erlend.aasland/src/cpython.git/Modules/readline.c -o Modules/readline.o
/Users/erlend.aasland/src/cpython.git/Modules/readline.c:1257:21: warning: incompatible function pointer types assigning to 'Function *' (aka 'int (*)(const char *, int)') from 'int (void)' [-Wincompatible-function-pointer-types]
    rl_startup_hook = on_startup_hook;
                    ^ ~~~~~~~~~~~~~~~
/Users/erlend.aasland/src/cpython.git/Modules/readline.c:1259:23: warning: incompatible function pointer types assigning to 'Function *' (aka 'int (*)(const char *, int)') from 'int (void)' [-Wincompatible-function-pointer-types]
    rl_pre_input_hook = on_pre_input_hook;
                      ^ ~~~~~~~~~~~~~~~~~
2 warnings generated.
gcc -bundle -undefined dynamic_lookup -fprofile-instr-generate -fcoverage-mapping     Modules/readline.o -lreadline  -o Modules/readline.cpython-313d-darwin.so
Checked 107 modules (31 built-in, 76 shared, 0 n/a on macosx-13.4-arm64, 0 disabled, 0 missing, 0 failed on import)
$ git revert a9305b5e80e414b8b9b2bd366e96b43add662d70
$ make
gcc  -fno-strict-overflow -g -Og -Wall -fprofile-instr-generate -fcoverage-mapping -Og   -std=c11 -Werror=implicit-function-declaration -fvisibility=hidden  -I/Users/erlend.aasland/src/cpython.git/Include/internal -IObjects -IInclude -IPython -I. -I/Users/erlend.aasland/src/cpython.git/Include     -c /Users/erlend.aasland/src/cpython.git/Modules/readline.c -o Modules/readline.o
gcc -bundle -undefined dynamic_lookup -fprofile-instr-generate -fcoverage-mapping     Modules/readline.o -lreadline  -o Modules/readline.cpython-313d-darwin.so
Checked 107 modules (31 built-in, 76 shared, 0 n/a on macosx-13.4-arm64, 0 disabled, 0 missing, 0 failed on import)

[erlend-aasland]

See #105323, which appeared a few days after a9305b5

[erlend-aasland]

@iritkatriel, do you get a compiler warning if you revert a9305b5?

[iritkatriel]

Go ahead and revert it. I don't remember the details of why I put it in.

[iritkatriel]

I do get a warning actually when I revert it:

./Modules/readline.c:1023:16: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_startup_hook()
               ^
                void
./Modules/readline.c:1038:18: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_pre_input_hook()
                 ^
                  void

[erlend-aasland]

I do get a warning actually when I revert it:

./Modules/readline.c:1023:16: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_startup_hook()
               ^
                void
./Modules/readline.c:1038:18: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_pre_input_hook()
                 ^
                  void

Right, so we should try and find out the real fix, then.

[erlend-aasland]

What's the output of grep READLINE Makefile config.log, @iritkatriel?

[sobolevn]

@erlend-aasland no, I don't work on this issue, I don't have any knowledge about readline :)

[erlend-aasland]

@corona10: suggesting to close #105323 as superseded, since we've got more information in this discussion.

[iritkatriel]

% grep READLINE Makefile config.log
Makefile:MODULE_READLINE_STATE=yes
Makefile:MODULE_READLINE_CFLAGS=
Makefile:MODULE_READLINE_LDFLAGS=-lreadline
Makefile:Modules/readline.o: $(srcdir)/Modules/readline.c $(MODULE_READLINE_DEPS) $(MODULE_DEPS_SHARED) $(PYTHON_HEADERS); $(CC) $(MODULE_READLINE_CFLAGS) $(PY_STDMODULE_CFLAGS) $(CCSHARED) -c $(srcdir)/Modules/readline.c -o Modules/readline.o
Makefile:Modules/readline$(EXT_SUFFIX):  Modules/readline.o; $(BLDSHARED)  Modules/readline.o $(MODULE_READLINE_LDFLAGS)  -o Modules/readline$(EXT_SUFFIX)
config.log:configure:23783: checking for LIBREADLINE
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:ac_cv_env_LIBREADLINE_CFLAGS_set=
config.log:ac_cv_env_LIBREADLINE_CFLAGS_value=
config.log:ac_cv_env_LIBREADLINE_LIBS_set=
config.log:ac_cv_env_LIBREADLINE_LIBS_value=
config.log:LIBREADLINE_CFLAGS=''
config.log:LIBREADLINE_LIBS=''
config.log:MODULE_READLINE_CFLAGS=
config.log:MODULE_READLINE_FALSE='#'
config.log:MODULE_READLINE_LDFLAGS=-lreadline
config.log:MODULE_READLINE_STATE=yes
config.log:MODULE_READLINE_TRUE=''
config.log:#define HAVE_READLINE_READLINE_H 1

[corona10]

See #105323, which appeared a few days after a9305b5

This issue is not related to whether we have to declare the params as void or not, it's just difference between explict declaration or not.

@corona10: suggesting to close #105323 as superseded, since we've got more information in this discussion.

I am not sure, so it looks like, the right solution is detecting different signatures between different readline versions and compiling with conditional compilation as we discussed from #105323? no?

[erlend-aasland]

I am not sure, so it looks like, the right solution is detecting different signatures between different readline versions and compiling with conditional compilation as we discussed from #105323? no?

Your issue is solved for me if I revert Irit's commit, so I do think they are related.

[erlend-aasland]

@corona10, see #108588 (comment).

[corona10]

Your issue is solved for me if I revert Irit's commit, so I do think they are related.

If you are arguing that the issue is just about ignoring the compiler warning, you are right.
But if we are focusing on the on_pre_input_hook and on_startup_hook are not declared as the proper params for each readline versions, the issue is not yet solved.

[erlend-aasland]

All right, I leave it to you to consider if this issue, #105323, and #105240 are related :) I don't have the bandwidth for this. FTR, here's some info from my machine:

$ echo '#include <editline/readline.h>' | gcc -E - | grep "typedef.*Function"
typedef int Function(const char *, int);
typedef void VFunction(void);
typedef void VCPFunction(char *);
typedef char *CPFunction(const char *, int);
typedef char **CPPFunction(const char *, int, int);
$ echo '#include <readline/readline.h>' | gcc -E - | grep "typedef.*Function"
typedef int Function(const char *, int);
typedef void VFunction(void);
typedef void VCPFunction(char *);
typedef char *CPFunction(const char *, int);
typedef char **CPPFunction(const char *, int, int);

[corona10]

Yeah, when I reviewed the #105241, I didn't deeply investigate the issue.
It was not a proper patch at that moment, but we discovered the root cause from #105323 :(

I think that it's still worth to solve the issue.

[corona10]

Patch is submitted for macOS: #108633

[ned-deily]

I don't have a lot of insight into this and I likely won't have time to look at it further for the next two weeks. But, AFAIK, there are at least three cases of interest that need to be covered: 1. linking the Python readline module with GNU readline; 2. linking with the latest BSD editline (libedit) module; and 3. linking with the version of BSD editline (libedit) that Apple ships with macOS. Note that editline provides an old GNU readline compatibility shim and that is what we link to when using any version of libedit, i.e. we do not support the native editline/libedit API. All three of those cases are being used today on macOS by various distributors so we should be sure they all work. The first two are used on other platforms and should also be tested. There may be other configurations of interest.

[erlend-aasland]

I think we can close this now that #117870 has landed.