public inbox for gdb@sourceware.org
 help / color / mirror / Atom feed
* Re: [Bug-readline] [PATCH] Enable visibility annotations
@ 2016-04-18 18:16 Doug Evans
  2016-04-20  6:02 ` Yury Gribov
  2016-04-20 21:16 ` Chet Ramey
  0 siblings, 2 replies; 11+ messages in thread
From: Doug Evans @ 2016-04-18 18:16 UTC (permalink / raw)
  To: Pedro Alves
  Cc: chet.ramey, Yury Gribov, bug-readline, Vyacheslav Barinov,
	Yury Usishchev, gdb, Jan Kratochvil

Pedro Alves writes:
  > [gdb list, plus a couple people]
  >
  > See https://lists.gnu.org/archive/html/bug-readline/2016-04/msg00006.html
  >
  > (note that ml archive only refreshes every 30 mins...)
  >
  > On 04/14/2016 03:59 PM, Chet Ramey wrote:
  > > On 4/14/16 6:47 AM, Pedro Alves wrote:
  > >
  > >> FYI, this is probably doing to break (at least) gdb against system
  > >> readline.  gdb relies on accessing a few private readline  
symbols...  :-/
  > >>
  > >> E.g.:
  > >>
  > >> tui/tui-io.c:437:  extern int _rl_echoing_p;
  > >> completer.c:1677:  extern int _rl_complete_mark_directories;
  > >> completer.c:1770:extern int _rl_completion_prefix_display_length;
  > >> completer.c:1771:extern int _rl_print_completions_horizontally;
  > >>
  > >> Unless/until someone fixes these and adds the missing public APIs
  > >> to readline,
  > >
  > > These aren't exactly the result of `missing public APIs'.  echoing_p is
  > > a saved value indicating whether or not the terminal flags readline
  > > inherits include ECHO -- something an application can easily check for
  > > itself.
  >
  > It's not that GDB wants to do something with the value.  It's that
  > when gdb enables/disables its TUI/curses mode (c-a x), it needs to
  > save/restore that variable, here:
  >
  >  
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/tui/tui-io.c;h=18c648c08fdbe9a8f7d04c406c6a99e9149ca295;hb=2d059a33d890c017c8105b102a6b56ccbd6128b2#l426
  >
  > That's quite old code.  This gdb commit:
  >
  >  commit cc88a640ca1d0356c5feb40bb48869bab5a2bdce
  >  Author:     Jan Kratochvil <jan.kratochvil@redhat.com>
  >  AuthorDate: Wed May 11 23:38:44 2011 +0000
  >
  >     Imported readline 6.2, and upstream patch 001.
  >
  > (That's gdb's built-in fallback readline copy, which is
  > discouraged in favor of the system one.)
  >
  > Did:
  >
  >  void
  >  tui_setup_io (int mode)
  >  {
  > -  extern int readline_echoing_p;
  > -
  > +  extern int _rl_echoing_p;
  > +
  >
  >
  > So looks like at some point readline_echoing_p was renamed
  > to _rl_echoing_p?
  >
  > And that readline_echoing_p referencing code was added back in 2001 ...:
  >
  >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000  
506) void
  >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000  
507) tui_setup_io (int mode)
  >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000  
508) {
  >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000  
509)   extern int readline_echoing_p;
  >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000  
510)
  >
  > I haven't tried to remove that and see what breaks.
  >
  > > The others are values for settable variables and should be retrieved
  > > using rl_variable_value (variable_name).  Since it returns a string,
  > > you can use the public macro RL_BOOLEAN_VARIABLE_VALUE to turn it into
  > > an int.
  >
  > The GDB commit that added those was:
  >
  >  commit 82083d6dbbc0b2f6a76095582c6e7ffb3e06432a
  >  Author:     Doug Evans <xdje42@gmail.com>
  >  AuthorDate: Sat Jan 31 14:11:54 2015 -0800
  >
  >     Unify CLI/TUI interface to readline tab completion.
  >
  >     This copies a lot of code from readline, but this is temporary.
  >     Readline currently doesn't export what we need.
  >     The plan is to have something that has been working for awhile,
  >     and then we'll have a complete story to present to the readline
  >     maintainers.
  >
  > This is again about the curses mode (TUI).
  >
  > Doug, could you comment?

The code in question (commit 82083d6dbbc0b2f6a76095582c6e7ffb3e06432a)
replicates the tab completion match list displayer in order to work
better with TUI. The patch includes this comment to (hopefully)
explain what's going on.

/* GDB replacement for rl_display_match_list.
    Readline doesn't provide a clean interface for TUI(curses).
    A hack previously used was to send readline's rl_outstream through a pipe
    and read it from the event loop.  Bleah.  IWBN if readline abstracted
    away all the necessary bits, and this is what this code does.  It
    replicates the parts of readline we need and then adds an abstraction
    layer, currently implemented as struct match_list_displayer, so that both
    CLI and TUI can use it.  We copy all this readline code to minimize
    GDB-specific mods to readline.  Once this code performs as desired then
    we can submit it to the readline maintainers.

    N.B. A lot of the code is the way it is in order to minimize differences
    from readline's copy.  */

There are two aspects to this:
1) some _rl_ state vars are referenced
2) some _rl_ functions are used

If there's a public way to access the _rl_ vars, great.

As for the _rl_ functions, I think there are two:
_rl_erase_entire_line
_rl_qsort_string_compare

Readline does export a few things like _rl_erase_entire_line,
e.g., rl_crlf, so may it'd be ok to export that one.

I think I used _rl_qsort_string_compare for simplicity.
If necessary gdb could duplicate that one I think.

That would get us past this patch.

Ultimately, there's still a lot of code duplication.
My hope is that we can reduce/remove it, but that's
perhaps a topic for another day.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-18 18:16 [Bug-readline] [PATCH] Enable visibility annotations Doug Evans
@ 2016-04-20  6:02 ` Yury Gribov
  2016-04-20  9:22   ` Pedro Alves
  2016-04-20 21:16 ` Chet Ramey
  1 sibling, 1 reply; 11+ messages in thread
From: Yury Gribov @ 2016-04-20  6:02 UTC (permalink / raw)
  To: Doug Evans, Pedro Alves
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/18/2016 09:16 PM, Doug Evans wrote:
> Pedro Alves writes:
>   > [gdb list, plus a couple people]
>   >
>   > See
> https://lists.gnu.org/archive/html/bug-readline/2016-04/msg00006.html
>   >
>   > (note that ml archive only refreshes every 30 mins...)
>   >
>   > On 04/14/2016 03:59 PM, Chet Ramey wrote:
>   > > On 4/14/16 6:47 AM, Pedro Alves wrote:
>   > >
>   > >> FYI, this is probably doing to break (at least) gdb against system
>   > >> readline.  gdb relies on accessing a few private readline
> symbols...  :-/
>   > >>
>   > >> E.g.:
>   > >>
>   > >> tui/tui-io.c:437:  extern int _rl_echoing_p;
>   > >> completer.c:1677:  extern int _rl_complete_mark_directories;
>   > >> completer.c:1770:extern int _rl_completion_prefix_display_length;
>   > >> completer.c:1771:extern int _rl_print_completions_horizontally;
>   > >>
>   > >> Unless/until someone fixes these and adds the missing public APIs
>   > >> to readline,
>   > >
>   > > These aren't exactly the result of `missing public APIs'.
> echoing_p is
>   > > a saved value indicating whether or not the terminal flags readline
>   > > inherits include ECHO -- something an application can easily check
> for
>   > > itself.
>   >
>   > It's not that GDB wants to do something with the value.  It's that
>   > when gdb enables/disables its TUI/curses mode (c-a x), it needs to
>   > save/restore that variable, here:
>   >
>   >
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/tui/tui-io.c;h=18c648c08fdbe9a8f7d04c406c6a99e9149ca295;hb=2d059a33d890c017c8105b102a6b56ccbd6128b2#l426
>
>   >
>   > That's quite old code.  This gdb commit:
>   >
>   >  commit cc88a640ca1d0356c5feb40bb48869bab5a2bdce
>   >  Author:     Jan Kratochvil <jan.kratochvil@redhat.com>
>   >  AuthorDate: Wed May 11 23:38:44 2011 +0000
>   >
>   >     Imported readline 6.2, and upstream patch 001.
>   >
>   > (That's gdb's built-in fallback readline copy, which is
>   > discouraged in favor of the system one.)
>   >
>   > Did:
>   >
>   >  void
>   >  tui_setup_io (int mode)
>   >  {
>   > -  extern int readline_echoing_p;
>   > -
>   > +  extern int _rl_echoing_p;
>   > +
>   >
>   >
>   > So looks like at some point readline_echoing_p was renamed
>   > to _rl_echoing_p?
>   >
>   > And that readline_echoing_p referencing code was added back in 2001
> ...:
>   >
>   >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54
> +0000 506) void
>   >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54
> +0000 507) tui_setup_io (int mode)
>   >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54
> +0000 508) {
>   >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54
> +0000 509)   extern int readline_echoing_p;
>   >  a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54
> +0000 510)
>   >
>   > I haven't tried to remove that and see what breaks.
>   >
>   > > The others are values for settable variables and should be retrieved
>   > > using rl_variable_value (variable_name).  Since it returns a string,
>   > > you can use the public macro RL_BOOLEAN_VARIABLE_VALUE to turn it
> into
>   > > an int.
>   >
>   > The GDB commit that added those was:
>   >
>   >  commit 82083d6dbbc0b2f6a76095582c6e7ffb3e06432a
>   >  Author:     Doug Evans <xdje42@gmail.com>
>   >  AuthorDate: Sat Jan 31 14:11:54 2015 -0800
>   >
>   >     Unify CLI/TUI interface to readline tab completion.
>   >
>   >     This copies a lot of code from readline, but this is temporary.
>   >     Readline currently doesn't export what we need.
>   >     The plan is to have something that has been working for awhile,
>   >     and then we'll have a complete story to present to the readline
>   >     maintainers.
>   >
>   > This is again about the curses mode (TUI).
>   >
>   > Doug, could you comment?
>
> The code in question (commit 82083d6dbbc0b2f6a76095582c6e7ffb3e06432a)
> replicates the tab completion match list displayer in order to work
> better with TUI. The patch includes this comment to (hopefully)
> explain what's going on.
>
> /* GDB replacement for rl_display_match_list.
>     Readline doesn't provide a clean interface for TUI(curses).
>     A hack previously used was to send readline's rl_outstream through a
> pipe
>     and read it from the event loop.  Bleah.  IWBN if readline abstracted
>     away all the necessary bits, and this is what this code does.  It
>     replicates the parts of readline we need and then adds an abstraction
>     layer, currently implemented as struct match_list_displayer, so that
> both
>     CLI and TUI can use it.  We copy all this readline code to minimize
>     GDB-specific mods to readline.  Once this code performs as desired then
>     we can submit it to the readline maintainers.
>
>     N.B. A lot of the code is the way it is in order to minimize
> differences
>     from readline's copy.  */
>
> There are two aspects to this:
> 1) some _rl_ state vars are referenced
> 2) some _rl_ functions are used
>
> If there's a public way to access the _rl_ vars, great.
>
> As for the _rl_ functions, I think there are two:
> _rl_erase_entire_line
> _rl_qsort_string_compare
>
> Readline does export a few things like _rl_erase_entire_line,
> e.g., rl_crlf, so may it'd be ok to export that one.
>
> I think I used _rl_qsort_string_compare for simplicity.
> If necessary gdb could duplicate that one I think.
>
> That would get us past this patch.
>
> Ultimately, there's still a lot of code duplication.
> My hope is that we can reduce/remove it, but that's
> perhaps a topic for another day.

Pedro,

Do you think the above is doable for gdb? That would of course leave the 
problem of linking older gdb with new readline which will need to be 
resolved by distros.

-Y

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20  6:02 ` Yury Gribov
@ 2016-04-20  9:22   ` Pedro Alves
  2016-04-20  9:45     ` Yury Gribov
  0 siblings, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2016-04-20  9:22 UTC (permalink / raw)
  To: Yury Gribov, Doug Evans
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/20/2016 07:02 AM, Yury Gribov wrote:

> Pedro,
> 
> Do you think the above is doable for gdb? 

I don't know -- I'm already juggling too many balls in the
air, and I'm afraid that if that depends on me, it'll take a
while before I'll manage to try.

Since gdb's use of private symbols is not an isolated incident,
I don't think we should try to clean up all packages that make
use of private symbols, in a rush.  Instead, I think readline
needs to take a staged approach:

- Export all the private symbols that programs are using today.
  Simply accept today's reality.  The dynamic symbol table of
  libreadline.so still shrinks, precedent for visibility
  annotations is still set, and private symbol leakage
  is contained, because future software will no longer be
  able to abuse all the other private symbols that are
  not exported.

- Claim that the symbols may no longer be available in a
  future release.

- Give time for packages to clean themselves up, and propose
  any necessary new replacement APIs.

- Optionally, in the release after the next, mark the symbols
  as deprecated with __attribute__((deprecated)), so packages
  that abuse private symbols get a build-time warning.

- In some future release, stop exporting the symbols.

> That would of course leave the
> problem of linking older gdb with new readline which will need to be
> resolved by distros.

Right.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20  9:22   ` Pedro Alves
@ 2016-04-20  9:45     ` Yury Gribov
  2016-04-20 10:19       ` Pedro Alves
  0 siblings, 1 reply; 11+ messages in thread
From: Yury Gribov @ 2016-04-20  9:45 UTC (permalink / raw)
  To: Pedro Alves, Doug Evans
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/20/2016 12:22 PM, Pedro Alves wrote:
> On 04/20/2016 07:02 AM, Yury Gribov wrote:
>
>> Pedro,
>>
>> Do you think the above is doable for gdb?
>
> I don't know -- I'm already juggling too many balls in the
> air, and I'm afraid that if that depends on me, it'll take a
> while before I'll manage to try.
>
> Since gdb's use of private symbols is not an isolated incident,
> I don't think we should try to clean up all packages that make
> use of private symbols, in a rush.  Instead, I think readline
> needs to take a staged approach:
>
> - Export all the private symbols that programs are using today.
>    Simply accept today's reality.  The dynamic symbol table of
>    libreadline.so still shrinks, precedent for visibility
>    annotations is still set, and private symbol leakage
>    is contained, because future software will no longer be
>    able to abuse all the other private symbols that are
>    not exported.

Agreed, that's my plan for now.

> - Claim that the symbols may no longer be available in a
>    future release.

You mean just email respective package maintainers?

> - Give time for packages to clean themselves up, and propose
>    any necessary new replacement APIs.

This would require significant expertise in readline though...

> - Optionally, in the release after the next, mark the symbols
>    as deprecated with __attribute__((deprecated)), so packages
>    that abuse private symbols get a build-time warning.

That won't help as these symbols are not present in headers anyway.  All 
users have their own private declarations.

