Skip to content

Commit

Permalink
Fix SIGBUS from alignment issues on 64bit ARM
Browse files Browse the repository at this point in the history
ARMv8 (and in general aarch64) has flipped some strictness requirements.
However, at some point in history the NEED_ALIGN configure check code was
optimized away by GCC.

This fixes detection of alignment, as well as fixes an unaligned access that
snuck in via the logging code.

Also fixes a 64bit GCC atomics test that possibly never worked before.
  • Loading branch information
dormando committed Mar 15, 2018
1 parent 8bdf0a3 commit 8333182
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 4 deletions.
6 changes: 5 additions & 1 deletion configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -490,13 +490,16 @@ AC_CHECK_FUNCS(clock_gettime)
AC_CHECK_FUNCS([accept4], [AC_DEFINE(HAVE_ACCEPT4, 1, [Define to 1 if support accept4])])
AC_CHECK_FUNCS([getopt_long], [AC_DEFINE(HAVE_GETOPT_LONG, 1, [Define to 1 if support getopt_long])])

dnl Need to disable opt for alignment check. GCC is too clever and turns this
dnl into wide stores and no cmp under O2.
AC_DEFUN([AC_C_ALIGNMENT],
[AC_CACHE_CHECK(for alignment, ac_cv_c_alignment,
[
AC_RUN_IFELSE(
[AC_LANG_PROGRAM([
#include <stdlib.h>
#include <inttypes.h>
#pragma GCC optimize ("O0")
], [
char *buf = malloc(32);
Expand Down Expand Up @@ -551,7 +554,8 @@ dnl Check for usage of 64bit atomics
dnl 32bit systems shouldn't have these.
have_gcc_64atomics=no
AC_MSG_CHECKING(for GCC 64bit atomics)
AC_TRY_LINK([],[
AC_TRY_LINK([#include <inttypes.h>
],[
uint64_t a;
uint64_t b;
b = __sync_add_and_fetch(&a, 1);
Expand Down
14 changes: 11 additions & 3 deletions logger.c
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ static int logger_thread_read(logger *l, struct logger_stats *ls) {
} else {
logger_thread_write_entry(e, ls, scratch, scratch_len);
}
pos += sizeof(logentry) + e->size;
pos += sizeof(logentry) + e->size + e->pad;
}
assert(pos <= size);

Expand Down Expand Up @@ -699,8 +699,9 @@ enum logger_ret_type logger_log(logger *l, const enum log_entry_type event, cons
l->dropped++;
return LOGGER_RET_NOSPACE;
}
e->gid = logger_get_gid();
e->event = d->subtype;
e->pad = 0;
e->gid = logger_get_gid();
/* TODO: Could pass this down as an argument now that we're using
* LOGGER_LOG() macro.
*/
Expand Down Expand Up @@ -754,8 +755,15 @@ enum logger_ret_type logger_log(logger *l, const enum log_entry_type event, cons
break;
}

#ifdef NEED_ALIGN
/* Need to ensure *next* request is aligned. */
if (sizeof(logentry) + e->size % 8 != 0) {
e->pad = 8 - (sizeof(logentry) + e->size % 8);
}
#endif

/* Push pointer forward by the actual amount required */
if (bipbuf_push(buf, (sizeof(logentry) + e->size)) == 0) {
if (bipbuf_push(buf, (sizeof(logentry) + e->size + e->pad)) == 0) {
fprintf(stderr, "LOGGER: Failed to bipbuf push a text entry\n");
pthread_mutex_unlock(&l->mutex);
return LOGGER_RET_ERR;
Expand Down
1 change: 1 addition & 0 deletions logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ struct logentry_item_store {

typedef struct _logentry {
enum log_entry_subtype event;
uint8_t pad;
uint16_t eflags;
uint64_t gid;
struct timeval tv; /* not monotonic! */
Expand Down

0 comments on commit 8333182

Please sign in to comment.