Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libdispatch++ on Mac OS X with vanilla libdispatch #2

Open
DrPizza opened this issue Jun 22, 2011 · 16 comments
Open

libdispatch++ on Mac OS X with vanilla libdispatch #2

DrPizza opened this issue Jun 22, 2011 · 16 comments
Assignees

Comments

@DrPizza
Copy link
Owner

DrPizza commented Jun 22, 2011

(courtesy of bames53 on the Ars forums: http://arstechnica.com/civis/viewtopic.php?p=21780264#p21780264)

I'm building with clang++ -stdlib=libc++ -std=c++0x -Wall dispatch.cpp -c using clang version 3.0 (http://llvm.org/git/clang.git b10a13b1e2c34732d13b84494bb0a13a5822f945) and Target: x86_64-apple-darwin10.7.0 (which unfortunately does not yet support lambdas)

  1. The extern causes an error in the following #define. Same when I use Apple's gcc.
    #define DISPATCHPP_EXPORT extern __attribute__((visibility("default")))
  1. the typedef dispatch_function_apply_t doesn't exist in the libdispatch headers on OS X 10.6.
  2. dispatch_once, dispatch_once_f, and dispatch_get_main_queue are implemented as macros, so prefixing them with :: to access the symbols in the global namespace fails.
  3. dispatch_once is a macro so defining a function of the same name inside a namespace fails, as does trying to call that function.
  4. The dispatch_atomic_* are not part of the public interface to libdispatch and so they're not in the headers. To get around this I copy/pasted the definition from atomic.h in your src tree for the one dispatch_atomic_* function that's used in dispatch.cpp.
  5. use of libdispatch extension dispatch_get_current_thread_queue() is not protected by #ifdefs
  6. some Windows stuff not protected by #ifdefs: Windows.h, SDKDDKVer.h, ::DebugBreak()
  7. Due to lack of compiler support I replaced the use of lambda's with a function object. (ON_BLOCK_EXIT)

After these were fixed or worked around it was enough to get a couple simple programs to run:

void foo(size_t i) {
   printf("Hello %zu\n",i);
}

int main(int argc,char *argv[]) {
   gcd::queue::get_global_queue(0,0).apply(10,std::function<void(size_t)>(foo));
   return 0;
}

Code:
void foo(void) {
   printf("Hello");
}
void bar(void) {
   exit(0);
}

int main(int argc,char *argv[]) {
   gcd::queue::get_main_queue().async(std::function<void()>(foo));
   gcd::queue::get_main_queue().async(std::function<void()>(bar));
   dispatch_main();
   return 0;
}

Compiled with clang++ -stdlib=libc++ -std=c++0x -Wall dispatch.cpp main.cpp

Ugh, for some reason the github markdown is breaking the list numbering.

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 22, 2011

Huh, it sucks that there's no lambdas. I take it that's the case for both clang and g++?

  1. I thought I just copy/pasted that stuff. I think it's meant to denote that the symbol should be exported when building libdispatch++ into an .so but unfortunately I'm not particularly familiar with how that works.
  2. Oh yes, I added the typedef because its non-existence offended me, and because function pointer syntax is for bitches. I guess the solution will be to create new typedefs inside the gcd namespace.
  3. Huh, I didn't realize that dispatch_once and dispatch_once_f were macros. I did know about dispatch_get_main_queue. I think I can #undef them if #defined; I know that dispatch_get_main_queue simply shadows an actual function, so I assume the other two do too.
  4. Fixing the previous issue fixes this.
  5. That is annoying. I guess I can test to see if my atomic header was included, and if not, define all the atomic operations in libdispatch++. I think that probably makes the most sense.
  6. OK, straightforward, as the chance of getting the change accepted any time soon is slim! Which is a pity, but oh well. Cocoa seems to just use main thread affinity for things that in Windows are just "some particular thread" affinity; that's why I need the more general-purpose mechanism of thread-affinitized queues--I don't necessarily handle window messages on the first thread in the application, I just have to handle them on the thread owning the window.
  7. Ditto. Actually, I may be able to remove the inclusions altogether, and just depend on dispatch.h including them. This would just leave ::DebugBreak() to guard.
  8. Annoying. I guess for robustness, then, I need to use a named function there anyway. Can you show me your code?

Ultimately, I'd like to ensure that my entire libdispatch builds correctly on Mac OS X and functions properly as a drop-in replacement for the system library.

@ghost ghost assigned DrPizza Jun 22, 2011
@DrPizza
Copy link
Owner Author

DrPizza commented Jun 22, 2011

Also it's worth pointing out that without lambdas it's much less compelling, and you may be better off with the function pointer/void* overloads instead.

@ghost
Copy link

ghost commented Jun 22, 2011

Yeah, Apple won't ship a version of GCC new enough to have lambdas because they're all GPLv3. Clang will probably have them in a few months though. Which will be good because lambdas definitely are a necessary part of a compelling GCD API.

  1. The example I found via Google didn't have the extern: http://gcc.gnu.org/wiki/Visibility
  2. sounds good to me. Though maybe they'd take this added typedef upstream.
  3. The macros do a bit more than just calling a function, including breaking the cardinal rule of macro usage (and I mean the cardinal rule other than "don't"). Those macros are: #define dispatch_once(x, ...) do { if (__builtin_expect(*(x), ~0l) != ~0l) dispatch_once((x), (__VA_ARGS__)); } while (0) and #define dispatch_once_f(x, y, z) do { if (__builtin_expect(*(x), ~0l) != ~0l) dispatch_once_f((x), (y), (z)); } while (0)
  4. yep
  5. Nothing better occurs to me
  6. I liked your suggestion for an API that would return a queue associated with a HANDLE. Though maybe this function is part of the machinery for setting those queues up? If so then maybe it shouldn't be part of the public API.
  7. would assert() work as well?
  8. I just did a straightforward conversion from lambda to function object (except I changed it so the lambda did capture by value)
class release_counted_pointer {
public:
    release_counted_pointer(counted_pointer<counted_function_t>*fun) : m_fun(fun) {}
    void operator()() {
        m_fun->release();
    }               
private:
    counted_pointer<counted_function_t>* m_fun;
};

@ghost
Copy link

ghost commented Jun 22, 2011

here's a patch containing all my changes to make it compile. I would just push my changes to my fork but this is a second GitHub account and GitHub doesn't allow more than one user to have the same public key. If I make any actual worthwhile changes I'll have to mess with it and make it work.

From bb1de1edd0cc7807eb4e023f650778c0022cff43 Mon Sep 17 00:00:00 2001
Date: Wed, 22 Jun 2011 01:59:40 -0400
Subject: [PATCH] hacks to make it build on os x

---
 libdispatch++/src/dispatch.cpp |   34 ++++++++++++++++++++++++++++------
 libdispatch++/src/dispatch.hpp |    6 ++++--
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/libdispatch++/src/dispatch.cpp b/libdispatch++/src/dispatch.cpp
index 7f9b9fb..c5c0373 100644
--- a/libdispatch++/src/dispatch.cpp
+++ b/libdispatch++/src/dispatch.cpp
@@ -1,13 +1,23 @@
 #include "dispatch.hpp"

+#ifdef WIN32
 #include <SDKDDKVer.h>
 #include <Windows.h>
+#endif

 #include <cstdarg>
 #include <memory>

 #include "scopeguard.hpp"

+#define dispatch_atomic_dec(p) __sync_sub_and_fetch((p), 1)
+
+#include <cassert>
+void DebugBreak() {
+    assert(false);
+}
+
+
 namespace gcd
 {
    namespace
@@ -60,6 +70,18 @@ namespace gcd
            }
        }

+
+
+       class release_counted_pointer {
+       public:
+           release_counted_pointer(counted_pointer<counted_function_t>*fun) : m_fun(fun) {}
+           void operator()() {
+               m_fun->release();
+           }       
+       private:
+           counted_pointer<counted_function_t>* m_fun;
+       };
+
 #ifdef _MSC_VER
 #pragma warning(push)
 #pragma warning(disable: 4189)
@@ -69,7 +91,7 @@ namespace gcd
            try
            {
                counted_pointer<counted_function_t>* fun(static_cast<counted_pointer<counted_function_t>*>(context));
-               ON_BLOCK_EXIT([&] { fun->release(); });
+               ON_BLOCK_EXIT(release_counted_pointer(fun));
                (**fun)(count);
            }
            catch(std::exception&)
@@ -261,14 +283,14 @@ namespace gcd

    queue queue::get_main_queue()
    {
-       return queue(::dispatch_get_main_queue());
+       return queue(dispatch_get_main_queue());
    }
-
+/*
    queue queue::get_current_thread_queue()
    {
        return queue(dispatch_get_current_thread_queue());
    }
-
+*/
    queue queue::get_current_queue()
    {
        return queue(::dispatch_get_current_queue());
@@ -459,9 +481,9 @@ namespace gcd
        return ::dispatch_semaphore_wait(static_cast<dispatch_semaphore_t>(o), timeout);
    }

-   void dispatch_once(::dispatch_once_t& predicate, function_t work)
+   void once(::dispatch_once_t& predicate, function_t work)
    {
        std::unique_ptr<function_t> fun(new function_t(work));
-       ::dispatch_once_f(&predicate, fun.release(), &dispatch_function_object);
+       dispatch_once_f(&predicate, fun.release(), &dispatch_function_object);
    }
 }
diff --git a/libdispatch++/src/dispatch.hpp b/libdispatch++/src/dispatch.hpp
index 149bff0..e117e03 100644
--- a/libdispatch++/src/dispatch.hpp
+++ b/libdispatch++/src/dispatch.hpp
@@ -7,7 +7,7 @@
 #include <cstdint>

 #if __GNUC__
-#define DISPATCHPP_EXPORT extern __attribute__((visibility("default")))
+#define DISPATCHPP_EXPORT __attribute__((visibility("default")))
 #else
 #if defined(_WINDLL) // building a DLL
 #define DISPATCHPP_EXPORT __declspec(dllexport)
