add high-level Library wrapper … #617

MartinNowak · 2013-09-28T03:17:34Z

loadLib now returns a Library struct
Library sub-types the native platform handle (alias this)
Library.findFunc loads a D mangled function
Library.findVar loads a D mangled variable

The single most annoying pull request I ever prepared. Too many long-standing unrelated bugs for such a straightforward implementation.

MartinNowak · 2013-09-28T03:23:10Z

And why does github sort commits in chronological instead of genealogical order?

JakobOvrum · 2013-09-28T04:44:46Z

Shouldn't we name them loadFunction and loadVariable to be consistent with loadLibrary?

Library.handle is a public and documented field; perhaps it should be private, and exposed only as a getter-only property? Further; publicly sub-typing void*? What's the rationale behind that? I think the change from the typed, cross-platform Library to the platform-specific, untyped void* should be explicit.

loadLibrary's documentation claims it returns null on error, but the documentation of Library does not describe a possible null-state.

MartinNowak · 2013-09-28T10:26:17Z

Shouldn't we name them loadFunction and loadVariable to be consistent with loadLibrary?

I find that annoyingly long, names where you always make a typo, and you have to write them much more often than loadLibrary. If it was only me, I'd name them func and var because lib.func("foo") is obvious enough.

Library.handle is a public and documented field; perhaps it should be private, and exposed only as a getter-only property? Further; publicly sub-typing void_? What's the rationale behind that? I think the change from the typed, cross-platform Library to the platform-specific, untyped void_ should be explicit.

We cannot add all functionality so people need to be able to use the underlying handle when necessary.
Sub-typing is mainly used, because it avoids breaking existing code.
Also we want to allow to initialize a Library with an existing handle.
NB: I've tried, but for some unknown reason I can't get the alias this to show up in the docs.

loadLibrary's documentation claims it returns null on error, but the documentation of Library does not describe a possible null-state.

That refers to the state of the super type. You can also test for equality with zero or use if (auto lib = loadLibrary("bla")) {}.

JakobOvrum · 2013-09-28T11:42:16Z

We cannot add all functionality so people need to be able to use the underlying handle when necessary.

Yes, the handle must be gettable, but I disagree that it should be arbitrarily settable which it currently is. I don't think it should be implicitly gettable either, but that's the issue quoted below.

Sub-typing is mainly used, because it avoids breaking existing code.

Well, I think subtyping void* in its entirety (lvalue) is particularly damaging to the integrity of the interface, so if this is going to be user-facing, I think we should figure out a way to deprecate it. As it is, Library can be assigned to any pointer and it's possible to do nonsense operations like performing pointer arithmetic on it.

Also we want to allow to initialize a Library with an existing handle.

As AliasThis only deals with implicit conversions from the UDT, the initialization/assignment issue is more appropriately solved with a constructor and opAssign overload.

That refers to the state of the super type. You can also test for equality with zero or use if (auto lib = loadLibrary("bla")) {}.

Right, I guess it's alright not to mention it as long as Library is a sub-type of void*.

JakobOvrum · 2013-09-28T11:45:41Z

I find that annoyingly long, names where you always make a typo, and you have to write them much more often than loadLibrary.

As for this issue, I don't buy these arguments; I don't think the full names are particularly long, and common words like "function" and "variable" seem to me like they should be less likely to be misspelled if anything.

However, I concede that it's hard to argue that the full names are inherently better on grounds other than the consistency-with-loadLibrary argument, so I think we'll have to agree to disagree.

MartinNowak · 2013-09-28T15:36:13Z

As for this issue, I don't buy these arguments; I don't think the full names are particularly long, and common words like "function" and "variable" seem to me like they should be less likely to be misspelled if anything.

Seriously yes, I just tried it stopping at the first mistake, see for yourself.

loadVarialb loadVarial loadVariable loadVairab loadVariable loadVariable loadVariable loadVarialbe loadVariable lod load loadVaira 5/12
loadVar loadVar loadVar load loadvar loadVar loadVar load loadVar loadVar loadVar loadVar loadVar loadVar 11/14
loadFucntipon loadFunction loadFucntio loadFunction loadFcun loadFucntion loadFuc loadFunction loadFcunt loadFunction loadFunction loadFucntion loadFucnt 5/13
loadFunc loadFucn loadFucn loadFunc loadFucn loadFunc loadFunc loadFunc loadFunc loadFunc loadFunc loadFunc loadFunc loadFunc loadFunc 12/15

