Typed continuations: resume instructions #6083

frank-emrich · 2023-11-05T21:53:13Z

This PR is part of a series that adds basic support for the typed continuations proposal.

This particular PR adds support for the resume instruction. The most notable missing feature is validation, which is not implemented, yet.

frank-emrich · 2023-11-05T21:54:42Z

There are currently a few FIXMEs which are basically questions of mine, one of them currently causing my new lit test to fail (see my inline Github comment on that particular FIXME).

frank-emrich · 2023-11-05T22:04:20Z

src/ir/branch-utils.h

+      // We could also store this information in the Resume node itself, but
+      // that would mean storing the signature of all the Tags that we have a
+      // handle clause for.
+      func(name, Type::none);


The issue described here is what's causing the lit test suite to fail:
Enabling BINARYEN_PASS_DEBUG means that validateBinaryenIR eventually re-finalises each block by calling Block::finalize(). This can cause situations where the types collected by BranchSeeker fully determine the new type given to the block, ignoring the type annotation on it. This means that the code snippet here, where Type::none is passed as a placeholder for the real type causes problems, because Block::finalize() will obtain this from BranchSeeker and uses it as the overall type of the block. This then makes validateBinaryenIR fail.

This failure seems to be a relatively recent change, since the test suite was passing a few weeks ago when I did the majority of the work on this PR... But I guess it just shows that my "solution" of passing Type::none here was never quite right to begin with.

cc @aheejin, I guess the new EH design will also run into the same problem here: the types of values carried on the branches from the catch_table instruction will depend on the tags on that instruction, so we will need the module here to look up the types of those tags.

@frank-emrich, I don't think we have a choice but to plumb the module through to here sooner or later, and probably sooner, since I can't think of a good workaround 🤔

I've worked on this for a bit, which mostly involved changing all use sites of BranchSeeker, and have a question:

Currently, BranchSeeker is mostly used by calling its has and count functions. These don't care about the types sent to the branches types at all. In fact, there are few uses of BranchSeeker that do inspect the types field afterwards. And we only need access to the Module in order to correctly calculate the types sent to the branch. That means we would have to change Branchseeker::count and BranchSeeker::has to take a Module parameter, even though the types field is ignored.

This suggests that it may be useful to change the existing BranchSeeker such that it continues offering has and count with the existing signatures but remove the types field. This could be implemented entirely using operateOnScopeNameUses, no type (or module) information needed.

We then define a new BranchTypeSeeker that behaves more like the old BranchSeeker, which uses operateOnScopeNameUsesAndSentTypes, does accumulate the types sent to the branch, and needs access to the Module to do so to implement typed continuations and the new style EH.

This question came up when changing the signature of the various finalize methods of Block: The existing finalize() must definitely take a Module in the future, because it uses BranchSeeker to actually inspect the types sent to branches.
However, Block::finalize(Type type_) andBlock::finalize(Type type_, Breakability breakability) only use BranchUtils::BranchSeeker::has and don't care about the types sent to the block. Thus, if we go through with the splitting of BranchSeeker mentioned above, we could avoid also needing to add a Module parameter to these two.

I've mostly gone through with changing the signatures of all three Block::finalize functions now, so I don't care much which option we take here, but it felt a bit unfortunate to change all these use sites of Branchseeker::count and BranchSeeker::has, even though they don't actually care about the extra typing information.

This refactoring sounds good to me.

One option might be for the finalize methods that would need to take a Module to instead conservatively keep the type unchanged, then have special logic in the ReFinalize that takes the module into account to do better refinalization.

I have created a separate PR with the Module parameter plumbing: #6096. I will rebase this PR once the other one has landed.

After the discussion in #6096 I will now implement storing the sent types directly in Resume nodes, instead of using the Module here to determine them.

The most straightforward implementation is an ArenaVector<Type> whose i-th entry contains the type sent to the i-th block (i.e, curr->handlerBlocks[i]). Let's assume this vector is called sentTypes.

