Initial draft of the layer 1. #33

KarlSchimpf · 2018-02-09T19:00:56Z

No description provided.

eholk

Thanks for writing this up! I left quite a few comments, but I'm going to go ahead and merge this as is. We can address the comments in later PRs. Some will need more in-depth discussion, which is probably best done via issues.

eholk · 2018-02-09T19:11:46Z

proposals/Layer-1.md

+2. Allow performance improvements in the VM.
+
+3. Introduce additional new functionality not available in layer 1.
+


We have a list here of what the proposal does not include, but it'd be good to include the requirements for the MVP. To me, these include:

Sufficient to implement C++ exception semantics by translating from Clang-generated LLVM code.

Allows WebAssembly to throw exceptions that can be caught by JavaScript.

Allows WebAssembly to catch exceptions thrown by JavaScript.

Can be implemented with low performance overhead.

The performance goal could be made more precise. I think it's roughly "code that does not use exceptions does not run any slower once the exception proposal has been adopted."

I opened #34 to track this.

eholk · 2018-02-09T19:13:22Z

proposals/Layer-1.md

@@ -0,0 +1,390 @@
+# Layer 1 exception handling


Is this meant to supersede the existing exceptions proposal?

The relation between the two docs is not clear to me either. Can this be clarified somehow?

eholk · 2018-02-09T19:15:50Z

proposals/Layer-1.md

+it may an unknown exception that was thrown by a called imported function.
+
+One of the problems with exception handling is that both WebAssembly and the
+host VM probably have different notions of what exceptions are, but both must be


nit: I'd probably say "embedder" rather than "host VM" here and elsewhere.

eholk · 2018-02-09T19:17:38Z

proposals/Layer-1.md

+It is difficult to define exceptions in WebAssembly because (in general)
+it doesn't have knowledge of the host VM. Further, adding such knowledge to
+WebAssembly would limit the ability for other host VMs to support WebAssembly
+exceptions.


For the first "host VM" here, it feels like you're talking about the JavaScript VM. Otherwise, it seems like adding exceptions to other host VMs would be no more limited than the host VM.

eholk · 2018-02-09T19:20:42Z

proposals/Layer-1.md

+VM.
+
+To access exceptions, WebAssembly provides instructions to check if the
+exception is one that WebAssembly understands. If so, the data of the


It would probably be helpful to make a distinction between "WebAssembly, the spec" and "WebAssembly, the program that is running embedded in a host VM." For the former, I'd suggest just WebAssembly, and for the latter I'd suggest "user code" or "user WebAssembly code."

eholk · 2018-02-09T19:34:51Z

proposals/Layer-1.md

+### The exception refernce data type
+
+Data types are extended to have a new `except_ref` type. The representation of
+an exception type is left to the host VM, but its size must be fixed. The actual


I think whether the size is fixed can be left up to the host VM. One could imagine a representation where they have a length field followed by that number of bytes. In practice, these are always going to be word-sized pointers though.

I added this constraint because local variables/global variables may be allocated in a linear memory in the embedder, and hence would require that.

Because an except_ref can hold any type of exception, it should have a common fixed size.

I think most reasonable implementations will have except_ref be a fixed size, but this is ultimately a question of representation. We don't expose any details of the representation to Wasm programs, so I think we should not make any requirements on what representations are allowed for the embedder provided they can otherwise implement the Wasm semantics.

eholk · 2018-02-09T19:36:44Z

proposals/Layer-1.md

+no exception is thrown, or the exception is successfully caught by the catch
+block.
+
+In the initial implementation, try blocks may only yield 0 or 1 values.


It sounds like we may get multi-value blocks pretty soon, so we don't we just do whatever the rest of Wasm does?

Added #36 to discuss this relationship.

eholk · 2018-02-09T19:47:53Z

proposals/Layer-1.md

+first search up the call stack to see if there is an enclosing try. If none are
+found, it could terminate the thread at the point of the throw. This would
+allow better debugging capability, since the corresponding call stack is still
+there to query.


Alternatively, the embedder can capture a stack trace at the point the exception object is created, which I think is roughly what JavaScript will do.

