freeekanayaka
commented
9 months ago Probably this is due to libraft being compiled without -fno-strict-aliasing.
Closed kloczek closed 7 months ago
freeekanayaka
commented
9 months ago Probably this is due to libraft being compiled without -fno-strict-aliasing.
kloczek
commented
9 months ago Indeed it is the case. Code should not rely on optimisation flags.
kloczek
commented
9 months ago Strange is that compiler do not warns that some code may be incorrectly optimised because aliasing use 🤔
kloczek
commented
7 months ago @freeekanayaka could please have look on above issue? 🤔
cole-miller
commented
7 months ago Note that Free is now working on https://github.com/cowsql and not on dqlite. The dqlite maintainers are @just-now, @MathieuBordere, and myself.
As for the issue at hand, if the current raft code depends on strict aliasing being disabled for correctness, that's arguably unfortunate but not a priority for us to fix.
kloczek
commented
7 months ago As for the issue at hand, if the current raft code depends on strict aliasing being disabled for correctness, that's arguably unfortunate but not a priority for us to fix.
Depending on strict aliasing to be disabled is always nothing more than asking for trouble. it would be really good to modify the code to remove that dependency.
Just checked and dqlite test suite still fails if raft is compiled with -fno-strict-aliasing
freeekanayaka
commented
7 months ago @kloczek FWIW this has been fixed in cowsql/raft:
kloczek
commented
7 months ago One sec please let me check that 😋
kloczek
commented
7 months ago Just tested and now build fails
[tkloczko@pers-jacek dqlite-1.16.2]$ make -k
CC src/libdqlite_la-server.lo
src/server.c: In function 'dqlite__init':
src/server.c:123:9: error: implicit declaration of function 'raft_register_state_cb' [-Wimplicit-function-declaration]
123 | raft_register_state_cb(&d->raft, state_cb);
| ^~~~~~~~~~~~~~~~~~~~~~
src/server.c:123:9: error: nested extern declaration of 'raft_register_state_cb' [-Werror=nested-externs]
cc1: all warnings being treated as errors
make: *** [Makefile:1611: src/libdqlite_la-server.lo] Error 1
make: Target 'all' not remade because of errors.
[tkloczko@pers-jacek dqlite-1.16.2]$ make -k V=1
/bin/sh ./libtool --tag=CC --mode=compile /usr/bin/gcc -DPACKAGE_NAME=\"libdqlite\" -DPACKAGE_TARNAME=\"libdqlite\" -DPACKAGE_VERSION=\"1.16.2\" -DPACKAGE_STRING=\"libdqlite\ 1.16.2\" -DPACKAGE_BUGREPORT=\"https://github.com/canonical/dqlite\" -DPACKAGE_URL=\"\" -DPACKAGE=\"libdqlite\" -DVERSION=\"1.16.2\" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DHAVE_WCHAR_H=1 -DSTDC_HEADERS=1 -D_ALL_SOURCE=1 -D_DARWIN_C_SOURCE=1 -D_GNU_SOURCE=1 -D_HPUX_ALT_XOPEN_SOCKET_API=1 -D_NETBSD_SOURCE=1 -D_OPENBSD_SOURCE=1 -D_POSIX_PTHREAD_SEMANTICS=1 -D__STDC_WANT_IEC_60559_ATTRIBS_EXT__=1 -D__STDC_WANT_IEC_60559_BFP_EXT__=1 -D__STDC_WANT_IEC_60559_DFP_EXT__=1 -D__STDC_WANT_IEC_60559_EXT__=1 -D__STDC_WANT_IEC_60559_FUNCS_EXT__=1 -D__STDC_WANT_IEC_60559_TYPES_EXT__=1 -D__STDC_WANT_LIB_EXT2__=1 -D__STDC_WANT_MATH_SPEC_FUNCS__=1 -D_TANDEM_SOURCE=1 -D__EXTENSIONS__=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_ARPA_INET_H=1 -DHAVE_FCNTL_H=1 -DHAVE_STDINT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_SYS_SOCKET_H=1 -DHAVE_UNISTD_H=1 -I. -std=c11 -g3 -fcf-protection --param=ssp-buffer-size=4 -pipe -fno-strict-aliasing -fdiagnostics-color -fexceptions -fstack-clash-protection -fstack-protector-strong -fasynchronous-unwind-tables -fdiagnostics-show-option -Wall -Wextra -Wimplicit-fallthrough=5 -Wcast-align -Wstrict-prototypes -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Wformat=2 -Wshadow -Wendif-labels -Wdate-time -Wnested-externs -Wconversion -Werror -pthread -O2 -fvisibility=hidden -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -O2 -g -grecord-gcc-switches -pipe -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdata-sections -ffunction-sections -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -flto=auto -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -Wall -Werror=format-security -fno-strict-aliasing -c -o src/libdqlite_la-server.lo `test -f 'src/server.c' || echo './'`src/server.c
libtool: compile: /usr/bin/gcc -DPACKAGE_NAME=\"libdqlite\" -DPACKAGE_TARNAME=\"libdqlite\" -DPACKAGE_VERSION=\"1.16.2\" "-DPACKAGE_STRING=\"libdqlite 1.16.2\"" -DPACKAGE_BUGREPORT=\"https://github.com/canonical/dqlite\" -DPACKAGE_URL=\"\" -DPACKAGE=\"libdqlite\" -DVERSION=\"1.16.2\" -DHAVE_STDIO_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_STRINGS_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_UNISTD_H=1 -DHAVE_WCHAR_H=1 -DSTDC_HEADERS=1 -D_ALL_SOURCE=1 -D_DARWIN_C_SOURCE=1 -D_GNU_SOURCE=1 -D_HPUX_ALT_XOPEN_SOCKET_API=1 -D_NETBSD_SOURCE=1 -D_OPENBSD_SOURCE=1 -D_POSIX_PTHREAD_SEMANTICS=1 -D__STDC_WANT_IEC_60559_ATTRIBS_EXT__=1 -D__STDC_WANT_IEC_60559_BFP_EXT__=1 -D__STDC_WANT_IEC_60559_DFP_EXT__=1 -D__STDC_WANT_IEC_60559_EXT__=1 -D__STDC_WANT_IEC_60559_FUNCS_EXT__=1 -D__STDC_WANT_IEC_60559_TYPES_EXT__=1 -D__STDC_WANT_LIB_EXT2__=1 -D__STDC_WANT_MATH_SPEC_FUNCS__=1 -D_TANDEM_SOURCE=1 -D__EXTENSIONS__=1 -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_ARPA_INET_H=1 -DHAVE_FCNTL_H=1 -DHAVE_STDINT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_SYS_SOCKET_H=1 -DHAVE_UNISTD_H=1 -I. -std=c11 -g3 -fcf-protection --param=ssp-buffer-size=4 -pipe -fno-strict-aliasing -fdiagnostics-color -fexceptions -fstack-clash-protection -fstack-protector-strong -fasynchronous-unwind-tables -fdiagnostics-show-option -Wall -Wextra -Wimplicit-fallthrough=5 -Wcast-align -Wstrict-prototypes -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Wformat=2 -Wshadow -Wendif-labels -Wdate-time -Wnested-externs -Wconversion -Werror -pthread -O2 -fvisibility=hidden -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -O2 -g -grecord-gcc-switches -pipe -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdata-sections -ffunction-sections -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -flto=auto -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -Wall -Werror=format-security -fno-strict-aliasing -c src/server.c -fPIC -DPIC -o src/.libs/libdqlite_la-server.o
src/server.c: In function 'dqlite__init':
src/server.c:123:9: error: implicit declaration of function 'raft_register_state_cb' [-Wimplicit-function-declaration]
123 | raft_register_state_cb(&d->raft, state_cb);
| ^~~~~~~~~~~~~~~~~~~~~~
src/server.c:123:9: error: nested extern declaration of 'raft_register_state_cb' [-Werror=nested-externs]
cc1: all warnings being treated as errorsand ..
[tkloczko@pers-jacek dqlite-1.16.2]$ rpm -ql raft-devel | grep include\*.h$ | xargs grep raft_register_state_cb
[tkloczko@pers-jacek dqlite-1.16.2]$Probably there was some confusion, when I wrote that this was fixed I meant the strict aliasing issue, not this missing function.
I could add that function to cowsql/raft for compatibility, but does not make much sense since dqlite is now going to ship with the raft source included.
I'd suggest that you just wait for #568 to be merged. After that you won't need any external raft library to build dqlite.
kloczek
commented
7 months ago I could add that function to cowsql/raft for compatibility, but does not make much sense since dqlite is now going to ship with the raft source included.
But why? 🤔 Why dqlite cannot be build against system installed raft? 🤔 Budling other projects code is always nothing more tan asking for troubles ..
freeekanayaka
commented
7 months ago I could add that function to cowsql/raft for compatibility, but does not make much sense since dqlite is now going to ship with the raft source included.
But why? 🤔 Why dqlite cannot be build against system installed raft? 🤔 Budling other projects code is always nothing more tan asking for troubles ..
Dqlite 1.16.0 is compatible with cowsql/raft (which is a fork of canonical/raft), starting from dqlite version 1.16.2 you need to use canonical/raft because new interfaces were added to it that are not present in cowsql/raft. Since canonical/raft is going to be merged into dqlite directly (see #568) I will not fix cowsql/raft to work with dqlite 1.16.2. Just wait for dqlite 1.16.3 and you will not need neither canonical/raft or cowsql/raft.
kloczek
commented
7 months ago Dqlite 1.16.0 is compatible with cowsql/raft (which is a fork of canonical/raft), starting from dqlite version 1.16.2 you need to use canonical/raft because new interfaces were added to it that are not present in cowsql/raft. Since canonical/raft is going to be merged into dqlite directly (see https://github.com/canonical/dqlite/pull/568) I will not fix cowsql/raft to work with dqlite 1.16.2. Just wait for dqlite 1.16.3 and you will not need neither canonical/raft or cowsql/raft.
Does it mean that raft project will be no longer maintained and will be archived? 🤔
freeekanayaka
commented
7 months ago Dqlite 1.16.0 is compatible with cowsql/raft (which is a fork of canonical/raft), starting from dqlite version 1.16.2 you need to use canonical/raft because new interfaces were added to it that are not present in cowsql/raft. Since canonical/raft is going to be merged into dqlite directly (see #568) I will not fix cowsql/raft to work with dqlite 1.16.2. Just wait for dqlite 1.16.3 and you will not need neither canonical/raft or cowsql/raft.
Does it mean that
raftproject will be no longer maintained and will be archived? 🤔
canonical/raft will be merged into canonical/dqlite, it will be a single repository with both source codes and a single shared library.
cowsql/raft will continue to be a standalone repository and library.
kloczek
commented
7 months ago
canonical/raftwill be merged intocanonical/dqlite, it will be a single repository with both source codes and a single shared library.
cowsql/raftwill continue to be a standalone repository and library.
So what is the sense use forked version and why not port dqlite to cowsql/raft? 🤔
freeekanayaka
commented
7 months ago
canonical/raftwill be merged intocanonical/dqlite, it will be a single repository with both source codes and a single shared library.cowsql/raftwill continue to be a standalone repository and library.So what is the sense use forked version and why not port dqlite to cowsql/raft? 🤔
dqlite has been forked too: https://github.com/cowsql/cowsql and that fork uses cowsql/raft. Cowsql is currently being used by Incus.
EDIT: As said, my plan was to keep cowsql/raft compatible with dqlite, and in fact it is compatible up to dqlite 1.16.0. However there's now no reason to maintain compatibility, since dqlite is going to embed raft in its own source code.
kloczek
commented
7 months ago dqlite has been forked too: https://github.com/cowsql/cowsql and that fork uses cowsql/raft. Cowsql is currently being used by Incus.
OK thank you to let me know. (I'm switching my build procedures to use those two repos)
EDIT: As said, my plan was to keep cowsql/raft compatible with dqlite, and in fact it is compatible up to dqlite 1.16.0. However there's now no reason to maintain compatibility, since dqlite is going to embed raft in its own source code.
Which is as always nothing more than asking for troubles ..
Looks like test suite two units are faling.
I'm not sure how can I try to diagnose that so will be appreciated for any hints what I can try to do that.