Re: [Qemu-devel] [PATCH 1/4] trace: Fix simple trace dropped event record for big endian

[Markus Armbruster]

Laszlo Ersek <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>