> - In some future release, stop exporting the symbols.
>
>> That would of course leave the
>> problem of linking older gdb with new readline which will need to be
>> resolved by distros.
>
> Right.
>
> Thanks,
> Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20  9:45     ` Yury Gribov
@ 2016-04-20 10:19       ` Pedro Alves
  2016-04-20 10:55         ` Yury Gribov
  0 siblings, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2016-04-20 10:19 UTC (permalink / raw)
  To: Yury Gribov, Doug Evans
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/20/2016 10:45 AM, Yury Gribov wrote:
> On 04/20/2016 12:22 PM, Pedro Alves wrote:

>> - Claim that the symbols may no longer be available in a
>>    future release.
> 
> You mean just email respective package maintainers?

I was thinking readline's CHANGES / release notes.

Emailing respective package maintainers / filing bugs with them
doesn't hurt of course.

> 
>> - Give time for packages to clean themselves up, and propose
>>    any necessary new replacement APIs.
> 
> This would require significant expertise in readline though...

The alternative is bless the private symbols as
public API forever...  Each package owner will know what their
package needs from readline and why they found a need
to (ab)use readline private symbols.  I see no way around that.

> 
>> - Optionally, in the release after the next, mark the symbols
>>    as deprecated with __attribute__((deprecated)), so packages
>>    that abuse private symbols get a build-time warning.
> 
> That won't help as these symbols are not present in headers anyway.  All
> users have their own private declarations.

OK.  Making it a linker warning instead,using ".gnu.warning.SYMBOL"
sections might still work:

 http://www.airs.com/blog/archives/54

Not sure it's a good idea to raise warnings without alternatives
already in place though, thus my "Optionally".

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20 10:19       ` Pedro Alves
@ 2016-04-20 10:55         ` Yury Gribov
  2016-04-20 15:30           ` Chet Ramey
  0 siblings, 1 reply; 11+ messages in thread
From: Yury Gribov @ 2016-04-20 10:55 UTC (permalink / raw)
  To: Pedro Alves, Doug Evans
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/20/2016 01:19 PM, Pedro Alves wrote:
> On 04/20/2016 10:45 AM, Yury Gribov wrote:
>> On 04/20/2016 12:22 PM, Pedro Alves wrote:
>
>>> - Claim that the symbols may no longer be available in a
>>>     future release.
>>
>> You mean just email respective package maintainers?
>
> I was thinking readline's CHANGES / release notes.
>
> Emailing respective package maintainers / filing bugs with them
> doesn't hurt of course.
>
>>
>>> - Give time for packages to clean themselves up, and propose
>>>     any necessary new replacement APIs.
>>
>> This would require significant expertise in readline though...
>
> The alternative is bless the private symbols as
> public API forever...  Each package owner will know what their
> package needs from readline and why they found a need
> to (ab)use readline private symbols.  I see no way around that.

Guess that's what's going to happen (unless some motivated readline 
maintainer steps in, willing to work with various users on getting rid 
of leaked symbols).

>>> - Optionally, in the release after the next, mark the symbols
>>>     as deprecated with __attribute__((deprecated)), so packages
>>>     that abuse private symbols get a build-time warning.
>>
>> That won't help as these symbols are not present in headers anyway.  All
>> users have their own private declarations.
>
> OK.  Making it a linker warning instead,using ".gnu.warning.SYMBOL"
> sections might still work:
>
>   http://www.airs.com/blog/archives/54

Oh my, this is such a cool feature. I was actually thinking about a 
"Deprecated" attribute for ELF symbols which static/runtime linker could 
use to bark. Pushing such thing to Binutils and GCC would take ages 
though...

> Not sure it's a good idea to raise warnings without alternatives
> already in place though, thus my "Optionally".
>
> Thanks,
> Pedro Alves
>
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20 10:55         ` Yury Gribov
@ 2016-04-20 15:30           ` Chet Ramey
  0 siblings, 0 replies; 11+ messages in thread
From: Chet Ramey @ 2016-04-20 15:30 UTC (permalink / raw)
  To: Yury Gribov, Pedro Alves, Doug Evans
  Cc: chet.ramey, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 4/20/16 3:55 AM, Yury Gribov wrote:

>> The alternative is bless the private symbols as
>> public API forever...  Each package owner will know what their
>> package needs from readline and why they found a need
>> to (ab)use readline private symbols.  I see no way around that.
> 
> Guess that's what's going to happen (unless some motivated readline
> maintainer steps in, willing to work with various users on getting rid of
> leaked symbols).

I can work with people as my time permits, as long as they contact me.

I'm going to look at the visibility patches for the next bash/readline
release, but it's too late to get them into bash-4.4/readline-7.0.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-18 18:16 [Bug-readline] [PATCH] Enable visibility annotations Doug Evans
  2016-04-20  6:02 ` Yury Gribov
@ 2016-04-20 21:16 ` Chet Ramey
  2016-04-22 14:43   ` Pedro Alves
  1 sibling, 1 reply; 11+ messages in thread
From: Chet Ramey @ 2016-04-20 21:16 UTC (permalink / raw)
  To: Doug Evans, Pedro Alves
  Cc: chet.ramey, Yury Gribov, bug-readline, Vyacheslav Barinov,
	Yury Usishchev, gdb, Jan Kratochvil

On 4/18/16 11:16 AM, Doug Evans wrote:

> There are two aspects to this:
> 1) some _rl_ state vars are referenced
> 2) some _rl_ functions are used
> 
> If there's a public way to access the _rl_ vars, great.

For the ones that correspond to bindable readline variables, it's easy,
though you may have to pay a small conversion cost.  Use

rl_get_variable_value (char *varname)

and handle the return value appropriately.  For boolean variables, use
something like bash's RL_BOOLEAN_VARIABLE_VALUE.  For numbers, use
strtol.

The other one is probably _rl_echoing_p, and I just added

rl_tty_set_echoing (int value)

to set it and return the previous value.

> As for the _rl_ functions, I think there are two:
> _rl_erase_entire_line

This can be emulated the way bash-4.3 does it:

ce = rl_get_termcap ("ce");
if (ce)
  {
    fprintf (rl_outstream "\r");
    fprintf (rl_outstream, "%s", ce);		/* or use tputs like bash */
    fprintf (rl_outstream, "\r");		/* optional */
    fflush (rl_outstream);
  }

That's pretty much exactly what _rl_erase_entire_line does.

> _rl_qsort_string_compare

This is the standard compare-two-strings-for-qsort function that exists
everywhere.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-20 21:16 ` Chet Ramey
@ 2016-04-22 14:43   ` Pedro Alves
  2016-04-22 15:25     ` Chet Ramey
  0 siblings, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2016-04-22 14:43 UTC (permalink / raw)
  To: chet.ramey, Doug Evans
  Cc: Yury Gribov, bug-readline, Vyacheslav Barinov, Yury Usishchev,
	gdb, Jan Kratochvil

On 04/20/2016 10:16 PM, Chet Ramey wrote:

> The other one is probably _rl_echoing_p, and I just added
> 
> rl_tty_set_echoing (int value)
> 
> to set it and return the previous value.

Thanks Chet, that should work nicely for gdb.

Is there some public git branch where we can follow this?

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
  2016-04-22 14:43   ` Pedro Alves
@ 2016-04-22 15:25     ` Chet Ramey
  0 siblings, 0 replies; 11+ messages in thread
From: Chet Ramey @ 2016-04-22 15:25 UTC (permalink / raw)
  To: Pedro Alves, Doug Evans
  Cc: chet.ramey, Yury Gribov, bug-readline, Vyacheslav Barinov,
	Yury Usishchev, gdb, Jan Kratochvil

On 4/22/16 10:43 AM, Pedro Alves wrote:
> On 04/20/2016 10:16 PM, Chet Ramey wrote:
> 
>> The other one is probably _rl_echoing_p, and I just added
>>
>> rl_tty_set_echoing (int value)
>>
>> to set it and return the previous value.
> 
> Thanks Chet, that should work nicely for gdb.
> 
> Is there some public git branch where we can follow this?

There is a `devel' branch of the readline git repository on savannah.  I
don't update it as often as the bash devel branch, but many fewer things
happen to the readline code.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
		 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRU    chet@case.edu    http://cnswww.cns.cwru.edu/~chet/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Bug-readline] [PATCH] Enable visibility annotations
       [not found]     ` <570FB04D.2060107@case.edu>
