Termux-packages: ncmpcpp Segmentation fault

Created on 11 Feb 2019  路  13Comments  路  Source: termux/termux-packages

Problem description
Starting 

$ ncmpcpp
Segmentation fault

all pkgs upgraded.

Steps to reproduce
install ncmpcpp
start it

Additional information
Samsung galaxy s9+, android version 9

Depending on problem, additional information may be requested:

  1. Android warning/error log: logcat -d "*:W".
02-11 09:27:07.169 11893 11893 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 11893 (ncmpcpp), pid 11893 (ncmpcpp)
02-11 09:27:07.191 11897 11897 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
02-11 09:27:07.191 11897 11897 F DEBUG   : Build fingerprint: 'samsung/star2ltexx/star2lte:9/PPR1.180610.011/G965FXXU2CRLI:user/release-keys'
02-11 09:27:07.191 11897 11897 F DEBUG   : Revision: '26'
02-11 09:27:07.191 11897 11897 F DEBUG   : ABI: 'arm64'
02-11 09:27:07.191 11897 11897 F DEBUG   : pid: 11893, tid: 11893, name: ncmpcpp  >>> ncmpcpp <<<
02-11 09:27:07.192 11897 11897 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
02-11 09:27:07.192 11897 11897 F DEBUG   : Cause: null pointer dereference
02-11 09:27:07.192 11897 11897 F DEBUG   :     x0  000000716d66e000  x1  0000000000000000  x2  0000000000000000  x3  0000007fe2e1a6e8
02-11 09:27:07.192 11897 11897 F DEBUG   :     x4  0000007fe2e1a6f0  x5  0000007fe2e1a83d  x6  0000000065757274  x7  0000000065757274
02-11 09:27:07.192 11897 11897 F DEBUG   :     x8  0000000000000000  x9  0000000000000008  x10 000000000000003d  x11 0101010101010101
02-11 09:27:07.192 11897 11897 F DEBUG   :     x12 0000000000000031  x13 ffffffffffffffff  x14 000000716d600000  x15 ffffffffffffffff
02-11 09:27:07.192 11897 11897 F DEBUG   :     x16 000000716e068080  x17 000000716e52bc88  x18 0000007fe2e1979a  x19 000000716d66e000
02-11 09:27:07.192 11897 11897 F DEBUG   :     x20 0000007fe2e1a834  x21 000000716d6332a8  x22 0000007fe2e1a834  x23 0000000000000001
02-11 09:27:07.192 11897 11897 F DEBUG   :     x24 0000000000000040  x25 0000007fe2e1a700  x26 0000000000000002  x27 0000000000000000
02-11 09:27:07.192 11897 11897 F DEBUG   :     x28 0000000000000000  x29 0000007fe2e1a6d0
02-11 09:27:07.192 11897 11897 F DEBUG   :     sp  0000007fe2e1a6c0  lr  000000716e0027e0  pc  000000716e52bcb4
02-11 09:27:07.193 11897 11897 F DEBUG   :
02-11 09:27:07.193 11897 11897 F DEBUG   : backtrace:
02-11 09:27:07.193 11897 11897 F DEBUG   :     #00 pc 0000000000028cb4  /system/lib64/libc.so (iconv+44)
02-11 09:27:07.193 11897 11897 F DEBUG   :     #01 pc 000000000001a7dc  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
02-11 09:27:07.193 11897 11897 F DEBUG   :     #02 pc 0000000000019434  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
02-11 09:27:07.193 11897 11897 F DEBUG   :     #03 pc 0000000000034574  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
02-11 09:27:07.193 11897 11897 F DEBUG   :     #04 pc 0000000000033998  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
02-11 09:27:07.193 11897 11897 F DEBUG   :     #05 pc 000000000001ff50  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::locale const&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+316)
02-11 09:27:07.193 11897 11897 F DEBUG   :     #06 pc 000000000001fde4  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+60)
02-11 09:27:07.193 11897 11897 F DEBUG   :     #07 pc 00000000000efd2c  /data/data/com.termux/files/usr/bin/ncmpcpp
02-11 09:27:07.193 11897 11897 F DEBUG   :     #08 pc 000000000010e990  /data/data/com.termux/files/usr/bin/ncmpcpp
02-11 09:27:07.193 11897 11897 F DEBUG   :     #09 pc 00000000000b0548  /system/lib64/libc.so (__libc_init+88)
android-9 arch-aarch64 bug report
Source
picture ibeex

Most helpful comment

