[gdb/build] Remove dependency on _rl_term_autowrap
Commit Message
[ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]
On 4/30/23 21:15, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Tom> As it happens, when TERM is set to ansi, readline decides that the terminal
> Tom> cannot auto-wrap lines, and reserves one column to deal with that, and as a
> Tom> result reports back one less than the actual screen width:
> ...
> Tom> This is currently not causing trouble in testing because we have a workaround
> Tom> in place in proc Term::resize. If we disable the workaround:
>
> Thank you for tracking this down.
> I never even considered this might be a readline issue.
>
> Tom> + readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
>
> I hate to have a new dependency on a readline internal variable.
Agreed, it's ugly.
> Don't some distros mark these as hidden in libreadline.so?
> I feel like there was another bug along these lines.
>
That's probably PR10723 - "Dependency on readline internal variable" (
https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).
> However, I don't see another way to do it.
I had an earlier version of the patch that used the COLUMNS env variable
instead. It felt a bit hacky, so I went for _rl_term_autowrap, but
patch attached below goes back to the COLUMNS approach. WDYT?
> Maybe some official API could be added by the upstream readline.
> Would you want to bring it up there?
>
If we commit the patch below, then this is solved for _rl_term_autowrap,
but not for the other instances.
I expect bringing it up will will need to be done per instance, with a
detailed explanation of why we need it, so that sounds like project I
don't have time for atm.
> Anyway it seems ok to me.
Thanks for the review.
- Tom
Comments
On 5/1/23 09:45, Tom de Vries wrote:
> [ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]
>
> On 4/30/23 21:15, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries via Gdb-patches
>>>>>>> <gdb-patches@sourceware.org> writes:
>>
>> Tom> As it happens, when TERM is set to ansi, readline decides that
>> the terminal
>> Tom> cannot auto-wrap lines, and reserves one column to deal with
>> that, and as a
>> Tom> result reports back one less than the actual screen width:
>> ...
>> Tom> This is currently not causing trouble in testing because we have
>> a workaround
>> Tom> in place in proc Term::resize. If we disable the workaround:
>>
>> Thank you for tracking this down.
>> I never even considered this might be a readline issue.
>>
>> Tom> + readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
>>
>> I hate to have a new dependency on a readline internal variable.
>
> Agreed, it's ugly.
>
>> Don't some distros mark these as hidden in libreadline.so?
>> I feel like there was another bug along these lines.
>>
>
> That's probably PR10723 - "Dependency on readline internal variable" (
> https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).
>
>> However, I don't see another way to do it.
>
> I had an earlier version of the patch that used the COLUMNS env variable
> instead. It felt a bit hacky, so I went for _rl_term_autowrap, but
> patch attached below goes back to the COLUMNS approach. WDYT?
>
Ping.
If this is not acceptable, I'll bring up making the variable public on
the readline mailing list.
Thanks,
- Tom
>> Maybe some official API could be added by the upstream readline.
>> Would you want to bring it up there?
>>
>
> If we commit the patch below, then this is solved for _rl_term_autowrap,
> but not for the other instances.
>
> I expect bringing it up will will need to be done per instance, with a
> detailed explanation of why we need it, so that sounds like project I
> don't have time for atm.
>
>> Anyway it seems ok to me.
>
> Thanks for the review.
>
> - Tom
On 5/16/23 10:53, Tom de Vries via Gdb-patches wrote:
> On 5/1/23 09:45, Tom de Vries wrote:
>> [ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]
>>
>> On 4/30/23 21:15, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries via Gdb-patches
>>>>>>>> <gdb-patches@sourceware.org> writes:
>>>
>>> Tom> As it happens, when TERM is set to ansi, readline decides that
>>> the terminal
>>> Tom> cannot auto-wrap lines, and reserves one column to deal with
>>> that, and as a
>>> Tom> result reports back one less than the actual screen width:
>>> ...
>>> Tom> This is currently not causing trouble in testing because we have
>>> a workaround
>>> Tom> in place in proc Term::resize. If we disable the workaround:
>>>
>>> Thank you for tracking this down.
>>> I never even considered this might be a readline issue.
>>>
>>> Tom> + readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
>>>
>>> I hate to have a new dependency on a readline internal variable.
>>
>> Agreed, it's ugly.
>>
>>> Don't some distros mark these as hidden in libreadline.so?
>>> I feel like there was another bug along these lines.
>>>
>>
>> That's probably PR10723 - "Dependency on readline internal variable" (
>> https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).
>>
>>> However, I don't see another way to do it.
>>
>> I had an earlier version of the patch that used the COLUMNS env
>> variable instead. It felt a bit hacky, so I went for
>> _rl_term_autowrap, but patch attached below goes back to the COLUMNS
>> approach. WDYT?
>>
>
Ping^2.
Thanks,
- Tom
>
> If this is not acceptable, I'll bring up making the variable public on
> the readline mailing list.
>
> Thanks,
> - Tom
>
>>> Maybe some official API could be added by the upstream readline.
>>> Would you want to bring it up there?
>>>
>>
>> If we commit the patch below, then this is solved for
>> _rl_term_autowrap, but not for the other instances.
>>
>> I expect bringing it up will will need to be done per instance, with a
>> detailed explanation of why we need it, so that sounds like project I
>> don't have time for atm.
>>
>>> Anyway it seems ok to me.
>>
>> Thanks for the review.
>>
>> - Tom
>
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
>> However, I don't see another way to do it.
Tom> I had an earlier version of the patch that used the COLUMNS env
Tom> variable instead. It felt a bit hacky, so I went for
Tom> _rl_term_autowrap, but patch attached below goes back to the COLUMNS
Tom> approach. WDYT?
Looks good to me. Maybe one nit:
Tom> + char *columns_env_str = getenv ("COLUMNS");
... mildly better to write "const char *" here.
Tom
From dbb4a6b592df08d6263af38c4857c55acb540352 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Mon, 1 May 2023 07:19:33 +0200
Subject: [PATCH] [gdb/build] Remove dependency on _rl_term_autowrap
Commit deb1ba4e38b ("[gdb/tui] Fix TUI resizing for TERM=ansi") introduced a
dependency on readline private variable _rl_term_autowrap.
There is precedent for this, but it's something we want to get rid of
(PR build/10723).
Remove the dependency on _rl_term_autowrap, and instead calculate
readline_hidden_cols by comparing the environment variable COLS with cols as
returned by rl_get_screen_size.
Tested on x86_64-linux.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=10723
---
gdb/utils.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
@@ -1116,10 +1116,6 @@ static bool filter_initialized = false;
-/* See readline's rlprivate.h. */
-
-EXTERN_C int _rl_term_autowrap;
-
/* See utils.h. */
int readline_hidden_cols = 0;
@@ -1160,10 +1156,17 @@ init_page_info (void)
(because rl_change_environment defaults to 1)
- may report one less than the detected screen width in
rl_get_screen_size (when _rl_term_autowrap == 0).
- We could set readline_hidden_cols by comparing COLUMNS to cols as
- returned by rl_get_screen_size, but instead simply use
- _rl_term_autowrap. */
- readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
+ We could use _rl_term_autowrap, but we want to avoid introducing
+ another dependency on readline private variables, so set
+ readline_hidden_cols by comparing COLUMNS to cols as returned by
+ rl_get_screen_size. */
+ char *columns_env_str = getenv ("COLUMNS");
+ gdb_assert (columns_env_str != nullptr);
+ int columns_env_val = atoi (columns_env_str);
+ gdb_assert (columns_env_val != 0);
+ readline_hidden_cols = columns_env_val - cols;
+ gdb_assert (readline_hidden_cols >= 0);
+ gdb_assert (readline_hidden_cols <= 1);
lines_per_page = rows;
chars_per_line = cols;
base-commit: 077a1f08485e88f3b234af1dbb8b907b16045e6a
--
2.35.3