It shouldn't be too surprising that short-term memorizing the movement to type a shorter word is easier and less error prone than for a long word. But usually I hit M-/ after 5-6 characters to hippie-expand the rest of the name.
My favorite line is auto sw = StopWatch(AutoStart.yes); which I find myself using quite often. Due to the longish name and the amount of camel-casing typing it requires way too much attention.

Looking at the alternatives I think it's a good compromise.
Nice and dense to read but not lacking information.

auto lib = load("libfoo.so"); // though that too short to be imported
auto pf = lib.func!bar();
auto pv = lib.var!baz();

auto lib = loadLibrary("libfoo.so");
auto pf = lib.loadFunction!bar();
auto pv = lib.loadVariable!baz();

auto lib = loadLib("libfoo.so");
auto pf = lib.loadFunc!bar();
auto pv = lib.loadVar!baz();

JakobOvrum · 2013-09-28T15:58:37Z

It shouldn't be too surprising that short-term memorizing the movement to type a shorter word is easier and less error prone than for a long word.

Well, then I'd like to invoke the argument that code is first and foremost supposed to be readily readable and understandable, not readily writable.

MartinNowak · 2013-09-28T16:23:37Z

Well, then I'd like to invoke the argument that code is first and foremost supposed to be readily readable and understandable, not readily writable.

Sure, that's most important, but at the same time short names that convey the same amount of information ARE easier to read.

MartinNowak · 2013-09-28T17:40:40Z

Another idea. We could ditch the Library and instead make the loadFunc/loadVar functions work with UFCS.
That way it causes no confusion Library is still just the handle returned from dlopen/LoadLibrary.

JakobOvrum · 2013-09-29T05:04:02Z

So you'd rather deprecate two symbols and be inconsistent with rt_loadLibrary/rt_unloadLibrary just so you can shave a couple of characters off two new functions? I think your take on this issue is seriously skewed...

JakobOvrum · 2013-09-29T05:05:04Z

Another idea. We could ditch the Library and instead make the loadFunc/loadVar functions work with UFCS.
That way it causes no confusion Library is still just the handle returned from dlopen/LoadLibrary.

We have a chance at type safety here. Let's not push void* on user code as if it was 1972.

MartinNowak · 2013-09-29T10:22:39Z

So you'd rather deprecate two symbols

They were supposed to be deprecated anyhow, because adding a Runtime namespace around symbols in a module core.runtime makes no sense.

just so you can shave a couple of characters off two new functions

Yes, these things are important because brief lines aid readability. Overlong names are a plaque as you can see by the terrible Java approach. In this case it doesn't matter so much, but I'd rather go with the short names if possible.

JakobOvrum · 2013-09-29T14:47:20Z

Yes, these things are important because brief lines aid readability. Overlong names are a plaque as you can see by the terrible Java approach. In this case it doesn't matter so much, but I'd rather go with the short names if possible.

Well, again let's agree to differ on which naming is better.

How about rt_loadLibrary and loadLibrary returning void* as they used to, then introducing the trio loadLib, loadFunc, and loadVar operating on Library? That way, the public interface of Library can be as clean as we want, while old code will keep working. What do you think?

MartinNowak · 2013-09-29T15:57:21Z

How about rt_loadLibrary and loadLibrary returning void* as they used to, then introducing the trio loadLib, loadFunc, and loadVar operating on Library? That way, the public interface of Library can be as clean as we want, while old code will keep working. What do you think?

👍
I just had the same idea, because it won't break existing Windows code.

MartinNowak · 2013-09-29T16:46:37Z

done

IgorStepanov · 2013-09-29T17:35:48Z

src/core/runtime.d

+
+    auto var1 = lib.loadVar!(size_t)("core.runtime.TestStruct.var");
+    auto var2 = lib.loadVar!(size_t, "core.runtime.TestStruct.var")();
+    auto var3 = lib.loadVar!(TestStruct.var)();


May be better another using of this function:

struct Foo { int* bar; } lib.load(Foo.bar); //set Foo.bar to pointer of `Foo.bar` symbol which mangled as a int variable?

The existing implementation I confused by the following fact:
Foo.bar variable need only to get name, type and linkage to the loadVar. We can not modify external Foo.bar through host Foo.bar.

This is intended to be used with .di headers for the library so it operates on the actual declarations.
What you do with the resulting address is up to you.

Is it typical situation, when user need to load shared library dynamically and you have an .di interface?
Why user can't link this library at compile time?

@IgorStepanov Any time that you do plugins, you need dynamically load them at runtime. They might not even exist yet when the program is compiled. Dynamically loading a library like this is the real benefit of shared libraries. There are obviously benefits for programs that just link against everything, but it's still debatable as to whether statically linking or dynamic linking is better, whereas for stuff like plugins, dynamically linking a library with a particular interface is really the only way to do it.

@jmdavis Yes, but when you use plugin, you haven't .di interface? Or I'm wrong? And you can write many different plugins for one .di header? But what about ODR? If you declare class Foo in you header 'foo.di', you can have only one foo.Foo class definition in your program.
You can't declare first foo.Foo in plugin_1.so, second foo.Foo in plugin_2.so and load both plugins at a same time. Is not it? May be I do not fully understand the mechanism of shared libraries.

You can't declare first foo.Foo in plugin_1.so, second foo.Foo in plugin_2.so and load both plugins at a same time. Is not it? May be I do not fully understand the mechanism of shared libraries.

Yes you can, it's the normal plugin use-case (see DConf Talk). This works because libraries aren't loaded into the global lookup scope (unless you specify RTLD_GLOBAL).
Recommended reading Drepper's DSO How to.

Thanks, it really opens up big opportunities. May be we also need to add function loadClass, which returns ClassInfo for this class? In can be usefull when host system declare interface for the plugins and plugin class implements this interface. In this case we can load ClassInfo for plugin from library, call create(), cast to interface and work with plugin through it.

You'll be able to use the existing ModuleInfo.localClasses metadata for that.
We will add a function to iterate over the modules of a loaded library.

But apparently Object.factory wouldn't know how to disambiguate identical FQNs.

JakobOvrum · 2013-09-30T00:07:37Z

I like the new interface.

Library and unloadLib's DDoc comments are out of date in places.

Should unloadLib take its argument by reference and set it to its null-state? Doesn't rule out accessing an unloaded library when there's aliasing, but as with destroy, surely it still helps a little. Also, its return value is undocumented.

I wish we could make it an error or exception to use the load* methods on an unloaded Library, but that doesn't look feasible for libdl as dlerror only returns a string error description, no error codes, as far as I can see...

We probably have to deal with Windows' reference counting on LoadLibrary and FreeLibrary somehow; do we specify that if unloadLib is called, it must not be called more times than loadLib?

MartinNowak · 2013-09-30T17:08:06Z

I like the new interface.

It's better.

Should unloadLib take its argument by reference and set it to its null-state?

Yes.

I wish we could make it an error or exception to use the load* methods on an unloaded Library

I'm a little undecided about this. From a composability standpoint I'd prefer to leave this as is, because simply testing the existence of a symbol is a valid use-case. I usually wrap all of these calls in enforce though.
For loadLib we can do something better, because dlerror is really informative here. For example it says, that libA.so couldn't be loaded because it contains an undefined symbol. And you can't easily get that information from the D API otherwise. It might also allows us to make Library a non-null struct.

We probably have to deal with Windows' reference counting on LoadLibrary and FreeLibrary somehow; do we specify that if unloadLib is called, it must not be called more times than loadLib?

Do you really think there is any doubt about this? And there are 2-levels of reference counting on linux, btw (#593).

JakobOvrum · 2013-10-01T05:46:55Z

I'm a little undecided about this. From a composability standpoint I'd prefer to leave this as is, because simply testing the existence of a symbol is a valid use-case. I usually wrap all of these calls in enforce though.

Testing the existence of a symbol in a loaded library should be perfectly valid, but attempting to load a symbol from a previously unloaded library is also going to return null, while it seems to me to be more of a logic error (akin to accessing an object after it has been explicitly destroyed).

ghost · 2013-10-01T20:56:16Z

Are the two calls to MultiByteToWideChar necessary? I mean surely there is some predefined length limit for DLL symbols (as there are limits to everything), in which case you could use a static array as a buffer and a single MultiByteToWideChar call.

MartinNowak · 2013-10-01T21:06:03Z

Are the two calls to MultiByteToWideChar necessary?

I haven't touched this code, only moved it, and despite it's ugliness I wouldn't want to stuff more than necessary into this pull request. Feel invited to make a pull once this is merged :).

ghost · 2013-10-01T22:05:54Z

Feel invited to make a pull once this is merged :).