One question is whether stack traces should be captured at the point of creating the exception, or at the point of throwing the exception. I think this is irrelevant to the core spec, since stack traces are provided purely by the embedder and we provide no way to access them in WebAssembly. It's worth discussing in the context of the JS embedding though.

Discuss in #37

I don't disagree with this. I only wanted to state that it is up to the host on how to best handle this, and, depending on whether it is compiled for debugging or release, do slightly different implementations.

Sounds good.

If we are going to do the two-phase unwinding, this may have to be incorporated into wasm spec, because the toolchain (more precisely, the library like libcxxabi) has to support this behavior.

For example, for x86 EH, in the first phase, libcxxabi does not actually unwind the stack but just walks up the stack to see if there is a matching handler. If a matching handler is found, in the second phase it actually starts unwinding the stack until it reaches the handler found. If no matching handler is found, libcxxabi does not unwind the stack but just crashes at the point of throwing, leaving the whole stack trace intact.

I'm not suggesting we do this now, and for Layer 1 I think we don't need to do this anyway. I'm just suggesting if we do this later, this can't be just left to the discretion of the host VM but explicitly needs to be specified in the spec.

eholk · 2018-02-09T19:49:49Z

proposals/Layer-1.md

+zero with imported exceptions followed by internally-defined exceptions in
+the [exception section](#exception-section).
+
+## Changes to the binary model


binery model -> binary encoding

eholk · 2018-02-09T19:52:38Z

proposals/Layer-1.md

+
+| Opcode | Type constructor |
+|--------|------------------|
+| -0x41  |  `except_ref`    |


I'd make this 0x7B, since except_ref feels more like a value type to me.

Well, anyfunc will become a value type as well eventually. I think it makes sense to put all the reference types close together.

aardappel · 2018-02-09T19:35:23Z

proposals/Layer-1.md

+## Overview
+
+Exception handling allows code to break control flow when an exception is
+thrown.  The exeception can be any exception known by the WebAssembly module, or


typo: exeception

aardappel · 2018-02-09T19:45:55Z

proposals/Layer-1.md

+   instance of the corresponding tagged exception class, and if true it pushes
+   the corresponding values of the exception onto the stack.
+
+### The exception refernce data type


typo: refernce

aardappel · 2018-02-09T19:51:09Z

proposals/Layer-1.md

+new `exception section` of a WebAssembly module. The exception section is a list
+of exception types, from which exceptions can be created.
+
+Each exception type has a `type signature`. The type signature defines the list


Playing advocate of the devil here: since in the general case a C++ exception type cannot be represented as a self-contained wasm signature (e.g. if it contains a string) I'd expect the front-end to often or always opt for a signature that is just a single linear memory index. Similarly, a host exception will likely often be a single anyref, once we get that. This in turn does not explain to me why we need the generality of a type signature, other than, "why not, may be nice to have".

Its more of the latter. This topic was brought up in many earlier CG meetings and the general consensus was that exceptions should not just be C++ specific, but more general so that other languages could be translated down to WebAssembly.

The most important rationale (which should probably be mentioned) is that Wasm is an open, heterogeneous environment. If you catch an exception you have no way of knowing who generated it and how the payload is to be interpreted, unless you can identify it via an unforgeable tag. There may be multiple language mappings running interleaved in the same computation, or even multiple instances of the same language "runtime" that use separate memories, so just having an uninterpreted pointer would be unreliable.

aardappel · 2018-02-09T19:53:15Z

proposals/Layer-1.md

+
+Exception types can be imported and exported by adding the appropriate entries
+to the import and export sections of the module. All imported/exported exception
+types must be named to reconcile exception tags between modules.


What if my signature is simple, e.g. a single i32, does it mean that if I try to catch exceptions of this type, I will also catch exceptions from other modules that are unrelated? Do exceptions need some form of identifier to make them unique?

Each exception type has an opaque tag associated with it, so if my module declared $exception1 at index 1 as type i32 and $exception2 at index 2 as type i32, these are different types. So, an if_except 1 block would not match anything of type $exception2 and vice-versa.

Technically, I think these can alias, if they are both imported exceptions. The host may supply the same underlying exception to both imports, the same way that one could import the same function under two different names.

aardappel · 2018-02-09T19:56:27Z

proposals/Layer-1.md

+
+### Exception data extraction
+
+The `if_except block` defines a conditional query of the exception on top of the


This adds another type of control-flow block specifically for this purpose, which has an implementation cost. What alternatives have been considered, e.g. an instruction that loads the type or Nth element from an exception object, such that a the existing if block can be used to inspect it?

I agree that there is an implementation cost to the current proposal. On the other hand, there
may be implementation cost of loading elements individually - I.e. does the element loader need to verify the access is correct on each element lookup.

In the current vacuum of realistic examples, it is hard to say which is better, so one was chosen, based on previous discussions in the original proposal (in Exceptions.md).

I think the main advantage of the approach we've presented here is the guarantees provided by the type system. Let's say we had a get_type_index instruction and a get_exception_field instruction, we'd end up with code like:

if get_type_index == 0: get_exception_field 0, 0 else: ...

The problem is that at get_exception_field, we don't statically know the exception has the correct type (here it's pretty obvious because it shows up immediately after a type check, but it may not always be that easy to see). This means we have to insert a dynamic type check on top of the one we just generated.