@@ -21,6 +21,8 @@
 #endif
 #endif

+typedef void (*dispatch_function_apply_t)(void*,size_t);
+
 namespace gcd
 {
    typedef std::function<void (void)> function_t;
@@ -108,7 +110,7 @@ namespace gcd
        long wait(dispatch_time_t timeout);
    };

-   DISPATCHPP_EXPORT void dispatch_once(::dispatch_once_t& predicate, function_t work);
+   DISPATCHPP_EXPORT void once(::dispatch_once_t& predicate, function_t work);
 }

 #endif
-- 
1.7.3.4

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 22, 2011

Well, I can understand avoiding GPLv3. Nonetheless, a little aggravating. Hopefully clang can catch up, and allow operating systems of various flavours to escape gcc's generally awful design.

  1. Hm, I will have to look into it some more, I guess.
  2. That's a thought; it's only a small change, and it's nice to have a name for the type.
  3. The extra stuff the macros do is irrelevant; it's just a minor optimization. They can safely be undefined.
  4. This function returns the thread-specific queue of the current thread. A quick example of this can be seen here: https://github.com/DrPizza/libdispatch/blob/master/libdispatchtest/src/dispatch_windows.cpp Essentially, this is the arbitrary thread equivalent to retrieving the main queue.
  5. Maybe, but for the time being I've changed it to use gcc's equivalent built-in instead. In theory, these functions should just do nothing (silent failure) if no debugger is attached; assert will crash instead. I want the former behaviour.
  6. OK, cool, glad that works.

Re: your diff, I actually made a bunch of modifications last night that obsoletes some of those changes, but I'll take the bits that are still relevant.

@ghost
Copy link

ghost commented Jun 23, 2011

turns out the dispatch_get_main_queue() macro isn't shadowing a function at all: #define dispatch_get_main_queue() (&_dispatch_main_q)

and now I've pushed my changes to my fork

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 23, 2011

Oh god, they are so fucking stupid.

The function exists and I believe is exported--it's certainly not static--but the fucking public header doesn't mention it.

Why do that!

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 23, 2011

If you just declare extern dispatch_queue_t dispatch_get_main_queue(void); in the global namespace, is that enough for the linker to find it?

@ghost
Copy link

ghost commented Jun 23, 2011

No, the linker reports it as an undefined symbol.

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 23, 2011

I saw the first time ;)

Oh, I guess it also needs to be extern "C".

If it still can't find it, that is rather annoying. Especially as I couldn't even get the macro to work; for some reason VC++ gave weird linker errors that didn't seem entirely appropriate, so I'm forced to put a proper declaration in the header.

@ghost
Copy link

ghost commented Jun 23, 2011

Ah, I should have seen that myself. extern "C" does indeed work.

@ghost
Copy link

ghost commented Jun 24, 2011

This function returns the thread-specific queue of the current thread. A quick example of this can be seen here: https://github.com/DrPizza/libdispatch/blob/master/libdispatchtest/src/dispatch_windows.cpp Essentially, this is the arbitrary thread equivalent to retrieving the main queue.

The thing that bothers me about it is it matters what thread it's called on. dispatch_get_main_queue() works for Cocoa because Cocoa requires all GUI updates to be on the same thread, so you can get the right queue from anywhere.

I think this is nicer, if it can actually be implmented:
https://github.com/bames53/libdispatch/commit/5cc4bef60da173d646dab92334cee3123ee374ab

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 24, 2011

Well, I agree that that would be a bit nicer, but there's no clear way to me to implement it from libdispatch's side of the fence. libdispatch doesn't know when HWNDs get conjured into existence, and thread queues are stored in TLS, so there's no easy way to get at them from outside the thread. They're also allocated only lazily; if a thread never touches its queue, it never gets created in the first place.

So, I think a satisfactory approach would be this: https://gist.github.com/1045683

In other words, leave it up to client code to keep its mapping of "arbitrary thing" (in this case, HWND) to dispatch_queue_t.

Now, naively one might say that you could do the same for a mapping from thread_id to queue too. It would probably be possible, but it would require more extensive changes in order to achieve the same affinitized queue draining behaviour.

@DrPizza
Copy link
Owner Author

DrPizza commented Jun 27, 2011

Intel TBB has a concurrent_hash_map that has safe concurrent insert and delete, so would be suitable for such a mapping.

@ghost
Copy link

ghost commented Aug 27, 2011

Just a note that your current version (1ed626c) of dispatch.cpp is still compiling without modification as of OS X 10.7.1 (11B26)

In case you didn't see and do care, the Lion modifications to libdispatch were released a little while ago and the maintainers were talking about taking changes before they merged Lion's modifications in.

@DrPizza
Copy link
Owner Author

DrPizza commented Aug 27, 2011

Yeah, I know they've made a load of changes. I'm a bit scared of looking at it all, to be honest; I fear that quite a lot of it will be a pain in the ass to port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant