Skip to content

Module/readline.c has suspicious macro #108588

Closed
Closed
Module/readline.c has suspicious macro#108588
Labels
extension-modulesC modules in the Modules dirtype-bugAn unexpected behavior, bug, or error

Description

@sobolevn
sobolevn
opened on Aug 28, 2023

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

Activity

  1. sobolevn
    added
    type-bugAn unexpected behavior, bug, or error
    extension-modulesC modules in the Modules dir
    on Aug 28, 2023

  2. CorvinM commented on Aug 28, 2023

    @CorvinM
    CorvinM
    on Aug 28, 2023
    Contributor

    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)

  3. sobolevn commented on Aug 29, 2023

    @sobolevn
    sobolevn
    on Aug 29, 2023
    MemberAuthor

    cc @iritkatriel

  4. iritkatriel commented on Aug 29, 2023

    @iritkatriel
    iritkatriel
    on Aug 29, 2023
    Member

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

  5. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

    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)

  6. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

    See #105323, which appeared a few days after a9305b5

  7. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

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

  8. iritkatriel commented on Aug 29, 2023

    @iritkatriel
    iritkatriel
    on Aug 29, 2023
    Member

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

  9. iritkatriel commented on Aug 29, 2023

    @iritkatriel
    iritkatriel
    on Aug 29, 2023
    Member

    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

  10. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023 · Hidden as outdated

  11. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

    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.

  12. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

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

  13. sobolevn commented on Aug 29, 2023

    @sobolevn
    sobolevn
    on Aug 29, 2023
    MemberAuthor

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

  14. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

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

  15. iritkatriel commented on Aug 29, 2023

    @iritkatriel
    iritkatriel
    on Aug 29, 2023
    Member
    % 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

  16. corona10 commented on Aug 29, 2023

    @corona10
    corona10
    on Aug 29, 2023
    Member

    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?

  17. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

    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.

  18. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

    @corona10, see #108588 (comment).

  19. corona10 commented on Aug 29, 2023

    @corona10
    corona10
    on Aug 29, 2023
    Member

    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.

  20. erlend-aasland commented on Aug 29, 2023

    @erlend-aasland
    erlend-aasland
    on Aug 29, 2023
    Contributor

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

  21. corona10 commented on Aug 29, 2023

    @corona10
    corona10
    on Aug 29, 2023
    Member

    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.

  22. corona10 commented on Aug 29, 2023

    @corona10
    corona10
    on Aug 29, 2023
    Member

    Patch is submitted for macOS: #108633

  23. ned-deily commented on Aug 29, 2023

    @ned-deily
    ned-deily
    on Aug 29, 2023
    Member

    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.

  24. erlend-aasland commented on Apr 17, 2024

    @erlend-aasland
    erlend-aasland
    on Apr 17, 2024
    Contributor

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

  25. erlend-aasland
    added
    pendingThe issue will be closed if no feedback is provided
    on Apr 17, 2024
  26. encukou
    closed this as completedon Apr 17, 2024
  27. erlend-aasland
    removed
    pendingThe issue will be closed if no feedback is provided
    on Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    extension-modulesC modules in the Modules dirtype-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions