Ponder compiling our packages with LTO on modern distributions

[rgacogne]

• Program: Authoritative, Recursor, dnsdist
• Issue type: Feature request

Short description

Enabling Link-Time Optimizations with a modern compiler is as easy as passing -flto (or -flto=thin for clang if we want to reduce the amount of memory needed) and allows the compiler to generate more accurate warnings, more optimized code and smaller binaries. I measured a reduction of at least 15% in binary size for dnsdist by adding -fvisibility=hidden -flto. I did not measure the performance impact (although running the unit tests takes slightly less time, but very close to the error margin).

Unless the cost in term of memory usage during the compilation is too high, I'd like to enable LTO in our packages for Bullseye, Focal and perhaps CentOS 8.

[omoerbeek]

I did a quick test on OpenBSD and it seems to work and reduce executable size, at the cost of increased link times. Compile times feel quicker. I'd have to check a full build to see how that adds up. I wonder if this is something we want for development work.

[rgacogne]

With g++ the linking time is really much longer, so I personally would not want LTO enabled during development. I was only considering our final packages, but perhaps it would make sense in CI as well?

[omoerbeek]

Having production executables that differ quite a bit from development ones worries me a bit.

CI executables used should as be close to production as possible.

[rgacogne]

CI executables used should as be close to production as possible.

I fully agree but note that we already have very different binaries since we do CI with sanitizers enabled, for example.

Having production executables that differ quite a bit from development ones worries me a bit.

I understand your point, and I would like to keep them as close as possible, but we do build for distributions we do not really do development on anyway, so the compiler, kernels and libraries differ already. I don't think we should enable LTO in our packages without testing it in CI, but I'm not too worried about not enabling it for development.

[omoerbeek]

CI executables used should as be close to production as possible.

I fully agree but note that we already have very different binaries since we do CI with sanitizers enabled, for example.

I think we also shold do runs without sanitizers. It' would not be the first time that debug aids make things work differently.

[zeha]

Debian plans to enable LTO for everything. It's possible Fedora and Ubuntu already did that. I'd be in favour of having CI builds with LTO.

[rgacogne]

Note that, for what it's worth, Arch already ships both the authoritative and recursor with LTO enabled.

[AdamMajer]

openSUSE TW has LTO enabled by default for some time now. Unfortunatelly, I get this error with dnsdist 1.8.0 RC1,

[   66s] In function 'memcpy',
[   66s]     inlined from 'move_assign' at /usr/include/boost/function/function_template.hpp:1016:24,
[   66s]     inlined from 'swap' at /usr/include/boost/function/function_template.hpp:862:22,
[   66s]     inlined from 'operator=' at /usr/include/boost/function/function_template.hpp:1155:22,
[   66s]     inlined from 'initialize' at ./ext/yahttp/yahttp/reqresp.hpp:106:33:
[   66s] /usr/include/bits/string_fortified.h:29:33: error: 'MEM <unsigned char[24]> [(char * {ref-all})&D.6589 + 8B]' is used uninitialized [-Werror=uninitialized]
[   66s]    29 |   return __builtin___memcpy_chk (__dest, __src, __len,
[   66s]       |                                 ^
[   66s] /usr/include/boost/function/function_template.hpp: In member function 'initialize':
[   66s] /usr/include/boost/function/function_template.hpp:1155:5: note: '<anonymous>' declared here
[   66s]  1155 |     self_type(f).swap(*this);
[   66s]       |     ^
[   67s] lto1: all warnings being treated as errors
[   67s] make[3]: *** [/tmp/ccxDteCr.mk:272: /tmp/cc581TSu.ltrans90.ltrans.o] Error 1
[   67s] make[3]: *** Waiting for unfinished jobs....

This is using Boost 1.81 and GCC 12.2.1

[rgacogne]

Yes, I have seen this warning as well (and your compilation seems to treat all warnings as errors), but I don't see how it can be fixed on our side, this looks like a false positive from the compiler, which might be fixable in Boost. Although, I wonder why this uses boost::function since we enable c++17, I'll have a look.

[AdamMajer]

It seems this -Werror comes from,

m4/pdns_d_fortify_source.m4:    CXXFLAGS="-Wall -W -Werror $CXXFLAGS"
m4/pdns_enable_lto.m4:    CXXFLAGS="-Wall -W -Werror $CXXFLAGS"

[rgacogne]

We set that to detect if the compiler supports that specific feature, but we restore the original flags after that. What does your ./configure line look like?

[rgacogne]

Yes, I have seen this warning as well (and your compilation seems to treat all warnings as errors), but I don't see how it can be fixed on our side, this looks like a false positive from the compiler, which might be fixable in Boost. Although, I wonder why this uses boost::function since we enable c++17, I'll have a look.

Right, so we define HAVE_CXX17 but yahttp is looking only at HAVE_CXX11 which is not defined.

[AdamMajer]

It's currently set as this, where the CFLAGS/CXXFLAGS get expanded to,

CXXFLAGS='-fmessage-length=0 -grecord-gcc-switches -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection'

and then configure is run

export CFLAGS="%{optflags} -Wno-error=deprecated-declarations -Wno-error=uninitialized"
%ifarch %arm %ix86
export CFLAGS="$CFLAGS -D__USE_TIME_BITS64"
%endif
export CXXFLAGS="$CFLAGS"

%configure \
  --enable-dnstap \
  --enable-dns-over-tls \
  --enable-systemd \
  --disable-lto \
  --enable-dnscrypt \
  --with-re2 \
  --with-ebpf \
  --with-net-snmp \
  --with-libcap \
  --with-lua=luajit \
  --with-lmdb \
  --disable-silent-rules \
  --bindir=%{_sbindir} \
  --sysconfdir=%{_sysconfdir}/%{name}/

[AdamMajer]

For the record, this is the same like in dnsdist 1.7.3 where the -Werror flag wasn't set. So I think it's leaking from one of the scripts in the 1.8.0.

[rgacogne]

Weird, with these options I get:

configure:
configure: Configuration summary
configure: =====================
configure:
configure: dnsdist configured with:  '--enable-dnstap' '--enable-dns-over-tls' '--enable-systemd' '--disable-lto' '--enable-dnscrypt' '--with-re2' '--with-ebpf' '--with-net-snmp' '--with-libcap' '--with-lua=luajit' '--with-lmdb' '--disable-silent-rules'
configure:
configure: CC: gcc (gcc (GCC) 12.2.1 20230201)
configure: CXX: g++ -std=c++17 (g++ (GCC) 12.2.1 20230201)
configure: LD: /usr/bin/ld -m elf_x86_64
configure: CFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 --param ssp-buffer-size=4 -fstack-protector -g -O3 -Wall -Wextra -Wshadow -Wno-unused-parameter -fvisibility=hidden -g -O2
configure: CPPFLAGS:
configure: CXXFLAGS:  -fPIE -DPIE -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 --param ssp-buffer-size=4 -fstack-protector -g -O3 -Wall -Wextra -Wshadow -Wno-unused-parameter -Wmissing-declarations -Wredundant-decls -fvisibility=hidden -g -O2
configure: LDFLAGS: -Wl,-z -Wl,relro -Wl,-z -Wl,now  -rdynamic
configure: LIBS:
configure: BOOST_CPPFLAGS:
configure:
configure: Features enabled
configure: ----------------
configure: Lua: luajit
configure: Protobuf: yes
configure: systemd: yes
configure: ipcipher: yes
configure: libedit: yes
configure: libsodium: yes
configure: DNSCrypt: yes
configure: dnstap: yes
configure: re2: yes
configure: SNMP: yes
configure: DNS over TLS: yes
configure: DNS over HTTPS (DoH): no
configure: GnuTLS: yes
configure: OpenSSL: yes
configure: nghttp2: yes
configure: cdb: yes
configure: lmdb: yes

[rgacogne]

Right, so we define HAVE_CXX17 but yahttp is looking only at HAVE_CXX11 which is not defined.

This will be fixed once https://github.com/PowerDNS/pdns/pull/12589 is merged.