Patchwork [1/4] trace: Fix simple trace dropped event record for big endian

login
register
mail settings
Submitter Markus Armbruster
Date Jan. 25, 2013, 3:43 p.m.
Message ID <1359128620-14226-2-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/215801/
State New
Headers show

Comments

Markus Armbruster - Jan. 25, 2013, 3:43 p.m.
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.  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.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 trace/simple.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Laszlo Ersek - Jan. 31, 2013, 11:18 a.m.
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 <armbru@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.)

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 <lersek@redhat.com>
Harsh Prateek Bora - Jan. 31, 2013, 11:58 a.m.
On 01/25/2013 09:13 PM, 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.  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.
>
> Signed-off-by: Markus Armbruster <armbru@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[];

I am just paranoid about this change, however if the patch has gone 
through a stress testing, I have no objections.

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

Just because of Glib's limitations, we are limiting our counting 
ability, that's bad.

>       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 like the above improvement for the price paid.

Lets see what Stefan has to say about this patch.

thanks,
Harsh

>               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;
>           }
>
Markus Armbruster - Jan. 31, 2013, 12:10 p.m.
Laszlo Ersek <lersek@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 <armbru@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 <lersek@redhat.com>
Laszlo Ersek - Jan. 31, 2013, 12:39 p.m.
On 01/31/13 13:10, Markus Armbruster wrote:
> Laszlo Ersek <lersek@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
Stefan Hajnoczi - Feb. 5, 2013, 8:56 p.m.
On Thu, Jan 31, 2013 at 12:18:52PM +0100, Laszlo Ersek wrote:
> 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.

Yes, they are host endian but the magic number tells you whether they
are in big or little endian.  So if a parser cared it could handle both
endiannesses.

Stefan

Patch

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