From 4e020d4aa2d684e99e5355ee34896fd0aeb0f281 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Wed, 9 Mar 2016 20:28:48 +0000 Subject: [PATCH 1/3] Increase verbosity of tile_map.h assertions. --- src/tile_map.h | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/tile_map.h b/src/tile_map.h index 4d5891d7ba..3748107698 100644 --- a/src/tile_map.h +++ b/src/tile_map.h @@ -30,7 +30,7 @@ */ static inline uint TileHeight(TileIndex tile) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X", tile, MapSize()); return _m[tile].height; } @@ -48,7 +48,7 @@ uint TileHeightOutsideMap(int x, int y); */ static inline void SetTileHeight(TileIndex tile, uint height) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X", tile, MapSize()); assert(height <= MAX_TILE_HEIGHT); _m[tile].height = height; } @@ -88,7 +88,7 @@ static inline uint TilePixelHeightOutsideMap(int x, int y) */ static inline TileType GetTileType(TileIndex tile) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X", tile, MapSize()); return (TileType)GB(_m[tile].type, 4, 4); } @@ -101,7 +101,7 @@ static inline TileType GetTileType(TileIndex tile) */ static inline bool IsInnerTile(TileIndex tile) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X", tile, MapSize()); uint x = TileX(tile); uint y = TileY(tile); @@ -123,11 +123,11 @@ static inline bool IsInnerTile(TileIndex tile) */ static inline void SetTileType(TileIndex tile, TileType type) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X, type: %d", tile, MapSize(), type); /* VOID tiles (and no others) are exactly allowed at the lower left and right * edges of the map. If _settings_game.construction.freeform_edges is true, * the upper edges of the map are also VOID tiles. */ - assert(IsInnerTile(tile) == (type != MP_VOID)); + assert_msg(IsInnerTile(tile) == (type != MP_VOID), "tile: 0x%X (%d), type: %d", tile, IsInnerTile(tile), type); SB(_m[tile].type, 4, 4, type); } @@ -170,9 +170,8 @@ static inline bool IsValidTile(TileIndex tile) */ static inline Owner GetTileOwner(TileIndex tile) { - assert(IsValidTile(tile)); - assert(!IsTileType(tile, MP_HOUSE)); - assert(!IsTileType(tile, MP_INDUSTRY)); + assert_msg(IsValidTile(tile), "tile: 0x%X, size: 0x%X", tile, MapSize()); + assert_msg(!IsTileType(tile, MP_HOUSE) && !IsTileType(tile, MP_INDUSTRY), "tile: 0x%X (%d)", tile, GetTileType(tile)); return (Owner)GB(_m[tile].m1, 0, 5); } @@ -190,9 +189,8 @@ static inline Owner GetTileOwner(TileIndex tile) */ static inline void SetTileOwner(TileIndex tile, Owner owner) { - assert(IsValidTile(tile)); - assert(!IsTileType(tile, MP_HOUSE)); - assert(!IsTileType(tile, MP_INDUSTRY)); + assert_msg(IsValidTile(tile), "tile: 0x%X, size: 0x%X, owner: %d", tile, MapSize(), owner); + assert_msg(!IsTileType(tile, MP_HOUSE) && !IsTileType(tile, MP_INDUSTRY), "tile: 0x%X (%d), owner: %d", tile, GetTileType(tile), owner); SB(_m[tile].m1, 0, 5, owner); } @@ -217,8 +215,8 @@ static inline bool IsTileOwner(TileIndex tile, Owner owner) */ static inline void SetTropicZone(TileIndex tile, TropicZone type) { - assert(tile < MapSize()); - assert(!IsTileType(tile, MP_VOID) || type == TROPICZONE_NORMAL); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X, type: %d", tile, MapSize(), type); + assert_msg(!IsTileType(tile, MP_VOID) || type == TROPICZONE_NORMAL, "tile: 0x%X (%d), type: %d", tile, GetTileType(tile), type); SB(_m[tile].type, 0, 2, type); } @@ -230,19 +228,19 @@ static inline void SetTropicZone(TileIndex tile, TropicZone type) */ static inline TropicZone GetTropicZone(TileIndex tile) { - assert(tile < MapSize()); + assert_msg(tile < MapSize(), "tile: 0x%X, size: 0x%X", tile, MapSize()); return (TropicZone)GB(_m[tile].type, 0, 2); } /** * Get the current animation frame * @param t the tile - * @pre IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) ||IsTileType(t, MP_STATION) + * @pre IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) || IsTileType(t, MP_STATION) * @return frame number */ static inline byte GetAnimationFrame(TileIndex t) { - assert(IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) ||IsTileType(t, MP_STATION)); + assert_msg(IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) || IsTileType(t, MP_STATION), "tile: 0x%X (%d)", t, GetTileType(t)); return _me[t].m7; } @@ -250,11 +248,11 @@ static inline byte GetAnimationFrame(TileIndex t) * Set a new animation frame * @param t the tile * @param frame the new frame number - * @pre IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) ||IsTileType(t, MP_STATION) + * @pre IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) || IsTileType(t, MP_STATION) */ static inline void SetAnimationFrame(TileIndex t, byte frame) { - assert(IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) ||IsTileType(t, MP_STATION)); + assert_msg(IsTileType(t, MP_HOUSE) || IsTileType(t, MP_OBJECT) || IsTileType(t, MP_INDUSTRY) || IsTileType(t, MP_STATION), "tile: 0x%X (%d)", t, GetTileType(t)); _me[t].m7 = frame; } From f82002cda2e3cb1ef1264f0c2e091bd88b449f48 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Thu, 10 Mar 2016 00:13:58 +0000 Subject: [PATCH 2/3] Use likely/__builtin_expect for assertion macros. --- src/stdafx.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/stdafx.h b/src/stdafx.h index cb99d244a8..1a5e19b7d1 100644 --- a/src/stdafx.h +++ b/src/stdafx.h @@ -444,6 +444,14 @@ assert_compile(SIZE_MAX >= UINT32_MAX); #define CloseConnection OTTD_CloseConnection #endif /* __APPLE__ */ +#ifdef __GNUC__ +#define likely(x) __builtin_expect(!!(x), 1) +#define unlikely(x) __builtin_expect(!!(x), 0) +#else +#define likely(x) (x) +#define unlikely(x) (x) +#endif + void NORETURN CDECL usererror(const char *str, ...) WARN_FORMAT(1, 2); void NORETURN CDECL error(const char *str, ...) WARN_FORMAT(1, 2); void NORETURN CDECL assert_msg_error(int line, const char *file, const char *expr, const char *str, ...) WARN_FORMAT(4, 5); @@ -455,13 +463,13 @@ void NORETURN CDECL assert_msg_error(int line, const char *file, const char *exp */ #if (defined(_MSC_VER) && defined(NDEBUG) && defined(WITH_ASSERT)) || (!defined(_MSC_VER) && !defined(NDEBUG) && !defined(_DEBUG)) #undef assert - #define assert(expression) if (!(expression)) error("Assertion failed at line %i of %s: %s", __LINE__, __FILE__, #expression); + #define assert(expression) if (unlikely(!(expression))) error("Assertion failed at line %i of %s: %s", __LINE__, __FILE__, #expression); #endif /* Asserts are enabled if NDEBUG isn't defined, or if we are using MSVC and WITH_ASSERT is defined. */ #if !defined(NDEBUG) || (defined(_MSC_VER) && defined(WITH_ASSERT)) #define OTTD_ASSERT - #define assert_msg(expression, ...) if (!(expression)) assert_msg_error(__LINE__, __FILE__, #expression, __VA_ARGS__); + #define assert_msg(expression, ...) if (unlikely(!(expression))) assert_msg_error(__LINE__, __FILE__, #expression, __VA_ARGS__); #else #define assert_msg(expression, ...) #endif From a609439d983bac111b1e0a0f5852702f5504ed52 Mon Sep 17 00:00:00 2001 From: Jonathan G Rennison Date: Thu, 10 Mar 2016 00:08:34 +0000 Subject: [PATCH 3/3] Add support for enhanced crash log stacktraces using gdb on Linux. This attempts to use gdb to attach to the current process and print a full backtrace. --- config.lib | 65 ++++++++++++++++++++++++-- src/os/unix/crashlog_unix.cpp | 88 +++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 5 deletions(-) diff --git a/config.lib b/config.lib index 379b2eaf1c..4ff37a8e20 100644 --- a/config.lib +++ b/config.lib @@ -97,6 +97,7 @@ set_default() { with_sse="1" with_libbfd="1" with_bfd_extra_debug="1" + with_dbg_gdb="1" save_params_array=" build @@ -176,6 +177,7 @@ set_default() { with_sse with_libbfd with_bfd_extra_debug + with_dbg_gdb CC CXX CFLAGS CXXFLAGS LDFLAGS CFLAGS_BUILD CXXFLAGS_BUILD LDFLAGS_BUILD" } @@ -477,6 +479,10 @@ detect_params() { --with-bfd-extra-debug) with_bfd_extra_debug="1";; --with-bfd-extra-debug=*) with_bfd_extra_debug="$optarg";; + --without-self-gdb-debug) with_dbg_gdb="0";; + --with-self-gdb-debug) with_dbg_gdb="1";; + --with-self-gdb-debug=*) with_dbg_gdb="$optarg";; + CC=* | --CC=*) CC="$optarg";; CXX=* | --CXX=*) CXX="$optarg";; CFLAGS=* | --CFLAGS=*) CFLAGS="$optarg";; @@ -1614,6 +1620,7 @@ EOL CFLAGS="$CFLAGS -DWITH_DL" fi + LIBBFD_LIBS= if [ "$with_libbfd" = "1" ]; then if test_compile_libbfd "-lbfd -lz"; then LIBBFD_LIBS="-lbfd -lz" @@ -1621,20 +1628,67 @@ EOL LIBBFD_LIBS="-lbfd -liberty -lz" elif test_compile_libbfd "-lbfd -liberty -lintl -lz"; then LIBBFD_LIBS="-lbfd -liberty -lintl -lz" - else - LIBBFD_LIBS= fi if [ -n "$LIBBFD_LIBS" ]; then log 1 "checking libbfd... found" LIBS="$LIBS $LIBBFD_LIBS" CFLAGS="$CFLAGS -DWITH_BFD" - if [ $enable_debug -lt 1 ] && [ "$with_bfd_extra_debug" = "1" ]; then - CFLAGS="$CFLAGS -g1" - fi else log 1 "checking libbfd... no" fi fi + + HAVE_GDB_DBG= + if [ "$with_dbg_gdb" = "1" ]; then + log 2 "executing $cc_host $CFLAGS $LDFLAGS $STATIC_FLAGS -o tmp.config.dbggdb -x c++ -" + "$cc_host" $CFLAGS $LDFLAGS $STATIC_FLAGS -o tmp.config.dbggdb -x c++ - 2> /dev/null << EOL + #include + #include + #include + #include + #include + #include + int main() { + pid_t tid = syscall(SYS_gettid); + int status; + waitpid((pid_t) 0, &status, 0); + return WIFEXITED(status) && WEXITSTATUS(status); + } +EOL + ret=$? + rm -f tmp.config.dbggdb + log 2 " exit code $ret" + if [ $ret -ne 0 ]; then + log 1 "checking dbg gdb... no" + else + log 1 "checking dbg gdb... found" + CFLAGS="$CFLAGS -DWITH_DBG_GDB" + HAVE_GDB_DBG=1 + + log 2 "executing $cc_host $CFLAGS $LDFLAGS $STATIC_FLAGS -o tmp.config.dbggdbprctl -x c++ -" + "$cc_host" $CFLAGS $LDFLAGS $STATIC_FLAGS -o tmp.config.dbggdbprctl -x c++ - 2> /dev/null << EOL + #include + int main() { + return prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0); + } +EOL + ret=$? + rm -f tmp.config.dbggdbprctl + log 2 " exit code $ret" + if [ $ret -ne 0 ]; then + log 1 "checking dbg gdb (prctl)... no" + else + log 1 "checking dbg gdb (prctl)... found" + CFLAGS="$CFLAGS -DWITH_PRCTL_PT" + fi + fi + fi + + if [ -n "$LIBBFD_LIBS" -o -n "$HAVE_GDB_DBG" ]; then + if [ $enable_debug -lt 1 ] && [ "$with_bfd_extra_debug" = "1" ]; then + CFLAGS="$CFLAGS -g1" + fi + fi fi if [ "$os" = "MINGW" ]; then @@ -3694,6 +3748,7 @@ showhelp() { echo " --without-sse disable SSE support (x86/x86_64 only)" echo " --without-libbfd disable libbfd support, used for improved crash logs (MinGW and Unix/glibc only)" echo " --without-bfd-extra-debug disable extra debugging information when using libbfd (MinGW and Unix/glibc only)" + echo " --without-self-gdb-debug disable improved crash logs using gdb (Linux only)" echo "" echo "Some influential environment variables:" echo " CC C compiler command" diff --git a/src/os/unix/crashlog_unix.cpp b/src/os/unix/crashlog_unix.cpp index 60c8208785..175eb7c534 100644 --- a/src/os/unix/crashlog_unix.cpp +++ b/src/os/unix/crashlog_unix.cpp @@ -21,6 +21,19 @@ #include #include +#if defined(WITH_DBG_GDB) +#include +#include +#include +#include +#include +#include +#endif /* WITH_DBG_GDB */ + +#if defined(WITH_PRCTL_PT) +#include +#endif /* WITH_PRCTL_PT */ + #if defined(__GLIBC__) /* Execinfo (and thus making stacktraces) is a GNU extension */ # include @@ -131,6 +144,79 @@ class CrashLogUnix : public CrashLog { } #endif + /** + * Get a stack backtrace of the current thread's stack using the gdb debugger, if available. + * + * Using GDB is useful as it knows about inlined functions and locals, and generally can + * do a more thorough job than in LogStacktrace. + * This is done in addition to LogStacktrace as gdb cannot be assumed to be present + * and there is some potentially useful information in the output from LogStacktrace + * which is not in gdb's output. + */ + char *LogStacktraceGdb(char *buffer, const char *last) const + { +#if defined(WITH_DBG_GDB) + +#if defined(WITH_PRCTL_PT) + prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0); +#endif /* WITH_PRCTL_PT */ + + pid_t tid = syscall(SYS_gettid); + + int pipefd[2]; + if (pipe(pipefd) == -1) return buffer; + + int pid = fork(); + if (pid < 0) return buffer; + + if (pid == 0) { + /* child */ + + close(pipefd[0]); /* Close unused read end */ + dup2(pipefd[1], STDOUT_FILENO); + close(pipefd[1]); + int null_fd = open("/dev/null", O_RDWR); + if (null_fd != -1) { + dup2(null_fd, STDERR_FILENO); + dup2(null_fd, STDIN_FILENO); + } + char buffer[16]; + seprintf(buffer, lastof(buffer), "%d", tid); + execlp("gdb", "gdb", "-n", "-p", buffer, "-batch", "-ex", "bt full", NULL); + exit(42); + } + + /* parent */ + + close(pipefd[1]); /* Close unused write end */ + + char *buffer_orig = buffer; + + buffer += seprintf(buffer, last, "Stacktrace (GDB):\n"); + while (buffer < last) { + ssize_t res = read(pipefd[0], buffer, last - buffer); + if (res < 0) { + if (errno == EINTR) continue; + break; + } else if (res == 0) { + break; + } else { + buffer += res; + } + } + buffer += seprintf(buffer, last, "\n"); + + int status; + int wait_ret = waitpid(pid, &status, 0); + if (wait_ret == -1 || !WIFEXITED(status) || WEXITSTATUS(status) != 0) { + /* gdb did not appear to run successfully */ + buffer = buffer_orig; + } +#endif /* WITH_DBG_GDB */ + + return buffer; + } + /** * Get a stack backtrace of the current thread's stack. * @@ -156,6 +242,8 @@ class CrashLogUnix : public CrashLog { */ /* virtual */ char *LogStacktrace(char *buffer, const char *last) const { + buffer = LogStacktraceGdb(buffer, last); + buffer += seprintf(buffer, last, "Stacktrace:\n"); #if defined(__GLIBC__)