Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ext/readline: readline_info fix usage when the buffer is not initialised #15139

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

devnexen
Copy link
Member

rl_initialise is only called when readline() is used so the global
buffer might not be initialised yet.

@petk
Copy link
Member

petk commented Jul 28, 2024

Is this perhaps also related to this issue:

./configure --with-libedit
make 
/php-src/ext/readline/readline.c: In function ‘zif_readline_info’:
/php-src/ext/readline/readline.c:191:41: warning: implicit declaration of function ‘rl_extend_line_buffer’ [-Wimplicit-function-declaration]
  191 |                                         rl_extend_line_buffer(Z_STRLEN_P(value) + 1);
      |                                         ^~~~~~~~~~~~~~~~~~~~~

/usr/bin/ld: ext/readline/readline.o: in function `zif_readline_info':
/php-src/ext/readline/readline.c:191:(.text+0x747): undefined reference to `rl_extend_line_buffer'

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

@devnexen
Copy link
Member Author

Not at all related but was not aware of the libedit/libreadline differences.

Comment on lines 190 to 195
if (!oldstr || strlen(oldstr) < Z_STRLEN_P(value)) {
rl_extend_line_buffer(Z_STRLEN_P(value) + 1);
} else if (!rl_line_buffer) {
rl_line_buffer = malloc(Z_STRLEN_P(value) + 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused, surely having the if (!rl_line_buffer) check first makes more sense? As you then don't need to check for !oldstr ?

@devnexen devnexen force-pushed the ext_readline_upd2 branch from 3ab4ff7 to 3eb00aa Compare July 29, 2024 21:54
devnexen referenced this pull request Jul 30, 2024
if the new value was larger, rl_line_buffer_length was never updated.
neither was rl_end (on unixes).

close GH-15120
@remicollet
Copy link
Member

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support

@devnexen
Copy link
Member Author

The libedit is actually preferred library to use in PHP. See #13184 (readline library shouldn't even be an option to use in PHP).

Indeed, Licenses are NOT compatible, IMHO at some point we should drop libreadline support

Let s try it for next major release then (php 9 ?).

@remicollet
Copy link
Member

Quick check,

If seems rl_end is not updated on Linux with libedit

#if !defined(PHP_WIN32) && !HAVE_LIBEDIT
...
				rl_end = Z_STRLEN_P(value);

But may be returned

#ifndef PHP_WIN32
		} else if (zend_string_equals_literal_ci(what, "end")) {
			RETVAL_LONG(rl_end);
#endif

 rl_initialise is only called when readline() is used so the global
 buffer might not be initialised yet.
@devnexen devnexen force-pushed the ext_readline_upd2 branch from ff2b44f to 56ec48a Compare July 30, 2024 15:29
@andypost
Copy link
Contributor

Faced the same trying to build 8.4.0_alpha3

@andypost
Copy link
Contributor

Thank you, the PR allowed to pass tests

Copy link
Member

@remicollet remicollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@devnexen devnexen merged commit b7e43bd into php:master Jul 31, 2024
9 of 11 checks passed
@petk
Copy link
Member

petk commented Jul 31, 2024

It works, thanks!

@andypost
Copy link
Contributor

andypost commented Jul 31, 2024

Will it cause the alpha3 tarball rebuild or better to wait for beta1?

@devnexen
Copy link
Member Author

devnexen commented Aug 1, 2024

Will it cause the alpha3 tarball rebuild or better to wait for beta1?

neither, there is alpha4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants