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

[Laszlo Ersek]

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".

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

Anyway I don't insist reinstating the cast, I'm just saying that ISO C
would require it.


> 
> 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.)

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.


>              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>