@ibeex The fix has only been pushed to the android >= 7 branch, you are subscribed to the android 5 & 6 repos. The android-7 repos are default for all fresh installations on android >= 7 devices since the last two weeks or so.

To upgrade to this repo you can run rm -rf $PREFIX and restart termux to get a fresh installation. This will leave $HOME intact but delete all installed packages in $PREFIX/ (you need to re-download all packages one way or another to use packages from the new repos)

picture Grimler91 on 29 Apr 2019
👍2

All 13 comments

I built debug versions of ncmpcpp, boost and openssl and looked at it with gdb. At first I got:

(gdb) r
Starting program: /data/data/com.termux/files/usr/bin/ncmpcpp 
warning: .dynamic section for "/data/data/com.termux/files/usr/bin/ncmpcpp" is not at the expected address (wrong library or version mismatch?)

Program received signal SIGILL, Illegal instruction.
0x0000007fb738ed10 in _armv8_sha512_probe () at crypto/arm64cpuid.S:50
50  crypto/arm64cpuid.S: No such file or directory.
(gdb) bt
#0  0x0000007fb738ed10 in _armv8_sha512_probe () at crypto/arm64cpuid.S:50
#1  0x0000007fb738efac in OPENSSL_cpuid_setup () at crypto/armcap.c:198
#2  0x0000007fb7ed16ac in __dl__ZL10call_arrayIPFviPPcS1_EEvPKcPT_mbS5_ () from /system/bin/linker64
#3  0x0000007fb7ed18c8 in __dl__ZN6soinfo17call_constructorsEv () from /system/bin/linker64
#4  0x0000007fb7ed17d0 in __dl__ZN6soinfo17call_constructorsEv () from /system/bin/linker64
#5  0x0000007fb7ed17d0 in __dl__ZN6soinfo17call_constructorsEv () from /system/bin/linker64
#6  0x0000007fb7ed17d0 in __dl__ZN6soinfo17call_constructorsEv () from /system/bin/linker64
#7  0x0000007fb7ecd8fc in __dl___linker_init () from /system/bin/linker64
#8  0x0000007fb7ed3a24 in __dl__start () from /system/bin/linker64

But I'm not sure if this an actual problem or a variant of https://github.com/termux/termux-packages/issues/1660.

Anyways, ignoring that error (disabling the check for hardware sha512 capabilities) gives:

(gdb) r
Starting program: /data/data/com.termux/files/usr/bin/ncmpcpp 
warning: .dynamic section for "/data/data/com.termux/files/usr/bin/ncmpcpp" is not at the expected address (wrong library or version mismatch?)

Program received signal SIGSEGV, Segmentation fault.
0x0000007fb78a8c3c in iconv () from /system/lib64/libc.so
(gdb) bt
#0  0x0000007fb78a8c3c in iconv () from /system/lib64/libc.so
#1  0x0000007fb7c83320 in boost::locale::conv::impl::iconverter_base::real_convert<char, wchar_t> (this=<optimized out>, ubegin=<optimized out>, uend=<optimized out>) at libs/locale/src/encoding/iconv_codepage.ipp:79
#2  0x0000007fb7c81a58 in boost::locale::conv::impl::convert_from<wchar_t> (begin=0x7ffffff624 L"true", end=0x7ffffff634 L"", charset=0x7ffffff609 "UTF-8", how=boost::locale::conv::skip) at libs/locale/src/encoding/codepage.cpp:99
#3  0x0000007fb7ca1394 in boost::locale::impl_std::utf8_numpunct_from_wide::utf8_numpunct_from_wide (this=0x7fb6a27080, base=..., refs=<optimized out>) at libs/locale/src/std/numeric.cpp:75
#4  0x0000007fb7ca08dc in boost::locale::impl_std::create_formatting (in=..., locale_name=..., type=<optimized out>, utf=<optimized out>) at libs/locale/src/std/numeric.cpp:342
#5  0x0000007fb7c89740 in boost::locale::generator::generate (this=0x7ffffff858, base=..., id=...) at libs/locale/src/shared/generator.cpp:142
#6  0x0000007fb7c895d4 in boost::locale::generator::generate (this=0x7fb6a6e000, id=...) at libs/locale/src/shared/generator.cpp:117
#7  0x000000555566a338 in Charset::internalLocale () at charset.cpp:30
#8  0x00000055556911f4 in main (argc=1, argv=0x7ffffffa68) at ncmpcpp.cpp:95

I guess we should patch ncmpcpp to just use UTF-8 and not check for other locales. I will experiment a bit with it later today and hopefully come up with a fix

Grimler91 picture Grimler91 on 1 Apr 2019

Applying this patch: 

--- ../charset.cpp.orig 2019-04-01 20:02:25.440202857 +0000
+++ ./src/charset.cpp   2019-04-01 20:31:12.456943884 +0000
@@ -27,20 +27,7 @@
 std::locale internalLocale()
 {
    boost::locale::generator gen;
-   std::locale loc = gen("");
-   bool is_utf = std::use_facet<boost::locale::info>(loc).utf8();
-   std::string name = std::use_facet<boost::locale::info>(loc).name();
-   if (!is_utf && name != "C" && name != "POSIX")
-   {
-       // if current locale does not use unicode, use variant of this
-       // locale with utf8 as ncmpcpp uses utf8 internally and we need
-       // current locale for sorting, case conversions etc.
-       std::string new_name = std::use_facet<boost::locale::info>(loc).language()
-                            + "_"
-                            + std::use_facet<boost::locale::info>(loc).country()
-                            + ".UTF-8";
-       loc = gen(new_name);
-   }
+   std::locale loc = gen("en_US.UTF-8");
    return loc;
 }

Unfortunately didn't change anything so the problem seem to be in boost.

Grimler91 picture Grimler91 on 1 Apr 2019

Attempted on-device build, no avail. Apparently a null pointer is dereferenced. It is indeed a boost problem.

04-11 18:12:21.668 29911 29911 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 29911 (ncmpcpp), pid 29911 (ncmpcpp)
04-11 18:12:21.684 29914 29914 I crash_dump64: obtaining output fd from tombstoned, type: kDebuggerdTombstone
04-11 18:12:21.686 29914 29914 I crash_dump64: performing dump of process 29911 (target tid = 29911)
04-11 18:12:21.687 29914 29914 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
04-11 18:12:21.687 29914 29914 F DEBUG   : Build fingerprint: 'samsung/jackpotltexx/jackpotlte:9/PPR1.180610.011/A530FXXU4CSC6:user/release-keys'
04-11 18:12:21.687 29914 29914 F DEBUG   : Revision: '7'
04-11 18:12:21.687 29914 29914 F DEBUG   : ABI: 'arm64'
04-11 18:12:21.687 29914 29914 F DEBUG   : pid: 29911, tid: 29911, name: ncmpcpp  >>> ./ncmpcpp <<<
04-11 18:12:21.687 29914 29914 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
04-11 18:12:21.687 29914 29914 F DEBUG   : Cause: null pointer dereference
04-11 18:12:21.687 29914 29914 F DEBUG   :     x0  0000007e4726e000  x1  0000000000000000  x2  0000000000000000  x3  0000007fcb1a69d8
04-11 18:12:21.687 29914 29914 F DEBUG   :     x4  0000007fcb1a69e0  x5  0000007fcb1a6b2d  x6  0000000065757274  x7  0000000065757274
04-11 18:12:21.687 29914 29914 F DEBUG   :     x8  0000000000000000  x9  0000000000000008  x10 000000000000003d  x11 0101010101010101
04-11 18:12:21.687 29914 29914 F DEBUG   :     x12 0000000000000000  x13 0000000000001840  x14 0000000000000070  x15 aaaaaaaaaaaaaaab
04-11 18:12:21.687 29914 29914 F DEBUG   :     x16 0000007e480f1088  x17 0000007e47b69bb4  x18 0000000000000001  x19 0000007e4726e000
04-11 18:12:21.687 29914 29914 F DEBUG   :     x20 0000007fcb1a6b24  x21 0000007e472332a8  x22 0000007fcb1a6b24  x23 0000000000000001
04-11 18:12:21.687 29914 29914 F DEBUG   :     x24 0000000000000040  x25 0000007fcb1a69f0  x26 0000000000000002  x27 0000000000000000
04-11 18:12:21.687 29914 29914 F DEBUG   :     x28 0000000000000000  x29 0000007fcb1a69c0
04-11 18:12:21.687 29914 29914 F DEBUG   :     sp  0000007fcb1a69b0  lr  0000007e4809bd28  pc  0000007e47b69be0
04-11 18:12:21.689 29914 29914 F DEBUG   :
04-11 18:12:21.689 29914 29914 F DEBUG   : backtrace:
04-11 18:12:21.689 29914 29914 F DEBUG   :     #00 pc 0000000000028be0  /system/lib64/libc.so (iconv+44)
04-11 18:12:21.689 29914 29914 F DEBUG   :     #01 pc 000000000001ad24  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-11 18:12:21.689 29914 29914 F DEBUG   :     #02 pc 00000000000199b8  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-11 18:12:21.689 29914 29914 F DEBUG   :     #03 pc 0000000000032828  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-11 18:12:21.689 29914 29914 F DEBUG   :     #04 pc 0000000000031d40  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-11 18:12:21.689 29914 29914 F DEBUG   :     #05 pc 000000000001fbbc  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::locale const&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+292)
04-11 18:12:21.689 29914 29914 F DEBUG   :     #06 pc 000000000001fa6c  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+60)
04-11 18:12:21.689 29914 29914 F DEBUG   :     #07 pc 0000000000130328  /data/data/com.termux/files/home/ncmpcpp/src/ncmpcpp (Charset::internalLocale()+48)
04-11 18:12:21.689 29914 29914 F DEBUG   :     #08 pc 000000000015ad24  /data/data/com.termux/files/home/ncmpcpp/src/ncmpcpp (main+60)
04-11 18:12:21.690 29914 29914 F DEBUG   :     #09 pc 00000000000ae878  /system/lib64/libc.so (__libc_init+88)

Somewhere here: https://github.com/boostorg/locale/blob/bc9ab436c525a7409f64926b8407e6215660debd/src/shared/generator.cpp#L120

Did some basic fixes, now ncmpcpp works again! https://github.com/bakaika/ncmpcpp

ghost picture ghost on 11 Apr 2019

Seems like boost uses iconv from libc instead of libandroid-support:

04-11 18:12:21.689 29914 29914 F DEBUG   :     #00 pc 0000000000028be0  /system/lib64/libc.so (iconv+44)
xeffyr picture xeffyr on 11 Apr 2019

Hm, yes, starting ncmpcpp with LD_PRELOAD=$PREFIX/lib/libandroid-support.so ncmpcpp works

Grimler91 picture Grimler91 on 11 Apr 2019

Then I guess linking boost/ncmpcpp with libandroid-support should completely solve problem.

xeffyr picture xeffyr on 11 Apr 2019

Yeah, libboost_locale is linked against libiconv/libandroid-support but ncmpcpp isn't, so I guess this is a variant of https://github.com/android-ndk/ndk/issues/201.

3667 fixes it

Grimler91 picture Grimler91 on 13 Apr 2019

Hi, after installing latest packages still thee is Segmentation fault when running ncmpcpp

here is logtrace, it looks like diferent error?