@ 2016-04-14 15:19       ` Pedro Alves
  0 siblings, 0 replies; 11+ messages in thread
From: Pedro Alves @ 2016-04-14 15:19 UTC (permalink / raw)
  To: chet.ramey, Yury Gribov, bug-readline
  Cc: Vyacheslav Barinov, Yury Usishchev, Doug Evans, gdb, Jan Kratochvil

[gdb list, plus a couple people]

See https://lists.gnu.org/archive/html/bug-readline/2016-04/msg00006.html

(note that ml archive only refreshes every 30 mins...)

On 04/14/2016 03:59 PM, Chet Ramey wrote:
> On 4/14/16 6:47 AM, Pedro Alves wrote:
> 
>> FYI, this is probably doing to break (at least) gdb against system 
>> readline.  gdb relies on accessing a few private readline symbols...  :-/
>>
>> E.g.:
>>
>> tui/tui-io.c:437:  extern int _rl_echoing_p;
>> completer.c:1677:  extern int _rl_complete_mark_directories;
>> completer.c:1770:extern int _rl_completion_prefix_display_length;
>> completer.c:1771:extern int _rl_print_completions_horizontally;
>>
>> Unless/until someone fixes these and adds the missing public APIs
>> to readline, 
> 
> These aren't exactly the result of `missing public APIs'.  echoing_p is
> a saved value indicating whether or not the terminal flags readline
> inherits include ECHO -- something an application can easily check for
> itself.

It's not that GDB wants to do something with the value.  It's that
when gdb enables/disables its TUI/curses mode (c-a x), it needs to
save/restore that variable, here:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/tui/tui-io.c;h=18c648c08fdbe9a8f7d04c406c6a99e9149ca295;hb=2d059a33d890c017c8105b102a6b56ccbd6128b2#l426

That's quite old code.  This gdb commit:

 commit cc88a640ca1d0356c5feb40bb48869bab5a2bdce
 Author:     Jan Kratochvil <jan.kratochvil@redhat.com>
 AuthorDate: Wed May 11 23:38:44 2011 +0000

    Imported readline 6.2, and upstream patch 001.

(That's gdb's built-in fallback readline copy, which is
discouraged in favor of the system one.)

Did:

 void
 tui_setup_io (int mode)
 {
-  extern int readline_echoing_p;
- 
+  extern int _rl_echoing_p;
+
 

So looks like at some point readline_echoing_p was renamed
to _rl_echoing_p?

And that readline_echoing_p referencing code was added back in 2001 ...:

 a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000 506) void
 a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000 507) tui_setup_io (int mode)
 a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000 508) {
 a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000 509)   extern int readline_echoing_p;
 a198b876 gdb/tui/tuiIO.c  (Stephane Carrez   2001-07-21 19:56:54 +0000 510)  

I haven't tried to remove that and see what breaks.

> The others are values for settable variables and should be retrieved
> using rl_variable_value (variable_name).  Since it returns a string,
> you can use the public macro RL_BOOLEAN_VARIABLE_VALUE to turn it into
> an int.

The GDB commit that added those was:

 commit 82083d6dbbc0b2f6a76095582c6e7ffb3e06432a
 Author:     Doug Evans <xdje42@gmail.com>
 AuthorDate: Sat Jan 31 14:11:54 2015 -0800

    Unify CLI/TUI interface to readline tab completion.
    
    This copies a lot of code from readline, but this is temporary.
    Readline currently doesn't export what we need.
    The plan is to have something that has been working for awhile,
    and then we'll have a complete story to present to the readline
    maintainers.

This is again about the curses mode (TUI).

Doug, could you comment?

> Bash doesn't use any _rl_ readline symbols, so I know it can be done.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-04-22 15:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-18 18:16 [Bug-readline] [PATCH] Enable visibility annotations Doug Evans
2016-04-20  6:02 ` Yury Gribov
2016-04-20  9:22   ` Pedro Alves
2016-04-20  9:45     ` Yury Gribov
2016-04-20 10:19       ` Pedro Alves
2016-04-20 10:55         ` Yury Gribov
2016-04-20 15:30           ` Chet Ramey
2016-04-20 21:16 ` Chet Ramey
2016-04-22 14:43   ` Pedro Alves
2016-04-22 15:25     ` Chet Ramey
     [not found] <570F38DD.1030703@samsung.com>
     [not found] ` <570F3C7E.30005@samsung.com>
     [not found]   ` <570F7554.7070901@redhat.com>
     [not found]     ` <570FB04D.2060107@case.edu>
2016-04-14 15:19       ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).