Roger that.

Otherwise this pull LGTM, and it's green. Are there any objections for pulling it?

The single most annoying pull request I ever prepared.

If this is due to the ddoc'ed unittest bugs, apologies. I couldn't figure out a simpler way to implement them other than in the parser, since there's really no concept of "what is the previous visible symbol (w.r.t. version statements and static if)".

dnadlinger · 2013-10-01T22:39:26Z

I'm not too sure about the loadXyz naming scheme for the symbol addresses, as the functions don't actually load something that can be unloaded again, in contrast to loadLib/unloadLib. Maybe even the dreaded get (as in getFunc) would be the better choice for a verb?

MartinNowak · 2013-10-02T00:54:17Z

I'm not too sure about the loadXyz naming scheme for the symbol addresses, as the functions don't actually load something that can be unloaded again, in contrast to loadLib/unloadLib. Maybe even the dreaded get (as in getFunc) would be the better choice for a verb?

Yeah I also thought about this during the implementation but I forgot it. I used findFunc and findVar to emphasize that it performs a costly lookup.

If this is due to the ddoc'ed unittest bugs

Not really, though now the implementation structure is somewhat confusing.
It was more spending several hrs in OllyDbg to fix Issue 3956 and also tracking down Issue 8229.

Otherwise this pull LGTM, and it's green. Are there any objections for pulling it?

I think the iterations over the API were quite useful, so let's wait until there is no more input.

ghost · 2013-10-02T01:07:14Z

Somewhat OT: With regard to the DLL linking mess, there's also these: Issue 9220 (you've replied here) and Issue 6019. I've filed 6019 a long time ago though.

jacob-carlborg · 2013-10-02T06:46:37Z

src/core/runtime.d

+    {
+        auto lib = loadLib(null); // the executable
+        scope (exit) lib.unloadLib();
+    }


Why doesn't Windows and Posix use the same API here?

Maybe I should remove this from the documentation, you can't use Windows's LoadLibrary with null to obtain a handle to the executable.

Can't you use loadLib and when null is passed on Windows call GetModuleHandleA(null).

Yes, this seems like a sensible approach, even though I'm always a bit wary of using special values like that.

How often is this expected to turn up in actual client code?

How about rejecting null for the current loadLib and then providing a zero-argument overload that returns a reference to the current module?

How about rejecting null for the current loadLib

Yep, let's add a separate function for dlopen(RTLD_SELF, ...) later. The implementation is tricky, because which Library is returned depends on the caller, so this function could not live in druntime or we'd need to fake the return address.
No idea whether we can implement this semantic on Windows.
Maybe we could also come up with a better name than "loadLib no arguments".

jacob-carlborg · 2013-10-02T07:09:07Z

I just have to say that I really don't like these abbreviated function names. I would hate to see D become like C in this regard where basically every symbol name is abbreviated. The 70's are over, the length of symbol names doesn't matter anymore. D already has too many abbreviated symbol names, we don't need to repeat the same mistake again.

And no, just because we don't abbreviate symbols names doesn't mean it will have overly verbose names an in Java or Objective-C.

- Move unittest to directly after version (D_Ddoc) followed by the implementations. - Dummy symbol for optlink's Bug 3956 needs to be placed before the unittest.

- no implicit conversion to make Library more typesafe - use Library in the new loadLib/unloadLib API - deprecate void* API Runtime.loadLibrary/Runtime.unloadLibray - add Library.opCast(T:bool) to test for initialized lib

- add handle !is null invariant to Library - remove opCast and disable default constructor

- Adapt the docs to emphasize that a search is performed and null is returned on failure.

MartinNowak · 2013-10-03T22:26:59Z

I moved two independent commits into separate pull requests (#623, #624)
and will try to address the review comments within the next days.

dnadlinger · 2013-10-03T22:59:07Z

The problem is that once it's in a release, it will be around for a long time, at least if it isn't accompanied with a big red warning saying otherwise.

JakobOvrum · 2013-10-04T07:31:35Z

This is longer possible, because there is an invariant in Library that asserts a non-null handle.

Library is copyable. Only the reference passed to unloadLib is nullified.

(edit: oops, sorry about not replying as a line comment; but then again, I've mentioned this like three times throughout)

MartinNowak · 2013-10-04T13:50:47Z

The problem is that once it's in a release, it will be around for a long time, at least if it isn't accompanied with a big red warning saying otherwise.

Right, we should come up with a good API here.
I'm just saying that I want to add functionality incrementally because we just won't be able to tackle all surrounding issues at once.
Specifically this pull adds two things a Library struct wrapper around the native handle and functions to find symbols within the library.
If it made reviewing easier I could split the pull request.

ghost · 2013-10-20T13:18:52Z

So what should I do about the changelog? I could mention shared libraries being supported, but without a nice usable wrapper such as this I don't know if it's worth mentioning it. I have to give some use-cases on how to use the feature after all.

MartinNowak · 2013-10-29T15:04:47Z

So what should I do about the changelog? I could mention shared libraries being supported, but without a nice usable wrapper such as this I don't know if it's worth mentioning it. I have to give some use-cases on how to use the feature after all.

Better not. The feature is still incomplete and experimental.
It would make sense to mention the support for link-time shared libraries (and -defaultlib=libphobos2.so) iff we decide to fix the distributed libphobos2.so (dlang/phobos#1661).

ghost · 2013-10-29T17:50:46Z

Ok. Anyway the changelog was merged as soon as I made the pull, so if you decide you want to mention shared libs you can make a pull against dlang.org, no need to go through me.

MartinNowak · 2013-11-01T14:29:05Z

Ok. Anyway the changelog was merged as soon as I made the pull, so if you decide you want to mention shared libs you can make a pull against dlang.org, no need to go through me.

If we no longer use druntime's changelog.dd we should probably remove it.

MartinNowak · 2013-11-01T14:43:16Z

Library is copyable. Only the reference passed to unloadLib is nullified.

So what should we do about this?
I see 5 options.

Declare it as a programming error.
Try to detect that error using dlerror and dlopen(handle, RTLD_NOLOAD).
This means whenever we don't find a symbol we'd have to call dlopen to see
whether the handle is still valid. Also reopening a different library might silently
make the handle valid again.
Make Library non-copyable, cheap option but moveable-only structs are sometimes unhandy.
Use the runtime internal thread local reference count to account for each Library.
Copying from one thread to another (e.g. through a global variable) would have undefined semantics.
Use a shared atomic reference count (same as rtld refcount) and implicitly initialize
Libraries when they are copied to another thread.

I'd prefer either 1 or 3.
The problem with 4 and 5 is the implicit unloading when the ref count drops to zero.
It can lead to all sorts of program corruption so I think unloading should be explicit.

MartinNowak · 2013-11-28T21:54:14Z

Ping @JakobOvrum

JakobOvrum · 2013-11-30T02:26:15Z

I have no idea what the best solution is :(

It's an awkward spot because library handles are reference counted in both libdl (right?) and on Windows, but we can't get at that count with libdl. dlerror only provides textual error information, which cannot be used to programmatically identify the nature of the error (translations etc), unlike GetProcAddress which uses GetLastError, which returns an error code.

A non-copyable type is probably just fine for most use cases where the library is never unloaded (think libraries like Derelict), but when uses like plugins are involved, the Librarys need to be stored somewhere for later unloading, which bodes trouble if the type is non-copyable. One thing to note is that a non-copyable type is forward compatible with a copyable one, but the converse is not true.

One note about 4 is that the type system prevents non-null instances of Library to be sent between threads (unless unsafe casts are used). Despite being unconventional, I actually like the idea of using thread-local reference counting, forcing the user to retain instances of Library, which to me seems like a small compromise for enforceable safety.

nazriel · 2014-01-29T11:26:13Z

@MartinNowak any decisions were made?

MartinNowak · 2014-01-30T01:11:50Z

I will continue to work on this soonish.

andralex · 2014-03-16T16:54:15Z

ping pliz pliz

MartinNowak · 2014-03-16T20:31:58Z

This got delayed because the release building took much longer.
I already allocated time for this for 2.066 (http://wiki.dlang.org/Agenda#high-level_shared_library_support) and might be able to do this in ~2 weeks.

andralex · 2014-03-16T20:34:00Z

ok thanks, please email me if blocked

AndrewEdwards · 2014-07-20T03:36:49Z

Ping @MartinNowak @andralex, status update please.

quickfur · 2014-08-05T23:06:31Z

ping
Rebase, please

DmitryOlshansky · 2014-09-28T22:14:10Z

I believe this is of critical importance to consider D having good support for shared libraries.
@MartinNowak thoughts?

IgorStepanov · 2014-09-28T22:36:59Z

@MartinNowak You seems a bit abstracted recent months. Are you ok? When does you return to us? :)

andralex · 2015-01-29T21:51:27Z

@MartinNowak this seemed to go on the backburner... close until you can get back to it?

ghost · 2015-08-11T03:48:06Z

@MartinNowak Is this branch still viable? This feature is pretty valuable.

MartinNowak · 2015-08-11T10:45:19Z

@MartinNowak Is this branch still viable? This feature is pretty valuable.

Yes, but I'm busy with other topics currently.
Also think we should get the low-level support for most platforms first, at least OSX, maybe not yet Windows.

Domain · 2017-04-24T02:27:12Z

@MartinNowak 4 years passed! Any plan to update this?

IgorStepanov reviewed Sep 29, 2013
View reviewed changes

jacob-carlborg reviewed Oct 2, 2013
View reviewed changes

MartinNowak added 5 commits October 4, 2013 00:24

workaround Bug 11133 - documented unittest outside of version scope

1ecf4af

- Move unittest to directly after version (D_Ddoc) followed by the implementations. - Dummy symbol for optlink's Bug 3956 needs to be placed before the unittest.

remove alias this handle in Library

a6cf9dc

- no implicit conversion to make Library more typesafe - use Library in the new loadLib/unloadLib API - deprecate void* API Runtime.loadLibrary/Runtime.unloadLibray - add Library.opCast(T:bool) to test for initialized lib

use exception handling for loadLib/unloadLib

5079abf

- add handle !is null invariant to Library - remove opCast and disable default constructor

rename loadFunc/loadVar to findFunc/findVar

314fa86

- Adapt the docs to emphasize that a search is performed and null is returned on failure.

wrap dlsym in a D function to avoid libdl dependency in client code

4ad6e56

MartinNowak closed this Jan 29, 2015

Uh oh!

add high-level Library wrapper … #617

add high-level Library wrapper … #617

Uh oh!

Conversation

MartinNowak commented Sep 28, 2013

Uh oh!

MartinNowak commented Sep 28, 2013

Uh oh!

JakobOvrum commented Sep 28, 2013

Uh oh!

MartinNowak commented Sep 28, 2013

Uh oh!

JakobOvrum commented Sep 28, 2013

Uh oh!

JakobOvrum commented Sep 28, 2013

Uh oh!

MartinNowak commented Sep 28, 2013

Uh oh!

JakobOvrum commented Sep 28, 2013

Uh oh!

MartinNowak commented Sep 28, 2013

Uh oh!

MartinNowak commented Sep 28, 2013

Uh oh!

JakobOvrum commented Sep 29, 2013

Uh oh!

JakobOvrum commented Sep 29, 2013

Uh oh!

MartinNowak commented Sep 29, 2013

Uh oh!

JakobOvrum commented Sep 29, 2013

Uh oh!

MartinNowak commented Sep 29, 2013

Uh oh!

MartinNowak commented Sep 29, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Sep 30, 2013

Uh oh!

MartinNowak commented Sep 30, 2013

Uh oh!

JakobOvrum commented Oct 1, 2013

Uh oh!

ghost commented Oct 1, 2013

Uh oh!

MartinNowak commented Oct 1, 2013

Uh oh!

ghost commented Oct 1, 2013

Uh oh!

dnadlinger commented Oct 1, 2013

Uh oh!

MartinNowak commented Oct 2, 2013

Uh oh!

ghost commented Oct 2, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment