On 01/31/13 13:10, Markus Armbruster wrote: > Laszlo Ersek <ler...@redhat.com> writes: >> On 01/25/13 16:43, Markus Armbruster wrote:
>>> @@ -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. The thought of casting &arguments[N] to another pointer type didn't cross my mind (independently of the element type). Who would do such a thing? memcpy() is pretty much a requirement in this case, in my world. > 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. Changing the element type to uint64_t doesn't guarantee in general that (type*)&arguments[N] will be a correctly aligned pointer. (The guaranteed cases are when "type" is a character type, void, or uint64_t). Anyway my reviewed-by stands. Laszlo