Gentoolto: app-shells/bash & sys-libs/readline false positive with LTO & O3 on configure MUST_REINSTALL_SIGHANDLERS

[zhuyifei1999]

Reproduced on latest ~amd64:
=sys-devel/gcc-7.3.0-r3 USE="cxx fortran multilib nls nptl openmp pch pgo pie sanitize ssp vtv"
=app-shells/bash-4.4_p23
=sys-libs/readline-7.0_p5

CFLAGS='-march=native -O3 -flto=4 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu'
LDFLAGS='-Wl,--as-needed -Wl,--hash-style=gnu'
(Copied from make.conf.lto, with LTO enabled (4 threads) and Graphite disabled, and add -ggdb)

The symptom: in a maximized terminal window with bash running,

• force a new prompt (by pressing enter on a blank prompt)
• type a character
• un-maximize it (as long as the horizontal size is decreased)
• type another character (can be the same)
• maximize again

In this state, long bash commands (typed or scrolled in bash history) will, somehow, not wrap to new lines, but overwrite the current line.

By strace debugging, it is visible that when bash echoes the commands, it outputs carriage returns \r where it expect the line to wrap in the smaller terminal window; however, because that incorrect expectation of line length, it starts 'wrapping' by \r before the line is exhausted, and \r in this case incorrectly wraps to the beginning of the current line.
It is also seen in strace that on each terminal resize SIGWINCH is correctly delivered to the bash process.

By gdb debugging, using an a gdb script to check the active signal handler for SIGWINCH (28), it is seen that on a new prompt the handler is rl_sigwinch_handler in section .text of /lib64/libreadline.so.7, but after the signal is delivered it is changed to sigwinch_sighandler in section .text of /bin/bash.

By looking at the relevant code (1, 2), it is clear that readline's implementation of SIGWINCH handler calls the old handler (in this case bash's), besides doing its own stuffs, upon receiving the signal. Further playing around with gdb (backtraceing, breakpointing, evaluating, etc.) confirms that it is readline's signal handler doing the real work of determining the window size, and it is readline's code that process the wrapping.

The theory would be that, MUST_REINSTALL_SIGHANDLERS is defined at least for the case of bash, and that readline invokes bash's SIGWINCH handler, which overwrites the signal handler table to set itself as the handler, and this handler does pretty much nothing. This macro definition is confirmed for both cases by re-emergeing, in which, using the above CFLAGS, in the ./configure logs:

/var/tmp/portage/sys-libs/readline-7.0_p5/work/readline-7.0/configure --prefix=/usr --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc --localstatedir=/var/lib --docdir=/usr/share/doc/readline-7.0_p5 --htmldir=/usr/share/doc/readline-7.0_p5/html --libdir=/usr/lib64 --cache-file=/var/tmp/portage/sys-libs/readline-7.0_p5/work/readline-7.0-abi_x86_64.amd64/config.cache --with-curses --disable-static
[...]
checking if signal handlers must be reinstalled when invoked... yes
[...]
./configure --prefix=/usr --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc --localstatedir=/var/lib --docdir=/usr/share/doc/bash-4.4_p23 --htmldir=/usr/share/doc/bash-4.4_p23/html --libdir=/usr/lib64 --disable-profiling --docdir=$(datarootdir)/doc/bash-4.4_p23 --htmldir=$(docdir)/html --with-curses --disable-mem-scramble --enable-net-redirections --enable-readline --enable-bang-history --enable-history --without-afs --without-bash-malloc --with-installed-readline=.
[...]
checking if signal handlers must be reinstalled when invoked... yes
[...]

The M4 macro used to generate the test is the same in both cases (1, 2). The test C code is pretty much:

#define HAVE_POSIX_SIGNALS
#define RETSIGTYPE void
#define HAVE_UNISTD_H

#include <signal.h>
#ifdef HAVE_UNISTD_H
#include <unistd.h>
#endif

typedef RETSIGTYPE sigfunc();

int nsigint;

#ifdef HAVE_POSIX_SIGNALS
sigfunc *
set_signal_handler(sig, handler)
     int sig;
     sigfunc *handler;
{
  struct sigaction act, oact;
  act.sa_handler = handler;
  act.sa_flags = 0;
  sigemptyset (&act.sa_mask);
  sigemptyset (&oact.sa_mask);
  sigaction (sig, &act, &oact);
  return (oact.sa_handler);
}
#else
#define set_signal_handler(s, h) signal(s, h)
#endif

RETSIGTYPE
sigint(s)
int s;
{
  nsigint++;
}

main()
{
    nsigint = 0;
    set_signal_handler(SIGINT, sigint);
    kill((int)getpid(), SIGINT);
    kill((int)getpid(), SIGINT);
    exit(nsigint != 2);
}

$ gcc -march=native -O3 -flto=4 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.c -o test -w; ./test; echo $?
1



md5-a4298824b3722c491720aced8011912b

$ gcc -march=native -O3 -flto=1 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.c -o test -w; ./test; echo $?
handler 1
handler 2
exit 0
0

Somehow, it was not reading the new `nsigint` value.

And now the most interesting part: with any -O > 0 and -flto, GCC simply optimizes out the return code:
```diff
diff -u <(gcc -march=native -O1 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.old.c -o nolto -w && gdb -batch -ex 'file nolto' -ex 'disassemble main') <(gcc -march=native -O1 -flto=0 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.old.c -o lto -w && gdb -batch -ex 'file lto' -ex 'disassemble main')
--- /dev/fd/63  2018-08-02 19:13:21.539065571 +0800
+++ /dev/fd/62  2018-08-02 19:13:21.544065532 +0800
@@ -13,8 +13,6 @@
    0x0000000000000887 <+58>:   mov    $0x2,%esi
    0x000000000000088c <+63>:   mov    %eax,%edi
    0x000000000000088e <+65>:   callq  0x680 <kill@plt>
-   0x0000000000000893 <+70>:   cmpl   $0x2,0x2007c2(%rip)        # 0x20105c <nsigint>
-   0x000000000000089a <+77>:   setne  %dil
-   0x000000000000089e <+81>:   movzbl %dil,%edi
-   0x00000000000008a2 <+85>:   callq  0x690 <exit@plt>
+   0x0000000000000893 <+70>:   mov    $0x1,%edi
+   0x0000000000000898 <+75>:   callq  0x690 <exit@plt>
 End of assembler dump.

This causes the whole chain: LTO -> exit(nsigint != 2) optimized to exit(1) -> bash & readline compiled with MUST_REINSTALL_SIGHANDLERS -> bash & readline compete for SIGWINCH signal handler installation -> chaos.

I suggest turning off LTO by default for bash & readline.

[zhuyifei1999]

Just to double check this isn't fixed by upgrading GCC (considering the 'major number' changed between 7.3.0 and 8.2.0), I emerged gcc:8.2.0 with same USE flags, compiled the test code with 8.2.0 explicitly, and was still able to reproduce the issue:

$ gcc-8.2.0 -march=native -O3 -flto=4 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.old.c -o test -w; ./test; echo $?; gdb -batch -ex 'file test' -ex 'disassemble main'
1
Dump of assembler code for function main:
   0x00000000000006b0 <+0>: sub    $0x8,%rsp
   0x00000000000006b4 <+4>: xor    %eax,%eax
   0x00000000000006b6 <+6>: movl   $0x0,0x20099c(%rip)        # 0x20105c <nsigint>
   0x00000000000006c0 <+16>:    callq  0x800 <set_signal_handler>
   0x00000000000006c5 <+21>:    callq  0x650 <getpid@plt>
   0x00000000000006ca <+26>:    mov    %eax,%edi
   0x00000000000006cc <+28>:    mov    $0x2,%esi
   0x00000000000006d1 <+33>:    callq  0x680 <kill@plt>
   0x00000000000006d6 <+38>:    callq  0x650 <getpid@plt>
   0x00000000000006db <+43>:    mov    %eax,%edi
   0x00000000000006dd <+45>:    mov    $0x2,%esi
   0x00000000000006e2 <+50>:    callq  0x680 <kill@plt>
   0x00000000000006e7 <+55>:    mov    $0x1,%edi
   0x00000000000006ec <+60>:    callq  0x690 <exit@plt>
End of assembler dump.
$ gcc-8.2.0 --version
gcc-8.2.0 (Gentoo 8.2.0 p1.3) 8.2.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[InBetweenNames]

Wow, great work!! I will disable LTO for bash and readline accordingly. I guess I never noticed because I use app-shells/fish xD.

Based on your excellent writeup, I think the problem is that the compiler is assuming the nsigint variable will not be updated by other processes. But this is exactly what could happen during a signal handler! To test this, I modified your code as follows, using the volatile type qualifier:

~~~
12c12

< int nsigint;

volatile int nsigint;
~~~

Sure enough:

~~~

gcc -march=native -O3 -flto=4 -fuse-linker-plugin -pipe -ggdb -Wl,--as-needed -Wl,--hash-style=gnu test.c -o test -w; ./test; echo $status
0
~~~

It fixed it!

I suspect a similar problem is happening inside of app-shells/bash and/or sys-libs/readline. Would make a great pull request to find that bug!

[InBetweenNames]

Patches created upstream:
https://savannah.gnu.org/patch/index.php?9676
https://savannah.gnu.org/patch/index.php?9677

[InBetweenNames]

One more thing: it actually needs to be volatile sig_atomic_t as per the C standard section 5.1.2.3 paragraph 5

[zhuyifei1999]

Not important, but I think you linked to the wrong issue at savannah :P

Anyways, thanks for the pointer to volatile sig_atomic_t. I wasn't aware of that type (so many _ts)

[InBetweenNames]

Ah damn, I did! I'll update the post. Thanks!

[InBetweenNames]

Considering making this an /etc/patches fix for now since I have not heard from upstream at all.

[InBetweenNames]

I went ahead and added these to the userpatches for now, since upstream has been quiet.

[InBetweenNames]

Closing issue for now -- will reopen when I hear from upstream.

[InBetweenNames]

The bash patch was accepted upstream!

[wolfwood]

• Applying lto.patch ...
  1 out of 1 hunk FAILED -- saving rejects to file aclocal.m4.rej
  [ !! ]
• ERROR: sys-libs/readline-7.0_p5::gentoo failed (prepare phase):
• patch -p1 failed with /etc/portage/patches/sys-libs/readline/lto.patch

looks like the patch is now coming from upstream.

[InBetweenNames]

@wolfwood , I tried on my system and it seems the patch still applies -- could you submit your build.log? I notice it was accepted into readline 8.0, but it doesn't seem to have propagated back to 7.0_p5.

[wolfwood]

readline.build.log

[wolfwood]

readline.build.log

I noticed after the patch system change I had a dispatch-conf update to /etc moving over /etc/portage/patches/sys-libs to a symlink. but dispatch-conf didn't seem to ever actually do the change, I had to do it manually to clear the prompting.

wolfwood@BMO /etc/portage $ ls -l /etc/portage/patches 
total 8
drwxr-xr-x 3 root root 4096 Aug  4 20:25 sys-devel
lrwxrwxrwx 1 root root   68 Oct 16 01:27 sys-libs -> /var/lib/layman/lto-overlay/sys-config/ltoize/files/patches/sys-libs

did I do it wrong?

[InBetweenNames]

Ahh I see what happened here -- the patch is being applied twice. This is actually my fault, as I should have added more smarts to the migration rather than just leave the existing symlinks intact if there were custom things in that directory. What you can do in your case is simply remove the symlink /etc/portage/patches/sys-libs and everything should once again work fine. The patches are actually applied via a separate script in bashrc.d now to avoid the need for installing those symlinks. Could you try that and see if it helps?

[wolfwood]

yup, that fixed it, thanks!

[InBetweenNames]

Accepted upstream as of #226!