Switch to thread local storage from cutils. #820

Merged
merged 1 commit into from Sep 5, 2013

Conversation

Projects
None yet
3 participants
Contributor

brendandahl commented Aug 31, 2013

Fixes #782

Luckily cutils has a thread local storage implementation already.
Some notes:

  • Storing the js_msg function pointer in tls is tricky since data pointer aren't guaranteed to be the same size as function pointers. Casting to void* seems to be fine on osx and windows xp, but I'm not sure about the linux 64 distros.
  • To solve the above I use a wrapper structure and store the pointer to the wrapper structure. However, there seems to be something wrong with how the destructor is called when you quit firefox causing a crash. Gdb also won't give me a good backtrace on this crash so I'll leave a small memory leak in for now and hope to address it in the future.
  • I moved adb_sysdeps_init() into a initialization function to ensure mutex locks were initialized on windows before anything else is called.

Aside: we need to do some work on the build system, there's way to much to change just to add a file.

Contributor

bkase commented Sep 2, 2013

Awesome! This looks good. Is libadb working in XP now?

Sorry about the build system problems -- it's pretty hacky (especially those batch files in Windows). I can try to at least consolidate the list of files to one spot in one of the Makefiles. Let me know if that would be helpful.

@mykmelez mykmelez commented on the diff Sep 4, 2013

android-tools/adb-bin/js_message.c
+ FunctionJsMsg js_msg = wrapper->func;
+ void* temp = js_msg(channel, (void*)instance_ptr);
+ return temp;
+}
+
+static void free_js_msg(void* value) {
+ free(value);
+ value = NULL;
+}
+
+void _install_js_msg(FunctionJsMsg js_msg_) {
+ struct func_ptr* wrapper = (struct func_ptr*) malloc(sizeof(struct func_ptr));
+ wrapper->func = js_msg_;
+ // TODO: Fix this memory leak by figuring out why there is bad access when the
+ // destructor calls free_js_msg (even if the function is empty).
+ thread_store_set(&js_msg_key, wrapper, /* free_js_msg */ NULL);
@mykmelez

mykmelez Sep 4, 2013

Owner

Do you have any leads on the cause of this leak?

Owner

mykmelez commented Sep 4, 2013

Aside: we need to do some work on the build system, there's way to much to change just to add a file.

Indeed!

Sorry about the build system problems -- it's pretty hacky (especially those batch files in Windows). I can try to at least consolidate the list of files to one spot in one of the Makefiles. Let me know if that would be helpful.

That would be very helpful!

Owner

mykmelez commented Sep 5, 2013

Awesome! This looks good. Is libadb working in XP now?

Yes, with this changeset, libadb builds, loads, and works on XP.

Owner

mykmelez commented Sep 5, 2013

Storing the js_msg function pointer in tls is tricky since data pointer aren't guaranteed to be the same size as function pointers. Casting to void* seems to be fine on osx and windows xp, but I'm not sure about the linux 64 distros.

I built and tested on three linux64 distros, and it seems to work correctly on them:

  • CentOS 6.4
  • Ubuntu 10.04 LTS
  • Ubuntu 13.04
Owner

mykmelez commented Sep 5, 2013

Do you have any leads on the cause of this leak?

Erm, sorry, I mean the crash! In any case, I'm comfortable with this, provided we don't forget about the problem, so I filed #825 on it.

@mykmelez mykmelez added a commit that referenced this pull request Sep 5, 2013

@mykmelez mykmelez Merge pull request #820 from brendandahl/thread-local
Switch to thread local storage from cutils.
43a39dd

@mykmelez mykmelez merged commit 43a39dd into mozilla:master Sep 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment