Eoan FTBFS with gcc-9 and MySQL 8

Bug #1840511 reported by Rafael David Tinoco on 2019-08-16
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
clickhouse (Ubuntu)
Medium
Unassigned
Eoan
Medium
Unassigned
gcc-9 (Ubuntu)
Medium
Unassigned
Eoan
Medium
Unassigned

Bug Description

C and C++ differ in the form of types being defined. While C++ structs are defined also as new types, in C you have to explicitly typedef the struct to have a new type. From clickhouse source code, we are getting:

https://launchpadlibrarian.net/429654087/buildlog_ubuntu-eoan-amd64.clickhouse_18.16.1+ds-4build1_BUILDING.txt.gz

----

[ 6%] Building CXX object libs/libmysqlxx/CMakeFiles/mysqlxx.dir/src/Connection.cpp.o
cd /<<BUILDDIR>>/clickhouse-18.16.1+ds/obj-x86_64-linux-gnu/libs/libmysqlxx && /usr/bin/c++ -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libcommon/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/obj-x86_64-linux-gnu/libs/libcommon/include -I/<<BUILDDIR>>/clickhouse-18.16.1+ds/contrib/cityhash102/include -g -O2 -fdebug-prefix-map=/<<BUILDDIR>>/clickhouse-18.16.1+ds=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -pipe -fno-omit-frame-pointer -Wall -Wnon-virtual-dtor -O2 -g -DNDEBUG -O3 -flto -fno-fat-lto-objects -fPIC -std=c++17 -o CMakeFiles/mysqlxx.dir/src/Connection.cpp.o -c /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp
In file included from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/ResultBase.h:4,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/UseQueryResult.h:3,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Query.h:6,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Connection.h:10,
                 from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp:7:
/<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/include/mysqlxx/Types.h:7:23: error: conflicting declaration ‘using MYSQL = struct st_mysql’
 using MYSQL = st_mysql;
                       ^
In file included from /<<BUILDDIR>>/clickhouse-18.16.1+ds/libs/libmysqlxx/src/Connection.cpp:4:
/usr/include/mysql/mysql.h:335:3: note: previous declaration as ‘typedef struct MYSQL MYSQL’
 } MYSQL;

----

If I edit file libs/libmysqlxx/include/mysqlxx/Types.h and comment the following lines:

//struct st_mysql;
//using MYSQL = st_mysql;
//
//struct st_mysql_res;
//using MYSQL_RES = st_mysql_res;
//
//using MYSQL_ROW = char**;
//
//struct st_mysql_field;
//using MYSQL_FIELD = st_mysql_field;

and add

#include <mysql/mysql.h>

I solve the issue, as MYSQL, MYSQL_RES, MYSQL_ROW and MYSQL_FIELD types are defined by the mysql.h header file (as C++ would expect).

----

Unfortunately doing that with gcc-9 was not successful not because of the change, per se, but gcc-9 crashed during clickhouse compilation. When changing gcc-9 to gcc-8 as the default compiler (Ubuntu Eoan userland) I was able to compile clickhouse libmysqlxx library successfully.

Changed in clickhouse (Ubuntu):
status: New → In Progress
importance: Undecided → Medium
assignee: nobody → Rafael David Tinoco (rafaeldtinoco)

I have opened the following issue upstream:

https://github.com/yandex/ClickHouse/issues/6552

Explaining what was discovered and asking what would be the best alternative to address the issue (particularly interested why we were not facing this before).

I opened the following upstream issue for GCC/G++ 9:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91493

and following upstream issue for ClickHouse to track the G++ issue:

https://github.com/yandex/ClickHouse/issues/6560

I already have patches to either fix (issue #1) or mitigate (issue #2) and will propose upstream soon (testing local builds to make sure everything is right).

@gianfranco:

https://github.com/yandex/ClickHouse/issues/6552

Based on the upstream discussion, I think upstream change should be:

----

From cc17b8f0687d0b290e7a543032a87ca289290563 Mon Sep 17 00:00:00 2001
From: Rafael David Tinoco <email address hidden>
Date: Tue, 20 Aug 2019 02:38:47 +0000
Subject: [PATCH 2/2] MySQL 8 integration requires previous declaration checks

C and C++ differ in the form of types being defined. While C++ structs
are defined also as new types, in C you have to explicitly typedef the
struct to have a new type.

Fir this case, it was enough to check if MySQL header was already
defined in order not to re-declare MYSQL, MYSQL_RES, MYSQL_ROW and
MYSQL_FIELD.

Signed-off-by: Rafael David Tinoco <email address hidden>
---
 libs/libmysqlxx/include/mysqlxx/Types.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libs/libmysqlxx/include/mysqlxx/Types.h b/libs/libmysqlxx/include/mysqlxx/Types.h
index 30abdeb..b5ed709 100644
--- a/libs/libmysqlxx/include/mysqlxx/Types.h
+++ b/libs/libmysqlxx/include/mysqlxx/Types.h
@@ -3,6 +3,8 @@
 #include <cstdint>
 #include <string>

+#ifndef _mysql_h
+
 struct st_mysql;
 using MYSQL = st_mysql;

@@ -14,7 +16,7 @@ using MYSQL_ROW = char**;
 struct st_mysql_field;
 using MYSQL_FIELD = st_mysql_field;

-
+#endif

 namespace mysqlxx
 {
--
2.20.1

----

instead. Im proposing that to upstream, just so you are aware for the next merge!

Im attaching 2 patches to the case:

- Fixing MySQL building issue (proposed upstream)
- Mitigating GCC 9 issue (reported upstream)

Package maintainer already mitigated both with different patches.

clickhouse (18.16.1+ds-4ubuntu1) eoan; urgency=medium

  * Use gcc-8 to build: see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91500
  * Patch to make it build with mysql

 -- Gianfranco Costamagna <email address hidden> Tue, 20 Aug 2019 13:39:54 +0200

Forcing GCC 8 instead and including mysql.h to a C++ header. These 2 patches could be considered now or in a near future merge, when upstream issues are addressed.

Changed in mysql-8.0 (Ubuntu Eoan):
status: New → In Progress
Changed in gcc-9 (Ubuntu Eoan):
status: New → In Progress
Changed in mysql-8.0 (Ubuntu Eoan):
status: In Progress → Fix Released
Changed in clickhouse (Ubuntu Eoan):
status: In Progress → Fix Released
Changed in gcc-9 (Ubuntu Eoan):
importance: Undecided → Medium
Changed in mysql-8.0 (Ubuntu Eoan):
importance: Undecided → Medium
Changed in gcc-9 (Ubuntu Eoan):
assignee: nobody → Gianfranco Costamagna (costamagnagianfranco)
Changed in clickhouse (Ubuntu Eoan):
assignee: Rafael David Tinoco (rafaeldtinoco) → nobody
assignee: nobody → Gianfranco Costamagna (costamagnagianfranco)
Changed in mysql-8.0 (Ubuntu Eoan):
assignee: nobody → Gianfranco Costamagna (costamagnagianfranco)

The attachment "0001-G-9-crashes-returning-exception-using-true-false-syn.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

tags: added: patch

Just for the record, it wasn't the migration, it was the build tests at the end. And it looks like the deviation from expected results was regarding number formatting other than different results:

00534_exp10: FAIL - result differs with reference:

--- /<<BUILDDIR>>/clickhouse-18.16.1+ds/dbms/tests/queries/0_stateless/00534_exp10.reference 2018-12-20 16:38:50.000000000 +0000
+++ /tmp/clickhouse.test..aS0HS/tmp/0_stateless/00534_exp10.stdout 2019-08-20 12:50:21.402976662 +0000
@@ -21,7 +21,7 @@
 20 100000000000000000000
 21 1e21
 22 1e22
-23 1e23
+23 1.0000000000000001e23
 24 1e24
 25 1e25
 26 1e26
@@ -208,7 +208,7 @@
 207 1e207
 208 1e208
 209 1e209
-210 1e210
+210 1.0000000000000001e210
 211 1e211
 212 1e212
 213 1e213

I'm testing this localy AND the upstream change:

https://github.com/yandex/ClickHouse/pull/6569

Failed in a test that is being skipped by our builds (00704_drop_truncate_memory_table).

Alright, the compilation error in comment #9 is related to:

libs/libcommon/include/common/preciseExp10.h:

/** exp10 from GNU libm fails to give precise result for integer arguments.
  * For example, exp10(3) gives 1000.0000000000001
  * despite the fact that 1000 is exactly representable in double and float.
  * Better to always use implementation from MUSL.
  *
  * Note: the function names are different to avoid confusion with symbols from the system libm.
  */

They prefer using libm from MUSL, and they even have a check to check exp() precision. I guess I'll have to blacklist that test for our builds.

Changed in mysql-8.0 (Ubuntu Eoan):
status: Fix Released → In Progress

@Gian,

As clickhouse is blocking the full MySQL 8 update, and we have only 1 day until the freeze, I'll update clickhouse with the accepted upstream patch:

https://github.com/yandex/ClickHouse/issues/6552
https://github.com/yandex/ClickHouse/pull/6569

instead of yours mysql.h patch, AND we *have* to use gcc-9, so I'll remove your change to defaulting to g++8 and apply the mitigation for the g++-9 bug (I'll maintain this as delta, from Debian, until, and if, you decide to have such change there).

For the failing build (from your previous upload):
https://launchpadlibrarian.net/438156898/buildlog_ubuntu-eoan-amd64.clickhouse_18.16.1+ds-4ubuntu1_BUILDING.txt.gz

I'll try to disable that test, as we are NOT using MUSL, and they already know the issue regarding glibc libm precision for exp().

Let me know if you have any comments or concerns, please.

Best,

Rafael

Looks like that patch:

0008-Replace-cpuid-with-cpuinfo.patch

Introduced a stderr in ctests:

11:
11: Running stateless tests.
11:
11: 00802_daylight_saving_time_shift_backwards_at_midnight: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:
11: 00802_system_parts_with_datetime_partition: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:
11: 00801_daylight_saving_time_hour_underflow: [ FAIL ] - having stderror:
11: Warning in cpuinfo: kernel_max value of 8191 parsed from /sys/devices/system/cpu/kernel_max exceeds platform-default
limit 1023
11:

And tests start to fail in:

test 11
      Start 11: with_server

From cpuinfo library:

#if defined(__ANDROID__) && !defined(CPU_SETSIZE)
    /*
     * Android NDK headers before platform 21 do not define CPU_SETSIZE,
     * so we hard-code its value, as defined in platform 21 headers
     */
    #if defined(__LP64__)
        static const uint32_t default_max_processors_count = 1024;
    #else
        static const uint32_t default_max_processors_count = 32;
    #endif
#else
    static const uint32_t default_max_processors_count = CPU_SETSIZE;
#endif

AND

#if !defined(__ANDROID__)
    /*
     * sched.h is only used for CPU_SETSIZE constant.
     * Android NDK headers before platform 21 do have this constant in sched.h
     */
    #include <sched.h>
#endif

and resolving includes, it gets CPU_SETSIZE (_CPU_SETSIZE) from:

x86_64-linux-gnu/bits/cpu-set.h:

/* Size definition for CPU sets. */
#define __CPU_SETSIZE 1024
#define __NCPUBITS (8 * sizeof (__cpu_mask))

And that leads to:

uint32_t cpuinfo_linux_get_max_processors_count(void) {
...
    cpuinfo_log_warning("using platform-default max processors count = %"PRIu32, default_max_processors_count);
...
}

I have opened the following BUG:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1840847

To address this issue, for now.. I might have to drop the specific patch:

----
From: Alexander GQ Gerasiov <email address hidden>
Date: Fri, 4 Jan 2019 03:04:07 +0300
Subject: Replace cpuid with cpuinfo.

cpuinfo is already shipped in Debian and provides more suitable API.
This commit is not enough for inclusion into upstream codebase. One also
needs to replace cpuid with cpuinfo in the "contrib" directory.

Signed-off-by: Alexander GQ Gerasiov <email address hidden>
----

That introduced this issue.

Im currently building the to-be-suggested source package locally so I can push it to a PPA for the debdiff reviewers before final upload.

Instead of reverting the libcpuid change (done in Debian and not upstream) I will make libcpuid not to stderr for warnings for now:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1840847

And this might fix comment #14 issue.

Fixing libcpuid I was able to run all tests and, apart from those already marked as buggy and/or flaky, in debian/rules, I'm adding those extra for now:

#
# LP: #1840511 cleanup
#

# libm precision on exp() related functions
SKIP_TESTS+=00536 00534

# long_http_bufferization ?
SKIP_TESTS+=00429

# system processes port ? (bind: error already in use)
SKIP_TESTS+=00379

# clickhouse-clang not found
SKIP_TESTS+=00281

# end of LP: #1840511 cleanup

They don't seem to be indicating real issues with this build of clickhouse.

I guess if tests make the build hard to happen we can just enable:

 #export DEB_BUILD_OPTIONS+=nocheck

in debian/rules but, for now, I tried to avoid that.

PPA containing clickhouse build and cpuinfo, with needed fix from (LP: #1840847):

https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1840511

Debdiff with proposed changes are attached to this bug.

Well it looks like clickhouse is an amd64 only package, I have added more architectures in the PPA, please ignore those building errors.

Changed in clickhouse (Ubuntu Eoan):
status: Fix Released → In Progress
Changed in mysql-8.0 (Ubuntu Eoan):
status: In Progress → Invalid

I uploaded after changing some little bit to make the changes upstreamable to Debian

Changed in mysql-8.0 (Ubuntu Eoan):
assignee: Gianfranco Costamagna (costamagnagianfranco) → nobody
Changed in gcc-9 (Ubuntu Eoan):
assignee: Gianfranco Costamagna (costamagnagianfranco) → nobody
no longer affects: mysql-8.0 (Ubuntu Eoan)
no longer affects: mysql-8.0 (Ubuntu)
Changed in clickhouse (Ubuntu Eoan):
status: In Progress → Fix Committed
status: Fix Committed → Fix Released
Changed in gcc-9 (Ubuntu Eoan):
status: In Progress → Fix Released
Changed in clickhouse (Ubuntu Eoan):
assignee: Gianfranco Costamagna (costamagnagianfranco) → nobody
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.