Instead, the combined match and unpack that we have with if_except eliminates the need to insert any dynamic check beyond what the programmer has written already.

aardappel · 2018-02-09T20:03:37Z

proposals/Layer-1.md

+Lastly, exception lifetimes must be maintained by the host VM, so that it can
+collect and reuse the memory used by exceptions. This implies that the host must
+know where exceptions are stored, so that it can determine when an exception can
+be garbage collected.


If we had more strict structure around throw/rethrow, in theory at least exceptions could have static life-times. Requiring them to be GC/refcounted seems necessary specifically to accomodate hosts that want to manage their own exceptions in a more dynamic fashion.

Remember, not all exception lifetimes are controlled by WebAssembly code. Exceptions thrown by the host must follow the lifetime expectation of the host. Hence, using static lifetimes limit this use case.

The biggest problem here is that we are trying to generate Wasm code from LLVM. LLVM exception code has been lowered to the point that it now longer has the usual exception structure (for example, landing pads may be combined, there's cleanup code to handle, etc.). With more structured Wasm instructions, we can probably implement C++'s semantics, but the tricky part is to be able to rethrow a foreign (i.e. JavaScript) exception once C++ is done with its handling code.

I and others have advocated a couple of approaches that give us statically known exception lifetimes, but unfortunately these just haven't been workable for generating code from LLVM inputs. I'd love to be proven wrong, but this seems like a surprisingly hard problem.

This is a hard problem that eventually made us introduce the first-class exception object. tl;dr: CFG does not have enough original C++ scope information. While it may not be impossible we preserve that information from clang and carry that all the way to the backend, we kind of reached a conclusion that it is non-trivial and has the possibility of hindering optimization. #30 and #31 discussed this problem.

aheejin · 2018-02-12T10:49:10Z

proposals/Layer-1.md

+exceptions, assigning monotonically-increasing indices based on the order
+defined in the import and exception sections. Thus, the index space starts at
+zero with imported exceptions followed by internally-defined exceptions in
+the [exception section](#exception-section).


Are 'exception index' and 'exception tag' the same thing?

If the indices of imported exceptions start from 0, how does a wasm compiler know which tag to use to denote, let's say, C++ exception? Is it the linker's job to reconcile all the indices and fix all indices within a wasm program?

Are 'exception index' and 'exception tag' the same thing?

No, but they're similar. The exception tag is an opaque identifier for the type of the exception. The exception index points to an entry in the exception table, which has its own tag.

If the indices of imported exceptions start from 0, how does a wasm compiler know which tag to use to denote, let's say, C++ exception? Is it the linker's job to reconcile all the indices and fix all indices within a wasm program?

By wasm compiler, do you mean something like the C++ to Wasm compiler (as opposed to the Wasm to Native Code VM)? If so, then the linker seems like a reasonable place to reconcile all the exception indices. Ultimately, the notion of "the C++ exception type" does not really exist in the Wasm spec, but is a convention used by the tools.

The exception tag is an opaque identifier for the type of the exception. The exception index points to an entry in the exception table, which has its own tag.

Then an except instruction take an exception index as an immediate, right? In the Binary encoding section, it says tag, so it was confusing.

aheejin · 2018-02-12T11:02:13Z

proposals/Layer-1.md

+
+The exception names subsection is a `name_map` which assigns names to a subset
+of the _exception_ indices from the exception section. (Used for both imports
+and module-defined).


I can be mistaken, but can there be any debug info attached to not an actual exception object but an exception tag?

You could attach a name to an exception tag, which could count as very basic debug info, but in general I'd say no. Runtime info like a stack trace can only be attached to an exception object.

Then what can the capability of attaching a name to an exception index be used for?

It'd be similar to attaching a name to a function. Without names, Wasm call stacks basically look like "wasm_function_0, wasm_function_1, wasm_function_1, ...." In the same way, names on exceptions would let devtools say something like "Unhandled exception: foo" rather than "Unhandled Exception: my_module.wasm:0" or worse, "Unhandled exception: <wasm exception 0x4ea2df>".

What you describe is we attach debug info to exception objects. But if we assign names to exception indices, that doesn't mean individual exception objects can have separate names, no?

Correct, this would just be a name for the exception type, not the individual object.

aheejin · 2018-02-12T11:06:10Z

proposals/Layer-1.md

+| Name | Opcode | Immediates | Description |
+| ---- | ---- | ---- | ---- |
+| `try` | `0x06` | sig : `block_type` | begins a block which can handle thrown exceptions |
+| `catch` | `0x07` | | begins the catch block of the try block |


So this is supposed to be catch_all? If we are gonna add static the catch tag instruction later, shouldn't this be catch_all to make them different?

Agreed with @aheejin. OTOH, we may not even need to distinguish the two. Instead, catch has a list of exception ids that defines what it catches. In the degenerate case that it is empty it is a catch-all. In the MVP, we could then require it to be empty (although I don't necessarily think we should).

So to make sure I understand the idea, you mean have something like:

try: ... catch 0, 1: ... catch 2: ... catch ε: ...

I kind of like that idea (although we'd probably want to require disjoint exception indices for each catch clause, and aliasing might be a problem).

Or did you mean have a single catch clause overall?

The reason for allowing only catch all for MVP is that our main consumer, C++, doesn't really have any use for non-catch_all. I think it makes sense to not spec and implement a feature until we have users for it. Are there other languages that are interested in being among the first exception users?

Yes, I meant still just a single exception clause, but annotated with an (optional) list of exception ids. That should give us the best of both worlds: simple instruction structure and efficient implementation.

Why does C++ not have use for specific catch? I would assume that any catch that is not a catch(...) in C++ could be using this. Or does LLVM make that impossible? Either way, other languages would probably only catch specific exceptions most of the time.

I think we'd still want to be able to associate different code with different catch clauses. For example, say my language didn't follow the one exception type per language convention, but instead had Wasm exceptions for things like FileNotFoundException, NetworkTimeoutException, etc. I'd want to be able to handle these differently. Although there is a workaround of just nesting try blocks, or dynamically inspecting the exception inside the catch clause.

The issue with C++ and specific catch was discussed some in #31. @aheejin can probably explain the issues better but I think it comes down to a combination of LLVM's exception handling being very unstructured, and needing catch(...) to apply to both C++ exception and JavaScript exceptions. Basically, Clang/LLVM generates a big chunk of code that checks if an exception is known to C++ or not and does the appropriate thing. It sounds like to use specific catch we'd basically have to duplicate the exception handling code in the catch_all branch, and even finding which code to duplicate is non-trivial.

I agree that a combined catch/match makes certain optimisations easier, but I'm not sure how big a deal it is to do a data flow analysis instead. OTOH, the separation allows for more flexible control flow, which I thought was the motivation?

Subtyping on C++ exns seems like an orthogonal issue -- they'd still all have the same signle exn tag, don't they? But I can see how the destructor business makes this mostly a moot point in C++.

The goal is to get both. Keep the dynamic exception matching for cases where you need to more flexible control flow, but allow the more efficient static version when it's sufficient. I'm not sure how valuable this is though, and the complexity is significantly higher to support both in the spec. As you point out, a dataflow analysis would work, and I expect this would be a local analysis making it cheap to do (you wouldn't have to examine the whole program before you start compiling).

I see what you mean about C++ exceptions. If there are no destructors, then there's no reason to handle JavaScript exceptions at all unless there's a catch(...). I'm pretty sure there are basically always destructors though, probably even more so if you're using exceptions.

I think the idea of catch instruction that takes variable number of tags (or indices), as @rossberg suggested, haven't been discussed yet. Is it possible for an instruction to take variable number of immediate values?

I can't think of any instructions that currently do this, but there's nothing fundamentally impossible about it. Rather than encoding it as catch exception_index, we'd just encode it as catch vec(exception_index), similar to how function types are encoded.

See br_table, which has vec(labelidx) as immediate.

rossberg · 2018-02-12T15:58:17Z

proposals/Layer-1.md

@@ -0,0 +1,390 @@
+# Layer 1 exception handling


The relation between the two docs is not clear to me either. Can this be clarified somehow?

rossberg · 2018-02-12T16:00:43Z

proposals/Layer-1.md

@@ -0,0 +1,390 @@
+# Layer 1 exception handling
+
+Layer 1 of exception handling is the MVP (minimal viable proposal) for


I don't quite understand how to interpret the concept of "layer" here. Will other layers be future extensions? If so, we at least have to make sure that the features proposed here are future-proof wrt to such extensions.

I'll follow up with a PR to clarify this. Sort of like we did in the host bindings discussion, we realized we can separate out some of the aspects of exception handling into what's absolutely necessary to support C++, and things that are useful in that other languages might use them or they might allow VMs to do a more efficient implementation.

Part of the goal in separating these out (and documenting them, I'll follow up with a document listing the extensions today), is to make sure our MVP is future-proof wrt such extensions.

In the interest of keeping the spec small and growing conservatively, my default would be to get the exception MVP into the spec and then follow up with the extensions as needed. I'm open to arguments for including the extensions as well though.

Fully agreed. It was mainly the term "layer" and some of the explanation that made it sound like other things would be higher-level abstractions being added on top, which would be a quite different scenario.

rossberg · 2018-02-12T16:10:58Z

proposals/Layer-1.md

+
+To do this, WebAssembly exceptions are immutable once created, to avoid cyclic
+data structures that can't be garbage collected. It also means that exceptions
+can't be stored into linear memory. The rationale for this is twofold:


I think the most important reason is missing from this list, namely that they may be pointers into the host's memory, and it would be fundamentally unsafe to expose these, misinterpretations of data layout aside. Host references have to be opaque types.

rossberg · 2018-02-12T16:16:03Z

proposals/Layer-1.md

+disallows their usage in linear memory.
+
+A WebAssembly exception is created by the `except` instruction. Once an
+exception is created, you can throw it with the `throw` instruction. Thrown


This separation between creating an exception and throwing it is new. I think it is problematic on several levels, and not a future-proof design. It both gets in the way of certain optimisations / implementation strategies and is incompatible with possible future extensions like resumption (unless we make the type system more complicated).

We went back and forth on this and couldn't see a clear reason to pick one over the other. I find your resumption argument compelling though.

rossberg · 2018-02-12T16:21:15Z

proposals/Layer-1.md

+new `exception section` of a WebAssembly module. The exception section is a list
+of exception types, from which exceptions can be created.
+
+Each exception type has a `type signature`. The type signature defines the list


The most important rationale (which should probably be mentioned) is that Wasm is an open, heterogeneous environment. If you catch an exception you have no way of knowing who generated it and how the payload is to be interpreted, unless you can identify it via an unforgeable tag. There may be multiple language mappings running interleaved in the same computation, or even multiple instances of the same language "runtime" that use separate memories, so just having an uninterpreted pointer would be unreliable.

rossberg · 2018-02-12T17:00:44Z

proposals/Layer-1.md

+
+### Exception creation
+
+A `except` instruction has a single immediate argument, an exception tag.  The


Separating out exception value creation from throwing is likely to make throw less efficient, because the compiler does not statically know what it is throwing. In the previous design, the compiler statically knew where to read off the handler to jump to (each exn could have a thread-local handler stack attached to it); with the separation it probably requires a hash table lookup.

The separation will also cause problems with future extensions. In particular, when you consider resumption, the type of throw will depend on the return type of the exception. That would only work if the exn_ref type you propose tracked that result type.

I assumed (like return-call) that a throw that also creates could be added as new operator after MVP.

Since the combined operation is the simpler and more extensible choice (e.g. avoids the need for an exn_ref type) it seems more natural to go the other way round. Is there an immediate use case for the separated instructions?

rossberg · 2018-02-12T17:08:27Z

proposals/Layer-1.md

+
+| Opcode | Type constructor |
+|--------|------------------|
+| -0x41  |  `except_ref`    |


Well, anyfunc will become a value type as well eventually. I think it makes sense to put all the reference types close together.

rossberg · 2018-02-12T17:12:04Z

proposals/Layer-1.md

+
+| Field | Type | Description |
+|-------|------|-------------|
+| `count` | `varuint32` | The number of types in the signature |


To allow for future extension (esp resumption), we should change this in two ways:

Have the type be defined by a function type (denoted by an index into the type section).

Leave space for an additional reserved "attribute" byte that we require to be zero in the MVP (a hook to allow as adding more information in the future).

(I apologise that I steered us away from 1 before, which is more similar to what @KarlSchimpf has suggested long ago. I didn't consider resumption at the time.)

I agree with both comments.

rossberg · 2018-02-12T17:13:36Z

proposals/Layer-1.md

+#### High-level structure
+
+A new `exception` section is introduced and is named `exception`. If included,
+it must appear between the `Export` and `Start` sections of the module.


Why after export? Since exceptions themselves can be exported it would be natural that it appears before the export section.

Good point. They should be moved before exports, and maybe before imports.

Why before imports? ;) That would screw up the order of the index space.

My bad. I didn't realize that a section id number was defined for an exception section.

rossberg · 2018-02-12T17:16:41Z

proposals/Layer-1.md

+| Name | Opcode | Immediates | Description |
+| ---- | ---- | ---- | ---- |
+| `try` | `0x06` | sig : `block_type` | begins a block which can handle thrown exceptions |
+| `catch` | `0x07` | | begins the catch block of the try block |


Agreed with @aheejin. OTOH, we may not even need to distinguish the two. Instead, catch has a list of exception ids that defines what it catches. In the degenerate case that it is empty it is a catch-all. In the MVP, we could then require it to be empty (although I don't necessarily think we should).

Initial draft of the layer 1.

d2ff7d8

eholk approved these changes Feb 9, 2018

View reviewed changes

eholk merged commit f87ba21 into WebAssembly:master Feb 9, 2018

This was referenced Feb 9, 2018

Document MVP requirements in Layer1.md #34

Open

Bikeshed: names of exception handling instructions #35

Closed

JS Embedding: When to capture stack trace #37

Closed

aardappel reviewed Feb 9, 2018

View reviewed changes

aheejin reviewed Feb 12, 2018

View reviewed changes

rossberg reviewed Feb 12, 2018

View reviewed changes

KarlSchimpf deleted the layer1 branch February 26, 2018 21:39

aheejin mentioned this pull request Nov 6, 2018

Add support for the event section WebAssembly/tool-conventions#70

Merged

ioannad pushed a commit to ioannad/exception-handling that referenced this pull request Jun 6, 2020

Add memory index for {memory,table}.init (WebAssembly#33)

7ea73f7

		2. Allow performance improvements in the VM.

		3. Introduce additional new functionality not available in layer 1.


		### Exception data extraction

		The `if_except block` defines a conditional query of the exception on top of the

		@@ -0,0 +1,390 @@
		# Layer 1 exception handling

		Layer 1 of exception handling is the MVP (minimal viable proposal) for


		### Exception creation

		A `except` instruction has a single immediate argument, an exception tag. The

Initial draft of the layer 1. #33

Initial draft of the layer 1. #33

Uh oh!

Conversation

KarlSchimpf commented Feb 9, 2018

Uh oh!

eholk left a comment

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!

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!

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!

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!

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!

aheejin Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

aheejin Feb 12, 2018 •

edited

Loading