ci: Bump to clang-8 for asan build to avoid segfaults on ppc64le #17517

MarcoFalke commented at 5:04 PM on November 19, 2019: member

Use clang-8 instead of default clang (which is clang-6 on Bionic) to avoid spurious segfaults when running the ci system on ppc64le

fanquake added the label Tests on Nov 19, 2019

practicalswift commented at 5:16 PM on November 19, 2019: contributor

ACK fa8d6bfb6f6f391a3a6af1fb47b9ca5f53344f1b assuming Travis is also happy: diff looks correct

Concept ACK

fanquake commented at 8:01 PM on November 19, 2019: member

Seeing some Travis failures in the asan build:

serialize.h:211:93: runtime error: implicit conversion from type 'int64_t' (aka 'long') of value -1 (64-bit, signed) to type 'uint64_t' (aka 'unsigned long') changed the value to 18446744073709551615 (64-bit, unsigned)
    [#0](/bitcoin-bitcoin/0/) 0x55cb0c158d0b in void Serialize<CSizeComputer>(CSizeComputer&, long) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:211:93
    [#1](/bitcoin-bitcoin/1/) 0x55cb0c158b45 in void SerializeMany<CSizeComputer, long>(CSizeComputer&, long const&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:964:5
    [#2](/bitcoin-bitcoin/2/) 0x55cb0c158b45 in void SerReadWriteMany<CSizeComputer, long>(CSizeComputer&, CSerActionSerialize, long const&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:983
    [#3](/bitcoin-bitcoin/3/) 0x55cb0c158b45 in void CTxOut::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./primitives/transaction.h:150
    [#4](/bitcoin-bitcoin/4/) 0x55cb0c42df66 in void CTxOut::Serialize<CSizeComputer>(CSizeComputer&) const /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./primitives/transaction.h:146:5
    [#5](/bitcoin-bitcoin/5/) 0x55cb0c42df66 in void Serialize<CSizeComputer, CTxOut>(CSizeComputer&, CTxOut const&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:602
    [#6](/bitcoin-bitcoin/6/) 0x55cb0c42df66 in CSizeComputer& CSizeComputer::operator<<<CTxOut>(CTxOut const&) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:945
    [#7](/bitcoin-bitcoin/7/) 0x55cb0c42aeae in unsigned long GetSerializeSize<CTxOut>(CTxOut const&, int) /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/./serialize.h:1006:37
    [#8](/bitcoin-bitcoin/8/) 0x55cb0b9ea299 in __cxx_global_var_init.12 /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/coins.cpp:249:76
    [#9](/bitcoin-bitcoin/9/) 0x55cb0b9ea299 in _GLOBAL__sub_I_coins.cpp /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/coins.cpp
    [#10](/bitcoin-bitcoin/10/) 0x55cb0d26c32c in __libc_csu_init (/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x3b5e32c)
    [#11](/bitcoin-bitcoin/11/) 0x7effa9a2db27 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b27)
    [#12](/bitcoin-bitcoin/12/) 0x55cb0b9ecaa9 in _start (/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x22deaa9)

test/crypto_tests.cpp:192:16: runtime error: implicit conversion from type 'unsigned char' of value 128 (8-bit, unsigned) to type 'char' changed the value to -128 (8-bit, signed)
    [#0](/bitcoin-bitcoin/0/) 0x55f6ff14f838 in crypto_tests::LongTestString[abi:cxx11]() /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/crypto_tests.cpp:192:16
    [#1](/bitcoin-bitcoin/1/) 0x55f6fecc8ae1 in __cxx_global_var_init.14 /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/crypto_tests.cpp:201:27
    [#2](/bitcoin-bitcoin/2/) 0x55f6fecc8ae1 in _GLOBAL__sub_I_crypto_tests.cpp /home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/crypto_tests.cpp
    [#3](/bitcoin-bitcoin/3/) 0x55f700b1339c in __libc_csu_init (/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x491a39c)
    [#4](/bitcoin-bitcoin/4/) 0x7f4f5878eb27 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b27)
    [#5](/bitcoin-bitcoin/5/) 0x55f6fed0ea99 in _start (/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x2b15a99)

DrahtBot commented at 10:09 PM on November 19, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17549 (ci: misc cleanups by MarcoFalke)
#12134 (Build previous releases and run functional tests by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake added the label Waiting for author on Nov 20, 2019

DrahtBot added the label Needs rebase on Nov 20, 2019

MarcoFalke removed the label Waiting for author on Nov 20, 2019

MarcoFalke force-pushed on Nov 20, 2019

MarcoFalke commented at 7:29 PM on November 20, 2019: member

@practicalswift I am having issues adding suppressions. See

https://travis-ci.org/bitcoin/bitcoin/jobs/614670639#L4122 https://github.com/bitcoin/bitcoin/pull/17517/commits/a9b6a758e38f42d56d11ad890cdbd0480c9c6114#diff-782c38017801582a8e7fe8245d9d0b15R48

Any idea what I could have made wrong?

DrahtBot removed the label Needs rebase on Nov 20, 2019

practicalswift commented at 9:21 AM on November 21, 2019: contributor

@MarcoFalke I think this is a UBSan suppressions bug in Clang 8 which I believe has been fixed in later versions.

I think the best way to work around the bug is to apply sanitization opt-outs for the triggering functions using __attribute__((no_sanitize("integer"))).

This illustrates the bug and also the workaround.

A testcase and the suppresions file:

$ cat implicit.cpp
signed char f(unsigned int i) { return i; }

int main() { return f(254); }
$ cat ubsan
implicit-signed-integer-truncation:implicit.cpp

The runtime suppression implicit-signed-integer-truncation is not applied when the binary is compiled with -fsanitize=integer:

$ clang++-8 -g -fsanitize=integer -o implicit-sanitize-integer implicit.cpp
$ ./implicit-sanitize-integer
implicit.cpp:1:40: runtime error: implicit conversion from type 'unsigned int' of value 254 (32-bit, unsigned) to type 'signed char' changed the value to -2 (8-bit, signed)
$ UBSAN_OPTIONS=suppressions=ubsan ./implicit-sanitize-integer
implicit.cpp:1:40: runtime error: implicit conversion from type 'unsigned int' of value 254 (32-bit, unsigned) to type 'signed char' changed the value to -2 (8-bit, signed)
$

On the other hand when using the more fine grained -fsanitize=implicit-integer-truncation (a subset of -fsanitize=integer) the suppression works as expected:

$ clang++-8 -g -fsanitize=implicit-integer-truncation -o implicit-sanitize-implicit-integer-truncation implicit.cpp
$ ./implicit-sanitize-implicit-integer-truncation
implicit.cpp:1:40: runtime error: implicit conversion from type 'unsigned int' of value 254 (32-bit, unsigned) to type 'signed char' changed the value to -2 (8-bit, signed)
$ UBSAN_OPTIONS=suppressions=ubsan ./implicit-sanitize-implicit-integer-truncation
$

The problem is that we don't want to limit us to-fsanitize=integer (and -fsanitize=integer,implicit-integer-truncation does not work unfortunately), so the best we can do is:

$ cat implicit-with-opt-out.cpp
#if defined(__clang__)
__attribute__((no_sanitize("integer")))
#endif
signed char f(unsigned int i) { return i; }

int main() { return f(254); }
$ clang++-8 -g -fsanitize=integer -o implicit-with-opt-out implicit-with-opt-out.cpp
$ ./implicit-with-opt-out
$

Perhaps an annotation macro INTEGER_SANITIZATION_OPT_OUT can be introduced making it INTEGER_SANITIZATION_OPT_OUT signed char f(unsigned int i).

practicalswift commented at 5:41 PM on November 21, 2019: contributor

@MarcoFalke The UBSAN_OPTIONS=report_error_type=1 could perhaps be helpful too. It will print the correct suppression code as part of error output. IIRC that option was present but broken in Clang 8, and fixed in later versions.

MarcoFalke commented at 6:45 PM on November 21, 2019: member

@practicalswift Any idea in what clang version this was fixed. I tried clang-9 and it still happens:

chain.h:430:16: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'int' changed the value to -1 (32-bit, signed)
    [#0](/bitcoin-bitcoin/0/) 0x556e26efa8ad in CChain::Height() const /home/marco/bitcoin/src/./chain.h:430:16
    [#1](/bitcoin-bitcoin/1/) 0x556e275efa97 in CChainState::RewindBlockIndex(CChainParams const&) /home/marco/bitcoin/src/validation.cpp:4435:35
    [#2](/bitcoin-bitcoin/2/) 0x556e275f0cb1 in RewindBlockIndex(CChainParams const&) /home/marco/bitcoin/src/validation.cpp:4508:31
    [#3](/bitcoin-bitcoin/3/) 0x556e26f3370f in AppInitMain(NodeContext&) /home/marco/bitcoin/src/init.cpp:1570:22
    [#4](/bitcoin-bitcoin/4/) 0x556e26f01319 in interfaces::(anonymous namespace)::NodeImpl::appInitMain() /home/marco/bitcoin/src/interfaces/node.cpp:81:16
    [#5](/bitcoin-bitcoin/5/) 0x556e26a2be5b in BitcoinCore::initialize() /home/marco/bitcoin/src/qt/bitcoin.cpp:147:26
    [#6](/bitcoin-bitcoin/6/) 0x556e26a42893 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (BitcoinCore::*)()>::call(void (BitcoinCore::*)(), BitcoinCore*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:152:13
    [#7](/bitcoin-bitcoin/7/) 0x556e26a42583 in void QtPrivate::FunctionPointer<void (BitcoinCore::*)()>::call<QtPrivate::List<>, void>(void (BitcoinCore::*)(), BitcoinCore*, void**) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:185:13
    [#8](/bitcoin-bitcoin/8/) 0x556e26a42583 in QtPrivate::QSlotObject<void (BitcoinCore::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:414:17
    [#9](/bitcoin-bitcoin/9/) 0x7fa202b39eb9 in QObject::event(QEvent*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2b1eb9)
    [#10](/bitcoin-bitcoin/10/) 0x7fa202143a85 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x16aa85)
    [#11](/bitcoin-bitcoin/11/) 0x7fa20214cdff in QApplication::notify(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x173dff)
    [#12](/bitcoin-bitcoin/12/) 0x7fa202b0da99 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x285a99)
    [#13](/bitcoin-bitcoin/13/) 0x7fa202b10717 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x288717)
    [#14](/bitcoin-bitcoin/14/) 0x7fa202b660a6  (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2de0a6)
    [#15](/bitcoin-bitcoin/15/) 0x7fa2007f084c in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5184c)
    [#16](/bitcoin-bitcoin/16/) 0x7fa2007f0acf  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51acf)
    [#17](/bitcoin-bitcoin/17/) 0x7fa2007f0b72 in g_main_context_iteration (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b72)
    [#18](/bitcoin-bitcoin/18/) 0x7fa202b656a4 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2dd6a4)
    [#19](/bitcoin-bitcoin/19/) 0x7fa202b0c63a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28463a)
    [#20](/bitcoin-bitcoin/20/) 0x7fa202945a74 in QThread::exec() (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xbda74)
    [#21](/bitcoin-bitcoin/21/) 0x7fa202946cc1  (/lib/x86_64-linux-gnu/libQt5Core.so.5+0xbecc1)
    [#22](/bitcoin-bitcoin/22/) 0x7fa202e81668 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x9668)
    [#23](/bitcoin-bitcoin/23/) 0x7fa200f92322 in clone /build/glibc-4WA41p/glibc-2.30/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change chain.h:430:16 in 
marco@marco-Standard-PC-Q35-ICH9-2009:~/bitcoin$ clang-9 --version
clang version 9.0.0-2 (tags/RELEASE_900/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

MarcoFalke commented at 7:04 PM on November 21, 2019: member

I think the best way to work around the bug is to apply sanitization opt-outs for the triggering functions using attribute((no_sanitize("integer"))).

I'd rather not add clumsy annotations in the code to work around compiler bugs that are present in multiple versions. If this sanitizer is broken, it should be disabled. It serves no use to enable it in configure and then disable it blindly with code annotations.

MarcoFalke force-pushed on Nov 21, 2019

practicalswift commented at 9:14 PM on November 21, 2019: contributor

@practicalswift Any idea in what clang version this was fixed. I tried clang-9 and it still happens:

chain.h:430:16: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'int' changed the value to -1 (32-bit, signed)
…
SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change >

I think you have the answer right there thanks to report_error_type=1: the suppression for chain.h should be implicit-signed-integer-truncation-or-sign-change instead of implicit-integer-sign-change.

Does that work? :)

MarcoFalke commented at 9:17 PM on November 21, 2019: member

No, clang can not parse that last time I checked.

practicalswift commented at 9:27 PM on November 21, 2019: contributor

@MarcoFalke Did you test for clang-9? IIRC clang-8 had some suppression naming issues.

MarcoFalke commented at 1:59 AM on November 22, 2019: member

Yes, see the commits. I am using clang-9.

https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/615347666#L3807

MarcoFalke commented at 2:10 AM on November 22, 2019: member

I don't think there is anything we can do. I will remove the integer sanitizer tomorrow.

DrahtBot added the label Needs rebase on Dec 3, 2019

DrahtBot commented at 4:44 PM on December 3, 2019: member

Needs rebase

practicalswift commented at 10:10 PM on December 3, 2019: contributor

@MarcoFalke Do you mean remove the integer sanitizer for the Travis pcc64le build? Removing it across the board seems like a very risky move. We badly need all the sanitizers :)