Laszlo Ersek <ler...@redhat.com> writes: > comments in-line > > On 01/25/13 16:43, Markus Armbruster wrote: >> We use atomic operations to keep track of dropped events. >> >> Inconveniently, GLib supports only int and void * atomics, but the >> counter dropped_events is uint64_t. > > That's too bad. Instead of (or in addition to) int, glib should target > the (biggest) natural word size, ie. the integer whose size equals that > of "void *". size_t or uintptr_t looks like a good choice. > >> Can't stop commit 62bab732: a >> quick (gint *)&dropped_events bludgeons the compiler into submission. >> >> That cast is okay only when int is exactly 64 bits wide, which it >> commonly isn't. >> >> If int is even wider, we clobber whatever follows dropped_events. Not >> worth worrying about, as none of the machines that interest us have >> such morbidly obese ints. >> >> That leaves the common case: int narrower than 64 bits. >> >> Harmless on little endian hosts: we just don't access the most >> significant bits of dropped_events. They remain zero. >> >> On big endian hosts, we use only the most significant bits of >> dropped_events as counter. The least significant bits remain zero. >> However, we write out the full value, which is the correct counter >> shifted left a bunch of places. >> >> Fix by changing the variables involved to int. >> >> There's another, equally suspicious-looking (gint *)&trace_idx >> argument to g_atomic_int_compare_and_exchange(), but that one casts >> unsigned *, so it's okay. But it's also superfluous, because GLib's >> atomic int operations work just fine for unsigned. Drop it. > > Agree with "OK", disagree with "superfluous".
http://developer.gnome.org/glib/stable/glib-Atomic-Operations.html The following is a collection of compiler macros to provide atomic access to integer and pointer-sized values. The macros that have 'int' in the name will operate on pointers to gint and guint. > In the intersection of signed int's and unsigned int's domains, the > object representation is indeed required to be the same. But that > doesn't make "signed int" a type compatible with "unsigned int". > > Since we have a prototype for g_atomic_int_compare_and_exchange(), "the > arguments are implicitly converted, as if by assignment"; however, for > the pointer-to-pointer assignment here, the argument-ptr and the > parameter-ptr would have to point to compatible types, which "signed > int" and "unsigned int" are not. > > gcc's "-pedantic" emits > > warning: pointer targets in passing argument N of 'F' differ in signedness So GLib reneges on the promise it made in its documentation. Meh. > Anyway I don't insist reinstating the cast, I'm just saying that ISO C > would require it. I'm leaving the decision to the maintainer. Stefan? >> Signed-off-by: Markus Armbruster <arm...@redhat.com> >> --- >> trace/simple.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/trace/simple.c b/trace/simple.c >> index ce17d64..ccbdb6a 100644 >> --- a/trace/simple.c >> +++ b/trace/simple.c >> @@ -53,7 +53,7 @@ enum { >> uint8_t trace_buf[TRACE_BUF_LEN]; >> static unsigned int trace_idx; >> static unsigned int writeout_idx; >> -static uint64_t dropped_events; >> +static int dropped_events; >> static FILE *trace_fp; >> static char *trace_file_name; >> >> @@ -63,7 +63,7 @@ typedef struct { >> uint64_t timestamp_ns; >> uint32_t length; /* in bytes */ >> uint32_t reserved; /* unused */ >> - uint8_t arguments[]; >> + uint64_t arguments[]; >> } TraceRecord; >> >> typedef struct { >> @@ -160,7 +160,7 @@ static gpointer writeout_thread(gpointer opaque) >> uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)]; >> } dropped; >> unsigned int idx = 0; >> - uint64_t dropped_count; >> + int dropped_count; >> size_t unused __attribute__ ((unused)); >> >> for (;;) { >> @@ -169,16 +169,16 @@ static gpointer writeout_thread(gpointer opaque) >> if (dropped_events) { >> dropped.rec.event = DROPPED_EVENT_ID, >> dropped.rec.timestamp_ns = get_clock(); >> - dropped.rec.length = sizeof(TraceRecord) + >> sizeof(dropped_events), >> + dropped.rec.length = sizeof(TraceRecord) + sizeof(uint64_t), >> dropped.rec.reserved = 0; >> while (1) { >> dropped_count = dropped_events; >> - if (g_atomic_int_compare_and_exchange((gint >> *)&dropped_events, >> + if (g_atomic_int_compare_and_exchange(&dropped_events, >> dropped_count, 0)) { >> break; >> } >> } >> - memcpy(dropped.rec.arguments, &dropped_count, sizeof(uint64_t)); >> + dropped.rec.arguments[0] = dropped_count; > > I *think* I'd prefer if the element type of TraceRecord.arguments[] were > not changed; an array of u8's seems to be more flexible. The element > type change appears both unrelated and not really necessary for this > patch. (You'd have to keep the memcpy(), but it doesn't hurt.) Casting uint64_t[] to uint8_t is safe. Casting uint8_t[] to FOO * isn't when alignof(FOO) > 1. That's why I really don't like uint8_t[] there. It happens to be amply aligned, and it happens to be used only with memcpy(). But neither is immediately obvious, or obviously bound to stay that way. > BTW I wonder if trace files are endianness-dependent by design -- > normally you'd store them in network byte order (= big endian) only and > use htonX() when writing, an ntohX() when reading. Good point, but outside the scope of this series. >> unused = fwrite(&dropped.rec, dropped.rec.length, 1, trace_fp); >> } >> >> @@ -220,11 +220,11 @@ int trace_record_start(TraceBufferRecord *rec, >> TraceEventID event, size_t datasi >> >> if (new_idx - writeout_idx > TRACE_BUF_LEN) { >> /* Trace Buffer Full, Event dropped ! */ >> - g_atomic_int_inc((gint *)&dropped_events); >> + g_atomic_int_inc(&dropped_events); >> return -ENOSPC; >> } >> >> - if (g_atomic_int_compare_and_exchange((gint *)&trace_idx, >> + if (g_atomic_int_compare_and_exchange(&trace_idx, >> old_idx, new_idx)) { >> break; >> } > > You decide if a respin is warranted... > > Reviewed-by: Laszlo Ersek <ler...@redhat.com>