There is one different issue here that affects the overall implementation of operateOnScopeNameUsesAndSentTypes: Since it is implemented using operateOnScopeNameUses, a Resume instruction with n handler clauses results in n calls to the lambda defined in operateOnScopeNameUsesAndSentTypes, with the name of the corresponding handler block as the name argument each time. However, in the lambda we then have would run code like this:

... } else if (auto* res = expr->dynCast<Resume>()) { for (usize i = 0; < res->handlerBlocks.size(); i++) { auto handlerBlock = res->handlerBlocks[i]; if (handlerBlock == name) { func(name, curr->sentTypes[i]); } } } else { ...

In total, this is quadratic in the number of handler clauses.

Which of the following solutions would you prefer:

operateOnScopeNameUses takes a lambda whose parameter is a vector of names. We would effectively call the lambda "in bulk" for those node types that have more than one scope name use. But this would require creating singleton vector for all the other node types, so this doesn't seem efficient.

We could pass some kind of index to the lambda, indicating that the currently passed Name is the i-th block name used by the particular node. But that doesn't seem great either, the index would be meaningless for all node types other than Resume.

sentTypes could be a map from block names to types instead of just a vector of types. That seems relatively clean, but uses more space, and it would need to be integrated into the logic that updates tag names in case a naming section is encountered in a binary.

We may implement operateOnScopeNameUsesAndSentValues without using operateOnScopeNameUses at all: Instead, it could just be a visitor on Expressions. In the case for Resume nodes, we would then process all the blocks at once and invoke func for each handler block.

I'm personally in favor of the last option: After all, the lambda passed to operateOnScopeNameUses in operateOnScopeNameUsesAndSentValues already disambiguates the different node types, operateOnScopeNameUses is basically just used to pre-filter the nodes and extract the name of the used block.

Note that this entire discussion equally applies to Try nodes once the updated EH proposal is implemented.

4 makes sense to me as well.

How many handler clauses do you expect to have with resume? With try_table, at least LLVM at the moment will generate instructions with only one clause (it is either for the C++ tag/label or the catch_all label. We don't generate both at the same time. But even if we do it's gonna be only two), so we are not really gonna be affected by the quadratic loop. If what you would have in average is less than 5, I'm not sure if this is something we should worry about?

I've now re-implemented operateOnScopeNameUsesAndSentTypes using a Visitor.

@aheejin There are some compilation schemes where you may create resume instructions with tag clauses that handle more or less every single tag defined. However, even then I'd surprised to see more than a dozen or two. However, given that there are source languages with effect handlers that would like to target our instructions directly, it's basically up to what people do in those source languages. Having the quadratic behavior here is probably fine unless someone translates some crazy generated code using effect handlers into wasm.

But given that the solution using a visitor was relatively simple to implement, I'd say let's just err on the side of caution.

src/ir/branch-utils.h

src/ir/effects.h

src/wasm/wasm-binary.cpp

tlively · 2023-11-08T19:37:37Z

src/ir/branch-utils.h

+      // We could also store this information in the Resume node itself, but
+      // that would mean storing the signature of all the Tags that we have a
+      // handle clause for.
+      func(name, Type::none);


This refactoring sounds good to me.

One option might be for the finalize methods that would need to take a Module to instead conservatively keep the type unchanged, then have special logic in the ReFinalize that takes the module into account to do better refinalization.

frank-emrich · 2023-11-15T19:00:55Z

I've now addressed the first round of comments.

One thing that I've noticed is that updated_lit_checks.py cannot distinguish between tag definitions (tag $mytag ...) in the tags section of a module and handler clauses appearing in resume instructions: (resume $ct (tag $mytag $myblock) ...). As result, the script messes up the CHECK clauses. As temporary workaround, I've removed the "Assertions have been generated by update_lit_checks.py ..." header line from my new test file and removed the erroneously added CHECK lines manually.

I've had a quick look at the logic in updated_lit_checks.py. In general, we could distinguish the two kinds of (tag ...) expressions based on whether or not something like a (resume or (func appears beforehand. Anything indicating that we're not in the tag section any more should suffice. I've tried doing this with a negative lookahead before tag, but Python only accepts them if they have a constant length, which doesn't quite work for what I tried to do.

tlively · 2023-11-15T22:17:31Z

I think the update_lit_checks.py issue will be solved if you put the (tag ...) clause on the same line as the (resume ... rather than on its own line.

frank-emrich · 2023-11-16T18:04:08Z

I think the update_lit_checks.py issue will be solved if you put the (tag ...) clause on the same line as the (resume ... rather than on its own line.

That doesn't seem to have worked if I just move it on its own line in the input function. Did you mean changing the printer such that it goes on its own line in the output? That seems tricky, because there may be many such clauses, so at some point they have to go on their own lines, rather than unconditionally keeping them all on the same line as the resume.

tlively · 2023-11-27T05:23:36Z

Yes, I think you'd have to change the printer to put the tag clauses on the same line as (resume as well. Even though this does not visually scale to large numbers of tag clauses, it is at least consistent with what we do with e.g. function parameters and results, br_table labels, etc. In general our printer never tries to be clever and break up long lines.

frank-emrich · 2023-12-13T20:11:28Z

I've changed the printing now so that all the (tag ...) clauses appear on the same line as the resume. That did indeed fix the issue with update_lit_checks.py.

In general, things should be ready now for you to have a look at.

This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in WebAssembly#6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in WebAssembly#6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to WebAssembly#6083 (comment) and WebAssembly#6096 for related discussions when `resume` was added.

tlively

Generally looks good, just a few final comments.

tlively · 2023-12-14T20:10:26Z

src/ir/branch-utils.h

-      func(name, br->getSentType());
-    } else {
-      assert(expr->is<Try>() || expr->is<Rethrow>()); // delegate or rethrow
+  struct SentTypesVisitor : public Visitor<SentTypesVisitor> {


Just in the interest of minimizing the change, can this go back to being an if-else chain?

src/wasm.h

tlively · 2023-12-14T20:15:57Z

src/wasm-interpreter.h

    multiValues.pop_back();
    return ret;
  }
+  Flow visitResume(Resume* curr) { return Flow(NONCONSTANT_FLOW); }


This should probably be WASM_UNREACHABLE("unimplemented"). NONCONSTANT_FLOW is only used in the ExpressionRunner up above.

tlively · 2023-12-14T21:44:51Z

src/ir/effects.h

+      // on null.
+      parent.implicitTrap = true;
+
+      if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {


Is this because we're modeling suspending as throwing? Probably worth a comment if so.

This was just inspired by the following happening at the end of visitCall here :

// When EH is enabled, any call can throw. if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) { parent.throws_ = true; }

I'm not sure if it's really necessary for Resume. @aheejin would you mind having a look?

Can a resume instruction throw an exception? If not, we can remove this. (Calls can, which is why they have it.)

Yes, if the resumed continuation throws, it will bubble out from the resume instruction, just like an exception from a called function would.

tlively · 2023-12-14T21:46:47Z

src/ir/subtype-exprs.h

  void visitStringSliceWTF(StringSliceWTF* curr) {}
  void visitStringSliceIter(StringSliceIter* curr) {}
+
+  void visitResume(Resume* curr) { WASM_UNREACHABLE("not implemented"); }


This should just note that the body is a subtype of the resume's type.

I think It's slightly more complicated than that: We may also need to report subtyping relations induced by values being sent to handler blocks: If a resume instruction has a (tag $t $h) clause, then the n param types of $t must match the first n result types of block $h. I guess this means that we would have to record these relationships here and skipped it so far.

src/passes/Print.cpp

aheejin

Looking good; mostly nits. I appreciate your discussions on the tag types in #6083 (comment) and #6096, which I referred to before I submitted #6181.

src/wasm.h

aheejin · 2023-12-15T22:56:26Z

src/passes/Print.cpp

+  o << ' ';
+  printHeapType(curr->contType);
+
+  // We deliberate keep all (tag ...) clauses on the same line as the resume
+  // itself to work around a quirk in update_lit_checks.py
+  for (size_t i = 0; i < curr->handlerTags.size(); i++) {
+    o << maybeSpace << '(';
+    printMedium(o, "tag ");
+    printName(curr->handlerTags[i], o);
+    o << " ";
+    printName(curr->handlerBlocks[i], o);
+    o << ")";
+  }


+1 on moving this to PrintExpressionContents::visitResume. PrintSExpression seems to be mainly for child expressions.

aheejin · 2023-12-15T22:59:42Z

src/wasm-delegations-fields.def

  }
+
+  case Expression::Id::ResumeId: {
+    DELEGATE_START(Resume);


It seems we need to add sentTypes here as well. See #6181 (comment)

aheejin · 2023-12-15T23:23:08Z

src/wasm/wasm.cpp

+ArenaVector<Type>& Resume::getSentTypes() {
+  if (this->sentTypes.size() != this->handlerBlocks.size()) {
+    Fatal() << "Types sent to blocks have not been determined, yet.";
+  }
+  return this->sentTypes;
+}


Do we need to make sentTypes a private member and have this function separately? I think we can access it directly, and then this if check might better be in the validator.

aheejin · 2023-12-15T23:25:42Z

src/wasm/wasm-binary.cpp

+      auto resume = allocator.alloc<Resume>();
+      curr = resume;
+      visitResume(resume);


Suggested change

auto resume = allocator.alloc<Resume>();

curr = resume;

visitResume(resume);

visitResume((curr = allocator.alloc<Resume>())->cast<Resume>());

I'm not sure why MemorySize and MemoryGrow above didn't use it, but can we do this one-liner we used for other instructions above?

aheejin · 2023-12-15T23:57:04Z

test/lit/typed_continuations_resume.wast

@@ -0,0 +1,94 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.


I've been moving basic tests into test/lit/basic/ (#6156 and #6160) and make commands and CHECK lines consistent across files (#6159). Can you move this test to test/lit/basic/ as well and make the RUN commands the same with the tests there?

aheejin · 2023-12-15T23:58:18Z

src/wasm.h

+  void finalize();
+  void finalize(Module* wasm);


Can we

Suggested change

void finalize();

void finalize(Module* wasm);

void finalize(Module* wasm = nullptr);

Would this be simpler?

Also it would be nice if we have some comments, as suggested with TryTable in https://github.com/WebAssembly/binaryen/pull/6181/files#r1426054122.

aheejin · 2023-12-16T00:00:06Z

src/wasm/wasm.cpp

+void Resume::finalize() {
+  // Only performing some validation, but no finalization. The finalize(Module*)
+  // function must have been called previously.
+
+  if (!(this->contType.isContinuation() &&
+        this->contType.getContinuation().type.isSignature())) {
+    Fatal() << "ill-formed Resume expression";
+  }
+
+  if (this->sentTypes.size() != this->handlerBlocks.size()) {
+    Fatal() << "Resume node was not finalized";
+  }
+}
+
+void Resume::finalize(Module* wasm) {
+  if (!(this->contType.isContinuation() &&
+        this->contType.getContinuation().type.isSignature())) {
+    Fatal() << "ill-formed Resume expression";
+  }


Can we move this (validation in Resume::finalize() and Resume::finalize(Module* wasm) to wasm-validator.cpp? Also if we do

void finalize(Module* wasm = nullptr);

in wasm.h, we wouldn't need two methods.

aheejin · 2023-12-16T00:09:36Z

test/lit/typed_continuations_resume.wast

+ ;; CHECK-TEXT:      (type $3 (func (param (ref $ct)) (result i32)))
+
+ ;; CHECK-TEXT:      (tag $t (result i32))
+ (tag $t (result i32))


Can we have a test with resume using a tag with non-empty params as well?

Done, I've just changed $t in this test.

aheejin · 2023-12-16T00:12:55Z

src/wasm/wasm.cpp

+    if (tgps.size() > 0) {
+      TypeList sentValueTypes;
+      sentValueTypes.reserve(tgps.size() + 1);
+
+      sentValueTypes.insert(sentValueTypes.begin(), tgps.begin(), tgps.end());
+      sentValueTypes.push_back(ctPrimeRef);
+      this->sentTypes[i] = Type(sentValueTypes);
+    } else {
+      this->sentTypes[i] = ctPrimeRef;
+    }


Do we need these separate cases? The body within if would work fine even if tgps is empty.

Good point! The else branch is probably the more common case, where the handler block only receives the continuation, but no additional payloads. So separating the two cases just boils down to avoiding the allocation for a singleton std::vector<Type> (namely, sentValueTypes). Would you prefer avoiding the separate else case?

The current code looks good to me as is.

This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in #6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in #6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to #6083 (comment) and #6096 for related discussions when `resume` was added.

- sentTypes is public now - only one finalize method with optional Module* parameter - populateResumeSentTypes helper function

frank-emrich · 2024-01-08T15:53:53Z

Thanks for the reviews, @tlively and @aheejin. I've addressed all comments, with most of them being resolved now.
In general, I made things a bit more consistent with #6181. I've also run the fuzzer for 10k iterations.

aheejin

Thanks!

aheejin · 2024-01-10T02:35:21Z

src/wasm/wasm-validator.cpp

+  shouldBeTrue(
+    curr->sentTypes.size() == curr->handlerBlocks.size(),
+    curr,
+    "sentTypes cache in Resume instruction has not been initialised");


Suggested change

"sentTypes cache in Resume instruction has not been initialised");

"sentTypes cache in Resume instruction has not been initialized");

Nit: Binaryen codebase seems to use American spelling, so...

aheejin · 2024-01-10T02:37:29Z

src/wasm/wasm.cpp

+    if (tgps.size() > 0) {
+      TypeList sentValueTypes;
+      sentValueTypes.reserve(tgps.size() + 1);
+
+      sentValueTypes.insert(sentValueTypes.begin(), tgps.begin(), tgps.end());
+      sentValueTypes.push_back(ctPrimeRef);
+      this->sentTypes[i] = Type(sentValueTypes);
+    } else {
+      this->sentTypes[i] = ctPrimeRef;
+    }


The current code looks good to me as is.

tlively · 2024-01-11T21:23:38Z

Oops, I saw your spelling nit just after I merged this, @aheejin. I'll fix it.

This adds basic support for the new instructions in the new EH proposal passed at the Oct CG hybrid CG meeting: https://github.com/WebAssembly/meetings/blob/main/main/2023/CG-10.md https://github.com/WebAssembly/exception-handling/blob/main/proposals/exception-handling/Exceptions.md This mainly adds two instructions: `try_table` and `throw_ref`. This is the bare minimum required to read and write text and binary format, and does not include analyses or optimizations. (It includes some analysis required for validation of existing instructions.) Validation for the new instructions is not yet included. `try_table` faces the same problem with the `resume` instruction in WebAssembly#6083 that without the module-level tag info, we are unable to know the 'sent types' of `try_table`. This solves it with a similar approach taken in WebAssembly#6083: this adds `Module*` parameter to `finalize` methods, which defaults to `nullptr` when not given. The `Module*` parameter is given when called from the binary and text parser, and we cache those tag types in `sentTypes` array within `TryTable` class. In later optimization passes, as long as they don't touch tags, it is fine to call `finalize` without the `Module*`. Refer to WebAssembly#6083 (comment) and WebAssembly#6096 for related discussions when `resume` was added.

This PR is part of a series that adds basic support for the [typed continuations proposal](https://github.com/wasmfx/specfx). This particular PR adds support for the `resume` instruction. The most notable missing feature is validation, which is not implemented, yet.

[Typed continuations] resume instruction

f9ab9d4

frank-emrich commented Nov 5, 2023

View reviewed changes

tlively reviewed Nov 6, 2023

View reviewed changes

src/ir/branch-utils.h Outdated Show resolved Hide resolved

tlively reviewed Nov 6, 2023

View reviewed changes

src/ir/effects.h Outdated Show resolved Hide resolved

tlively reviewed Nov 8, 2023

View reviewed changes

frank-emrich mentioned this pull request Nov 9, 2023

Provide Module& parameter to operateOnScopeNameUsesAndSentTypes #6096

Closed

frank-emrich added 11 commits November 14, 2023 10:17

Merge remote-tracking branch 'upstream/main' into wasmfx-resume

2a932d3

dummy implementation for TypeGeneralizing pass

e96014a

new implementation of operateOnScopeNameUsesAndSentTypes

6b8787d

OverriddenScopeNameUseVisitor

a3cc291

pass nullptr to func in operateOnScopeNameUsesAndSentValues

dfbb193

resume instructions have implicitTrap effect

ef0a683

move comment on own line

53e40d4

manually remove (tag ...) lines erroneously added by updater script

8f85434

experiment: move SentTypesVisitor outside of lambda

b475268

combine both experiments

3c1906e

give up on OverriddenScopeNameUseVisitor

6ab20ad

add typed_continuations_resume.wast to fuzzer ignore list

3556171

frank-emrich added 3 commits November 21, 2023 12:49

remove visitTry, visitRethrow from SentTypesVisitor

bc86090

Merge remote-tracking branch 'upstream/main' into wasmfx-resume

97d54f7

dummy implementation of SubtypingDiscoverer::visitResume

cf17036

frank-emrich added 2 commits December 13, 2023 19:29

Merge remote-tracking branch 'upstream/main' into wasmfx-resume

db5b02a

when printing resume instructions, keep tag clauses on same line

115d0e2

aheejin mentioned this pull request Dec 14, 2023

[EH] Add instructions for new proposal #6181

Merged

tlively requested a review from aheejin December 14, 2023 20:08

tlively reviewed Dec 14, 2023

View reviewed changes

aheejin reviewed Dec 16, 2023

View reviewed changes

frank-emrich added 9 commits December 23, 2023 18:25

remove SentTypesVisitor in favor of simple loop

143bb2d

printing: move as much into PrintExpressionContents as possible

6749e08

Merge remote-tracking branch 'upstream/main' into wasmfx-resume

17be964

rephrase comment

c262d3c

Make Resume class more consistent with TryTable

fbcb7fb

- sentTypes is public now - only one finalize method with optional Module* parameter - populateResumeSentTypes helper function

include Resume's sentTypes field in field delegations

06a26c1

address minor nits

d41256d

move invariant checks to FunctionValidator::visitResume

ab6ff8b

move & update typed_continuations_resume.wast

179faa4

aheejin approved these changes Jan 10, 2024

View reviewed changes

tlively merged commit b4dee3d into WebAssembly:main Jan 11, 2024

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

		@@ -0,0 +1,94 @@
		;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.

	void finalize();
	void finalize(Module* wasm);
	void finalize(Module* wasm = nullptr);

	"sentTypes cache in Resume instruction has not been initialised");
	"sentTypes cache in Resume instruction has not been initialized");

Typed continuations: resume instructions #6083

Typed continuations: resume instructions #6083

Uh oh!

Conversation

frank-emrich commented Nov 5, 2023

Uh oh!

frank-emrich commented Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frank-emrich commented Nov 15, 2023

Uh oh!

tlively commented Nov 15, 2023

Uh oh!

frank-emrich commented Nov 16, 2023

Uh oh!

tlively commented Nov 27, 2023

Uh oh!

frank-emrich commented Dec 13, 2023

Uh oh!

tlively 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!

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!

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

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

frank-emrich commented Nov 5, 2023 •

edited

Loading

aheejin Dec 16, 2023 •

edited

Loading