04-29 11:51:47.916 11198 11198 W IInputConnectionWrapper: getTextBeforeCursor on inactive InputConnection
04-29 11:51:47.918 11198 11198 W IInputConnectionWrapper: getSelectedText on inactive InputConnection
04-29 11:51:47.919 11198 11198 W IInputConnectionWrapper: getTextAfterCursor on inactive InputConnection
04-29 11:52:17.985 11198 11224 W libEGL  : EGLNativeWindowType 0x72f5d19010 disconnect failed
04-29 11:52:54.861 12062 12062 F libc    : Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 12062 (ncmpcpp), pid 12062 (ncmpcpp)
04-29 11:52:54.879 12065 12065 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
04-29 11:52:54.879 12065 12065 F DEBUG   : Build fingerprint: 'samsung/star2ltexx/star2lte:9/PPR1.180610.011/G965FXXU2CRLI:user/release-keys'
04-29 11:52:54.879 12065 12065 F DEBUG   : Revision: '26'
04-29 11:52:54.879 12065 12065 F DEBUG   : ABI: 'arm64'
04-29 11:52:54.879 12065 12065 F DEBUG   : pid: 12062, tid: 12062, name: ncmpcpp  >>> ncmpcpp <<<
04-29 11:52:54.879 12065 12065 F DEBUG   : signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
04-29 11:52:54.879 12065 12065 F DEBUG   : Cause: null pointer dereference
04-29 11:52:54.879 12065 12065 F DEBUG   :     x0  0000007cd3a6e000  x1  0000000000000000  x2  0000000000000000  x3  0000007fe7e41898
04-29 11:52:54.879 12065 12065 F DEBUG   :     x4  0000007fe7e418a0  x5  0000007fe7e419ed  x6  0000000065757274  x7  0000000065757274
04-29 11:52:54.879 12065 12065 F DEBUG   :     x8  0000000000000000  x9  0000000000000008  x10 000000000000003d  x11 0101010101010101
04-29 11:52:54.879 12065 12065 F DEBUG   :     x12 0000000000000031  x13 ffffffffffffffff  x14 0000007cd3a00000  x15 ffffffffffffffff
04-29 11:52:54.879 12065 12065 F DEBUG   :     x16 0000007cd4677080  x17 0000007cd4aacc88  x18 0000007fe7e408ea  x19 0000007cd3a6e000
04-29 11:52:54.879 12065 12065 F DEBUG   :     x20 0000007fe7e419e4  x21 0000007cd3a332a8  x22 0000007fe7e419e4  x23 0000000000000001
04-29 11:52:54.879 12065 12065 F DEBUG   :     x24 0000000000000040  x25 0000007fe7e418b0  x26 0000000000000002  x27 0000000000000000
04-29 11:52:54.879 12065 12065 F DEBUG   :     x28 0000000000000000  x29 0000007fe7e41880
04-29 11:52:54.879 12065 12065 F DEBUG   :     sp  0000007fe7e41870  lr  0000007cd462178c  pc  0000007cd4aaccb4
04-29 11:52:54.881 12065 12065 F DEBUG   :
04-29 11:52:54.881 12065 12065 F DEBUG   : backtrace:
04-29 11:52:54.881 12065 12065 F DEBUG   :     #00 pc 0000000000028cb4  /system/lib64/libc.so (iconv+44)
04-29 11:52:54.881 12065 12065 F DEBUG   :     #01 pc 000000000001a788  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-29 11:52:54.881 12065 12065 F DEBUG   :     #02 pc 000000000001941c  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-29 11:52:54.881 12065 12065 F DEBUG   :     #03 pc 000000000003228c  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-29 11:52:54.881 12065 12065 F DEBUG   :     #04 pc 00000000000317a4  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0
04-29 11:52:54.881 12065 12065 F DEBUG   :     #05 pc 000000000001f620  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::locale const&, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+292)
04-29 11:52:54.881 12065 12065 F DEBUG   :     #06 pc 000000000001f4d0  /data/data/com.termux/files/usr/lib/libboost_locale.so.1.69.0 (boost::locale::generator::generate(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char>> const&) const+60)
04-29 11:52:54.881 12065 12065 F DEBUG   :     #07 pc 00000000000ea7bc  /data/data/com.termux/files/usr/bin/ncmpcpp
04-29 11:52:54.881 12065 12065 F DEBUG   :     #08 pc 0000000000107b94  /data/data/com.termux/files/usr/bin/ncmpcpp
04-29 11:52:54.881 12065 12065 F DEBUG   :     #09 pc 00000000000b0548  /system/lib64/libc.so (__libc_init+88)
ibeex picture ibeex on 29 Apr 2019

I'm not exactly sure where to begin. It's beyond my understanding. Ive been trying to understand and figure out where to begin but I've just been so confused.

jaredtl4587 picture jaredtl4587 on 29 Apr 2019

@ibeex what's your output of termux-info?

Grimler91 picture Grimler91 on 29 Apr 2019 
$ termux-info
Updatable packages:
All packages up to date
Subscribed repositories:
https://dl.bintray.com/grimler/science-packages-21 science/stable
https://dl.bintray.com/grimler/game-packages-21 games/stable
https://termux.net stable/main
https://termux.net stable/main
System information:
Linux localhost 4.9.59-15044066 #1 SMP PREEMPT Thu Dec 20 01:51:28 KST 2018 aarch64 Android
Termux-packages arch:
aarch64
Android version:
9
Device manufacturer:
samsung
Device model:
SM-G965F
ibeex picture ibeex on 29 Apr 2019

@ibeex The fix has only been pushed to the android >= 7 branch, you are subscribed to the android 5 & 6 repos. The android-7 repos are default for all fresh installations on android >= 7 devices since the last two weeks or so.

To upgrade to this repo you can run rm -rf $PREFIX and restart termux to get a fresh installation. This will leave $HOME intact but delete all installed packages in $PREFIX/ (you need to re-download all packages one way or another to use packages from the new repos)

Grimler91 picture Grimler91 on 29 Apr 2019
👍2

@Grimler91 thx, this is it, now it works

ibeex picture ibeex on 29 Apr 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

Zuccace picture Zuccace  路  3Comments
Wetitpig picture Wetitpig  路  3Comments
loveablefellow007 picture loveablefellow007  路  3Comments
ILadis picture ILadis  路  3Comments
StephanBeer picture StephanBeer  路  3Comments