<div dir="ltr"><div><div><div>Chang,<br></div>We cannot change WL_SERVER_ID_START. I'm not 100% sure why we chose that value but it's probably because there are significantly more client-side allocated objects than server-side. However, at this point, it is fairly well baked into the protocol and changing it would break backwards compatibility badly. <br>
<br></div>And yes, the wl_map structure could stand to be documented better.<br></div>--Jason Ekstrand<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 9, 2013 at 8:10 PM, Chang Liu <span dir="ltr"><<a href="mailto:cl91tp@gmail.com" target="_blank">cl91tp@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Hi Jason,<br>
Thanks for the review! Indeed, on 32-bit big-endian systems we cannot guarentee that the LSB of next and data will coincide. So on these systems bumping next to 64 bits might not be a good idea.</p>
<p dir="ltr">I agree that on 32-bit systems this is likely to be a non-issue. Anyway I think we should at least document this (somewhat surprising) behaviour in the documentation.</p>
<p dir="ltr">Actually since we are going to have this wrap-back behaviour for client ids, why don't we just define WL_SERVER_ID_START to be simply 0x80000000? Is there any particular reason that we have to choose 0xff000000 for the split point of client and server ids? If we don't and current code doesn't depend on the precise value of WL_SERVER_ID_START, by simply redefining WL_SERVER_ID_START to be 0x80000000 we can be free of wrap-backs of ids and have a more symmetric split between client and server ids.</p>
<p dir="ltr">Cheers,<br>
Chang Liu</p><div class="HOEnZb"><div class="h5">
<div class="gmail_quote">On Sep 9, 2013 11:49 PM, "Jason Ekstrand" <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Chang,<div>I think what your doing here may technically be more correct, but I don't like it for two reasons.</div><div><br></div><div>First, I'm not 100% sure how the unions are going to pack and we heavily use the fact that the bottom bit of the value is for flagging it as unused. If the next and data fields are different sizes this could cause problems particularly when you consider 32bit vs. 64bit and little vs. bit endian. I'm not sure that we can guarantee that the bottom bits will still coincide if we simply change it to uint64_t.</div>
<div><br></div><div>Second, I think it's a non-issue. Sure, the protocol theoretically allows a client allocate 0x80000000 objects on a 32-bit system. However, doing so would create so many wl_resource and wl_proxy objects that both client and server would run out of address space long before we ever hit that mark. On a 64bit system, uintptr_t is big enough that it's not a problem anyway.</div>
<div><br></div><div>Thanks,</div><div>--Jason Ekstrand<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Sep 8, 2013 at 6:11 AM, Chang Liu <span dir="ltr"><<a href="mailto:cl91tp@gmail.com" target="_blank">cl91tp@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The current implementation of wl_map uses uint32_t for free_list.<br>
When removing client id >= 0x80000000, shifting said id by 1 bit left<br>
will cause free_list to wrap back to 0x0. Bump both wl_map->free_list<br>
and union_entry->next to uint64_t to fix this problem.<br>
---<br>
The relavent lines are:<br>
In wl_map_remove:<br>
start[i].next = map->free_list;<br>
map->free_list = (i << 1) | 1;<br>
If we are removing a client id i >= 0x80000000, the MSB of i will be lost<br>
when we perform i << 1, and this causes free_list to wrap back to 0x0.<br>
In wl_map_insert_new:<br>
if (map->free_list) {<br>
entry = &start[map->free_list >> 1];<br>
map->free_list = entry->next;<br>
}<br>
With free_list wrapped back to 0x0, we are longer able to recover the missing<br>
MSB in the client id even if we perform a right-shift to free_list.<br>
<br>
src/wayland-private.h | 2 +-<br>
src/wayland-util.c | 6 +++---<br>
2 files changed, 4 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/wayland-private.h b/src/wayland-private.h<br>
index 67e8783..5b3715d 100644<br>
--- a/src/wayland-private.h<br>
+++ b/src/wayland-private.h<br>
@@ -63,7 +63,7 @@ struct wl_map {<br>
struct wl_array client_entries;<br>
struct wl_array server_entries;<br>
uint32_t side;<br>
- uint32_t free_list;<br>
+ uint64_t free_list;<br>
};<br>
<br>
typedef void (*wl_iterator_func_t)(void *element, void *data);<br>
diff --git a/src/wayland-util.c b/src/wayland-util.c<br>
index 4fe9c81..1798fd6 100644<br>
--- a/src/wayland-util.c<br>
+++ b/src/wayland-util.c<br>
@@ -149,12 +149,12 @@ wl_array_copy(struct wl_array *array, struct wl_array *source)<br>
}<br>
<br>
union map_entry {<br>
- uintptr_t next;<br>
+ uint64_t next;<br>
void *data;<br>
};<br>
<br>
#define map_entry_is_free(entry) ((entry).next & 0x1)<br>
-#define map_entry_get_data(entry) ((void *)((entry).next & ~(uintptr_t)0x3))<br>
+#define map_entry_get_data(entry) ((void *)((entry).next & ~0x3))<br>
#define map_entry_get_flags(entry) (((entry).next >> 1) & 0x1)<br>
<br>
WL_EXPORT void<br>
@@ -291,7 +291,7 @@ wl_map_remove(struct wl_map *map, uint32_t i)<br>
<br>
start = entries->data;<br>
start[i].next = map->free_list;<br>
- map->free_list = (i << 1) | 1;<br>
+ map->free_list = ((uint64_t)i << 1) | 1;<br>
}<br>
<br>
WL_EXPORT void *<br>
<span><font color="#888888">--<br>
1.8.3.4<br>
<br>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</font></span></blockquote></div><br></div></div></div>
</blockquote></div>
</div></div></blockquote></div><br></div>