Bug#868609: le FTBFS with latest ncurses

[Adrian Bunk]

Source: le
Version: 1.16.3-1
Severity: serious
Tags: buster sid

https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/le.html

...
gcc -DHAVE_CONFIG_H -I. -I../lib -I../lib -I../lib -I/usr/include/ncursesw -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wwrite-strings -Woverloaded-virtual -fno-exceptions -fno-rtti -fno-implement-inlines -c -o color.o color.cc
color.cc: In function 'int find_pair(int, int)':
color.cc:36:12: error: 'int find_pair(int, int)' was declared 'extern' and later 'static' [-fpermissive]
static int find_pair(int fg,int bg)
^~~~~~~~~
In file included from edit.h:36:0,
from color.cc:27:
/usr/include/ncursesw/curses.h:924:28: note: previous declaration of 'int find_pair(int, int)'
extern NCURSES_EXPORT(int) find_pair (int, int);
^~~~~~~~~
Makefile:1299: recipe for target 'color.o' failed
make[3]: *** [color.o] Error 1

[Alexander V. Lukyanov]

Post by Adrian Bunk
Source: le
Version: 1.16.3-1
Severity: serious
Tags: buster sid
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/le.html
...
gcc -DHAVE_CONFIG_H -I. -I../lib -I../lib -I../lib -I/usr/include/ncursesw -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wwrite-strings -Woverloaded-virtual -fno-exceptions -fno-rtti -fno-implement-inlines -c -o color.o color.cc
color.cc:36:12: error: 'int find_pair(int, int)' was declared 'extern' and later 'static' [-fpermissive]
static int find_pair(int fg,int bg)
^~~~~~~~~
In file included from edit.h:36:0,
/usr/include/ncursesw/curses.h:924:28: note: previous declaration of 'int find_pair(int, int)'
extern NCURSES_EXPORT(int) find_pair (int, int);
^~~~~~~~~
Makefile:1299: recipe for target 'color.o' failed
make[3]: *** [color.o] Error 1

ncurses was extended with new symbols, some of which conflict with "le"
internal names. So either ncurses should be fixed not to export these
symbols by default or le should be fixed to rename its identifiers.

--
Alexander.

[Thomas Dickey]

Post by Alexander V. Lukyanov

Post by Adrian Bunk
Source: le
Version: 1.16.3-1
Severity: serious
Tags: buster sid
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/le.html
...
gcc -DHAVE_CONFIG_H -I. -I../lib -I../lib -I../lib -I/usr/include/ncursesw -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wwrite-strings -Woverloaded-virtual -fno-exceptions -fno-rtti -fno-implement-inlines -c -o color.o color.cc
color.cc:36:12: error: 'int find_pair(int, int)' was declared 'extern' and later 'static' [-fpermissive]
static int find_pair(int fg,int bg)
^~~~~~~~~
In file included from edit.h:36:0,
/usr/include/ncursesw/curses.h:924:28: note: previous declaration of 'int find_pair(int, int)'
extern NCURSES_EXPORT(int) find_pair (int, int);
^~~~~~~~~
Makefile:1299: recipe for target 'color.o' failed
make[3]: *** [color.o] Error 1

ncurses was extended with new symbols, some of which conflict with "le"
internal names. So either ncurses should be fixed not to export these
symbols by default or le should be fixed to rename its identifiers.

hmm - not that I'm oblivious to the problem, but (for example) a quick
check on an alternate name "find_color_pair" finds existing usage too.
This problem will come up since there's no namespaces in C (not that
C++ is free from the problem).

So... I could look for a rarely-used name (which still gives the same
connotations), or the couple of applications using ncurses could be
modified.

[Alexander V. Lukyanov]

Post by Thomas Dickey

Post by Alexander V. Lukyanov

Post by Adrian Bunk
Source: le
Version: 1.16.3-1
Severity: serious
Tags: buster sid
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/le.html
...
gcc -DHAVE_CONFIG_H -I. -I../lib -I../lib -I../lib -I/usr/include/ncursesw -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wwrite-strings -Woverloaded-virtual -fno-exceptions -fno-rtti -fno-implement-inlines -c -o color.o color.cc
color.cc:36:12: error: 'int find_pair(int, int)' was declared 'extern' and later 'static' [-fpermissive]
static int find_pair(int fg,int bg)
^~~~~~~~~
In file included from edit.h:36:0,
/usr/include/ncursesw/curses.h:924:28: note: previous declaration of 'int find_pair(int, int)'
extern NCURSES_EXPORT(int) find_pair (int, int);
^~~~~~~~~
Makefile:1299: recipe for target 'color.o' failed
make[3]: *** [color.o] Error 1

ncurses was extended with new symbols, some of which conflict with "le"
internal names. So either ncurses should be fixed not to export these
symbols by default or le should be fixed to rename its identifiers.

hmm - not that I'm oblivious to the problem, but (for example) a quick
check on an alternate name "find_color_pair" finds existing usage too.
This problem will come up since there's no namespaces in C (not that
C++ is free from the problem).
So... I could look for a rarely-used name (which still gives the same
connotations), or the couple of applications using ncurses could be
modified.

What about a separate include file for curses extensions, somthing like
ncurses_ext.h? This way any new identifiers won't conflict with legacy code.

Other common way to avoid conflicts is prefixing the identifiers.
nc_find_pair is much less likely to conflict.

--
Alexander.

[Thomas Dickey]

Post by Adrian Bunk
Source: le
Version: 1.16.3-1
Severity: serious
Tags: buster sid
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/le.html

This is a problem with "le". I'm reassigning to that program, along with
a patch to fix the issues I found.

[Thomas Dickey]

To ensure that I made a correct fix, I test-compiled le-1.16.3 and ran it.
Doing that, I noticed some additional issues (partly because I did not
override the makefile's C++ variables, but that was just as well, since
it prompted me to do the extra fixes):

a) renamed the private symbol find_pair to find_or_init_pair
b) include unistd.h to get prototype for write()
c) add a cast to fix a signed/unsigned compiler warning
d) add (to help with running valgrind) the ExitProgram macro
e) fix a different fail-to-build with the opaque TERMTYPE

Having done that, in a quick check the menus came up, and valgrind
had only reported an issue with linux_process_key() which I suppose
Alexander is familiar with.

There are still more than a hundred compiler warnings.

[Alexander V. Lukyanov]

Post by Thomas Dickey
c) add a cast to fix a signed/unsigned compiler warning

I will check if a newer/better version of regex.c if available in emacs (it was taken from there).

Post by Thomas Dickey
d) add (to help with running valgrind) the ExitProgram macro

Where is HAVE__NC_FREE_AND_EXIT defined?

Post by Thomas Dickey
e) fix a different fail-to-build with the opaque TERMTYPE

I don't see how these lines are equivalent:

- TERMTYPE *tp = &cur_term->type;
+ TERMTYPE *tp = (TERMTYPE *)(&cur_term);

Can you elaborate?

--
Alexander.

[Thomas Dickey]

Post by Alexander V. Lukyanov

Post by Thomas Dickey
c) add a cast to fix a signed/unsigned compiler warning

I will check if a newer/better version of regex.c if available in emacs (it was taken from there).

Post by Thomas Dickey
d) add (to help with running valgrind) the ExitProgram macro

Where is HAVE__NC_FREE_AND_EXIT defined?

It would be defined if you had (an optional) configure check for
_nc_free_and_exit.

I do this for example when debugging ncurses applications and want to check
for memory leaks. But see

http://invisible-island.net/ncurses/ncurses.faq.html#config_leaks

Post by Alexander V. Lukyanov

Post by Thomas Dickey
e) fix a different fail-to-build with the opaque TERMTYPE

- TERMTYPE *tp = &cur_term->type;
+ TERMTYPE *tp = (TERMTYPE *)(&cur_term);

They're the same because the first member of TERMINAL happens to be
a TERMTYPE, and since TERMINAL is opaque in current code (so you
cannot refer to the "type" member any longer).

[Alexander V. Lukyanov]

Post by Thomas Dickey

Post by Alexander V. Lukyanov

Post by Thomas Dickey
e) fix a different fail-to-build with the opaque TERMTYPE

- TERMTYPE *tp = &cur_term->type;
+ TERMTYPE *tp = (TERMTYPE *)(&cur_term);

They're the same because the first member of TERMINAL happens to be
a TERMTYPE, and since TERMINAL is opaque in current code (so you
cannot refer to the "type" member any longer).

But shouln't it be (TERMTYPE *)(cur_term) ?

Is there a new function or macro to get current TERMTYPE, so that I could
use it if available?

--
Alexander.

[Thomas Dickey]

Post by Alexander V. Lukyanov

Post by Thomas Dickey

Post by Alexander V. Lukyanov

Post by Thomas Dickey
e) fix a different fail-to-build with the opaque TERMTYPE

- TERMTYPE *tp = &cur_term->type;
+ TERMTYPE *tp = (TERMTYPE *)(&cur_term);

They're the same because the first member of TERMINAL happens to be
a TERMTYPE, and since TERMINAL is opaque in current code (so you
cannot refer to the "type" member any longer).

But shouln't it be (TERMTYPE *)(cur_term) ?

hmm - I agree that doesn't look right (the problem with casts).
I'm surprised it worked.

attaching an improved patch (with some additional compiler-warning fixes)

sadly enough, either way the cast gives no warnings, and the program
appears to run.

also attaching a copy of valgrind log from running the program with
the _nc_free_and_exit function turned on (just from opening a file,
looking at some menus and quitting - ymmv).

Post by Alexander V. Lukyanov
Is there a new function or macro to get current TERMTYPE, so that I could
use it if available?

/* The cast works because TERMTYPE is the first data in TERMINAL */
#define CUR ((TERMTYPE *)(cur_term))->

I'm reluctant to add a new function for accessing things that "should"
just work: the definitions in term.h are the defined interface, and
providing a function to access those in a different way doesn't seem
a good solution.

Of course, if that structure hadn't happened to be first in TERMINAL,
I'd have had to provide a function, just to keep TERMINAL opaque.

Since I'm keeping binary compatibility (aside from preventing applications
from knowing how large TERMINAL is), it's good enough for now.