HPCC-29914 Add embedded wasm support #17570

GordonSmith · 2023-07-10T15:29:20Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-07-10T15:29:38Z

https://track.hpccsystems.com/browse/HPCC-29914
Jira updated

ghalliday

@GordonSmith some initial comments. A few general themes

i) Need to be careful about types, and integer overflow.
ii) unicode is tricky - especially sizes of utf8 strings. I didn't check all the code, but some looks wrong.
iii) I didn't see any thread protection. E.g., hash table access/modification and other items need to be thread safe. I didn't check if there were any problems.

ghalliday · 2023-07-11T11:21:01Z

plugins/wasmembed/secure-enclave/abi.hpp

+    for (int i = 0; i < nbytes; i++)
+    {
+        int b = data[ptr + i];
+        if (i == 3 && is_signed && b >= 0x80)


i == 3 or i == nbytes-1? Is it a 1/2/3 byte int?

ghalliday · 2023-07-11T11:25:24Z

plugins/wasmembed/secure-enclave/abi.hpp

+        {
+            b -= 0x100;
+        }
+        result += b << (i * 8);


Left shifting a -ve signed number is undefined behaviour. It would work if b was unsigned.

ghalliday · 2023-07-11T11:26:20Z

plugins/wasmembed/secure-enclave/abi.hpp

+    return result;
+}
+
+std::string global_encoding = "utf8";


Is this a constant? There doesn't seem to be any way to change it.

It is constant for now - but will be configurable in a future version of wasmtime

ghalliday · 2023-07-11T11:27:29Z

plugins/wasmembed/secure-enclave/abi.hpp

+    return result;
+}
+
+std::string global_encoding = "utf8";


Problematic having a variable defined in a header - you will get duplicate definitions.

ghalliday · 2023-07-11T11:30:34Z

plugins/wasmembed/secure-enclave/abi.hpp

+    if (global_encoding.compare("utf8") == 0)
+    {
+        alignment = 1;
+        byte_length = tagged_code_units;


Not convinced about this. if tagged code units is the number of code points, then the byte length could be from tagged_code_units to 4*tagged_code_units depending on the characters

They do distinguish between tagged_code_points and tagged_code_units, so this should be good.

ghalliday · 2023-07-11T11:51:46Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+    {
+        auto instanceItr = wasmInstances.find(wasmName);
+        if (instanceItr == wasmInstances.end())
+            throw std::runtime_error("Wasm instance not found: " + wasmName);


careful about all throws...

ghalliday · 2023-07-11T11:54:06Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+    virtual void bindUTF8Param(const char *name, size32_t chars, const char *val)
+    {
+        TRACE("bindUTF8Param %s %d %s", name, chars, val);
+        auto memIdxVar = wasmEngine->callRealloc(wasmName, {0, 0, 1, (int32_t)chars});


Need to calculate the size from the length. rtlUtf8Size(chars, val)

ghalliday · 2023-07-11T11:54:50Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+    {
+        TRACE("getSignedResult");
+        if (results[0].kind() == wasmtime::ValKind::I64)
+            return (int32_t)results[0].i64();


Cast removes the top bits

ghalliday · 2023-07-11T12:00:49Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+    {
+        TRACE("getUnsignedResult");
+        if (results[0].kind() == wasmtime::ValKind::I64)
+            return (int32_t)results[0].i64();


ghalliday · 2023-07-11T12:02:13Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+        TRACE("getUnsignedResult");
+        if (results[0].kind() == wasmtime::ValKind::I64)
+            return (int32_t)results[0].i64();
+        return results[0].i32();


probably best to cast to unsigned first - otherwise it is not clear if it is signed extended

ghalliday

Various random comments. A few comments from before are still relevant.

ghalliday · 2023-07-31T13:04:52Z

plugins/wasmembed/secure-enclave/abi.cpp

+
+int align_to(int ptr, int alignment)
+{
+  return std::ceil(ptr / alignment) * alignment;


Not sure what this is trying to do, but std::ceil isn't right. If trying to round down use

(ptr / alignment * alignment)
or more efficient assuming a power of 2:
ptr & ~(alignment-1)

Also, the parameters need to be more strictly typed. Are they uint32_t?

isAligned(x, y) would be return (x & (y-1)) == 0;

Looking in https://github.com/WebAssembly/component-model/blob/5a11e36e43244130a71919b7cdd82139e5fe65b3/design/mvp/canonical-abi/definitions.py outside of the tests, it is used to find the next aligned ptr given a current ptr - so am inclined to leave as is for now and look at optimisation once the abi is complete?

ghalliday · 2023-07-31T13:07:05Z

plugins/wasmembed/secure-enclave/abi.cpp

+  {
+    uint8_t b = data[ptr + i];
+    if (i == nbytes - 1 && std::is_signed<T>::value && b >= 0x80)
+      b -= 0x100;


This has no effect since b is a single byte. Much clearer to negate the value outside the loop.

ghalliday · 2023-07-31T13:07:48Z

plugins/wasmembed/secure-enclave/abi.cpp

+    uint8_t b = data[ptr + i];
+    if (i == nbytes - 1 && std::is_signed<T>::value && b >= 0x80)
+      b -= 0x100;
+    retVal += b << (i * 8);


this is an integer overflow (undefined behaviour) for 8 byte -ve numbers.

ghalliday · 2023-07-31T13:10:49Z

plugins/wasmembed/secure-enclave/abi.cpp

+//  More:  Not currently available from the wasmtime::context object, see https://github.com/bytecodealliance/wasmtime/issues/6719
+std::string global_encoding = "utf8";
+
+std::pair<uint32_t /*ptr*/, uint32_t /*byte length*/> load_string_from_range(const wasmtime::Span<uint8_t> &data, uint32_t ptr, uint32_t tagged_code_units)


Should have a comment to clarify what tagged_code_units is. I assume it is not the number of codepoints.

I will add a comment, but a "code unit" (and a "code point") are apparently the "correct" terminology to distinguish between a char and a byte: https://unicode.org/glossary/

ghalliday · 2023-07-31T13:11:45Z

plugins/wasmembed/secure-enclave/abi.cpp

+
+std::pair<uint32_t /*ptr*/, uint32_t /*byte length*/> load_string_from_range(const wasmtime::Span<uint8_t> &data, uint32_t ptr, uint32_t tagged_code_units)
+{
+  std::string encoding = "utf-8";


minor. 4 byte indentation is used throughout the rest of the platform code.

ghalliday · 2023-07-31T13:29:54Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+        TRACE("bindVStringParam %s %s", name, val);
+        bindStringParam(name, strlen(val), val);
+    }
+    virtual void bindUTF8Param(const char *name, size32_t code_points, const char *val)


style: mixture of multi-word casing conventions. code_points/code_units and elemType, totalBytes. camel case is generally used in most of the platform.

ghalliday · 2023-07-31T13:30:42Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+    virtual void bindUTF8Param(const char *name, size32_t code_points, const char *val)
+    {
+        TRACE("bindUTF8Param %s %d %s", name, code_points, val);
+        auto code_units = rtlUtf8Size(code_points, val);


size (or bytes) is clearer than _units. e.g. val_size (or sizeVal)

ghalliday · 2023-07-31T14:30:49Z

plugins/wasmembed/secure-enclave/secure-enclave.cpp

+        uint32_t strPtr;
+        uint32_t code_units;
+        std::tie(strPtr, code_units) = load_string(data, ptr);
+        __chars = rtlUtf8Length(code_units, &data[strPtr]);


__chars is not a standard name for the result parameter

ghalliday · 2023-07-31T14:34:46Z

plugins/wasmembed/wasmembed.cpp

+        }
+        void manifestPaths(ICodeContext *codeCtx)
+        {
+            if (codeCtx && !manifestAdded)


Does this need to be thread safe?

Nope - refactored away

ghalliday · 2023-07-31T14:36:10Z

plugins/wasmembed/wasmembed.cpp

+
+MODULE_EXIT()
+{
+    kill();


This could do with a clearer name - hard to tell from the context what it does. (I didn't search to find it.)

ghalliday

@GordonSmith a few comments relating to c++ details, but broadly looks good.

ghalliday · 2023-10-10T08:50:59Z

plugins/wasmembed/CMakeLists.txt

@@ -0,0 +1,67 @@
+project(wasmembed)
+
+set(CMAKE_CXX_STANDARD 20)


Is this going to cause problems on any platforms? Should this be inside the if(WASEMBED) test instead?

It should only be local to the wasmembed project, but I can relocate anyway

ghalliday · 2023-10-10T08:51:49Z

plugins/wasmembed/abi.cpp

+#include <sstream>
+#include <vector>
+
+auto UTF16_TAG = 1 << 31;


technically undefined behaviour. Would need to be 1U << 31. i.e. an unsigned value.

ghalliday · 2023-10-10T08:53:58Z

plugins/wasmembed/abi.cpp

+
+uint32_t align_to(uint32_t ptr, uint32_t alignment)
+{
+    return std::ceil(ptr / alignment) * alignment;


I would avoid using a floating point function in an integer context like this. Assuming alignment is a power of two use

(ptr + alignment -1) & ~(alignment - 1);

Also, the name of the function doesn't make it clear it is rounding up.

I agree WRT to the naming, but want to keep the names in sync with the official ABI spec.

ghalliday · 2023-10-10T08:55:46Z

plugins/wasmembed/abi.cpp

+    auto nbytes = sizeof(retVal);
+    for (int i = 0; i < nbytes; ++i)
+    {
+        uint8_t b = data[ptr + i];


use an unsigned type of the correct type, otherwise b << (i * 8) will promote b to an integer, and then you will get an integer overflow error. You need to use unsigned types of the right size to do operations like this.

ghalliday · 2023-10-10T08:57:07Z

plugins/wasmembed/abi.cpp

+    }
+    if (std::is_signed<T>::value)
+    {
+        retVal += (data[ptr + nbytes - 1] & 0x80) ? -1 << (8 * nbytes) : 0;


This is also going to have integer overflow problems.

ghalliday · 2023-10-10T09:30:33Z

plugins/wasmembed/util.cpp

+
+std::pair<std::string, std::string> splitQualifiedID(const std::string &qualifiedName)
+{
+    std::istringstream iss(qualifiedName);


efficiency: std::istringstream is a fairly heavy-weight way to solve this problem. Simpler to use find to search for a '.' and then create substrings (similar to the function above).

ghalliday · 2023-10-10T10:52:52Z

plugins/wasmembed/secure-enclave.cpp

+
+// #define ENABLE_TRACE
+#ifdef ENABLE_TRACE
+#define TRACE(format, ...) DBGLOG(format, ##__VA_ARGS__)


I think this is non standard (only gcc). See https://en.cppreference.com/w/cpp/preprocessor/replace

I think #define TRACE(...) DBGLOG(VA_ARGS)

may work, but I'm not 100% sure. Would need some testing (or to use the c++20 extension.)

ghalliday · 2023-10-10T10:54:34Z

plugins/wasmembed/secure-enclave.cpp

+        return map.find(key) != map.end();
+    }
+
+    void for_each(std::function<void(const K &key, const V &value)> func) const


A bit of a mix of naming conventions: insertIfMissing and for_each, but maybe makes sense since latter is like a c++ std function.

ghalliday · 2023-10-10T10:57:18Z

plugins/wasmembed/secure-enclave.cpp

+
+        wasmtime::Linker linker(engine);
+        linker.define_wasi().unwrap();
+        TRACE("WASM SE resolveModule4 %s", wasmName.c_str());


I suspect quite a lot of this tracing could be removed as unlikely to be useful in the future.

ghalliday · 2023-10-10T11:04:15Z

plugins/wasmembed/secure-enclave.cpp

+        uint32_t strPtr;
+        uint32_t bytes;
+        std::tie(strPtr, bytes) = load_string(data, ptr);
+        rtlStrToStrX(chars, result, bytes, reinterpret_cast<const char *>(&data[strPtr]));


Need to convert from utf8 to a string.

ghalliday

@GordonSmith looks close. A few final comments

ghalliday · 2023-10-13T10:20:07Z

plugins/wasmembed/abi.cpp

@@ -124,12 +129,13 @@ T load_int(const wasmtime::Span<uint8_t> &data, int32_t ptr)
    auto nbytes = sizeof(retVal);
    for (int i = 0; i < nbytes; ++i)
    {
-        uint8_t b = data[ptr + i];
+        std::make_unsigned_t<T> b = data[ptr + i];


retVal will also need to be unsigned, otherwise you can get integer overflow

ghalliday · 2023-10-13T10:23:14Z

plugins/wasmembed/abi.cpp

        retVal += b << (i * 8);
    }
-    if (std::is_signed<T>::value)
+    if (std::is_signed<T>::value && data[ptr + nbytes - 1] & 0x80)


There also needs to be a check that nbytes != sizeof(T), because value << 8 * sizeof(T) is undefined behaviour.

There should also be a comment
//if the top bit is set then sign extend the result by setting the top bits.

NOTE: Running in debug on linux will throw errors when undefined behaviour is spotted.

ghalliday · 2023-10-13T10:26:52Z

plugins/wasmembed/secure-enclave.cpp

@@ -551,7 +542,7 @@ class SecureFunction : public CInterfaceOf<IEmbedFunctionContext>
        uint32_t strPtr;
        uint32_t bytes;
        std::tie(strPtr, bytes) = load_string(data, ptr);
-        rtlStrToStrX(chars, result, bytes, reinterpret_cast<const char *>(&data[strPtr]));
+        rtlUtf8ToStrX(chars, result, bytes, reinterpret_cast<const char *>(&data[strPtr]));


careful about difference between length/codepoints and size (I wish it wasn't this way!)

size32_t codepoints = rtlUtfLength(bytes, strPtr); rtlUtf8ToStrX(chars, result, codePoints, reinterpret_cast<const char *>(&data[strPtr]));

ghalliday

@GordonSmith I suspect this is close to ready to merge. I think it might be worth a walkthrough with me and someone else to look at and address any remaining issues.

ghalliday · 2023-10-25T11:15:54Z

plugins/wasmembed/secure-enclave.cpp

+        TRACE("WasmEngine loadWasmFiles");
+        IEngineContext *engine = codeCtx->queryEngineContext();
+        if (!engine)
+            throw makeStringException(100, "Faile to get engine context");


typo: failed

ghalliday · 2023-10-25T11:20:00Z

plugins/wasmembed/secure-enclave.cpp

+            throw makeStringException(100, "Faile to get engine context");
+
+        StringArray manifestModules;
+        engine->getManifestFiles("wasm", manifestModules);


I think this needs rethinking, although not for this PR. Currently it extracts to a file, but much better would be to return the contents as binary data.

ref: https://track.hpccsystems.com/browse/HPCC-30650

Add ticket to support multiple manifests loading diff wasm files.

Especially for Roxie.

https://track.hpccsystems.com/browse/HPCC-30692

ghalliday · 2023-10-25T12:12:24Z

plugins/wasmembed/secure-enclave.cpp

+
+    void registerInstance(const std::string &wasmName)
+    {
+        TRACE("WASM SE registerInstance %s", wasmName.c_str());


Are there thread safety issues here? Currently thread local, so no, but will that cause other issues?

I don't believe so, this PR reworked the engine / store threading specifically to match the threaded examples in the wasmtime repo (engine is a global and store / store context is per thread).

ghalliday · 2023-10-25T12:15:17Z

plugins/wasmembed/secure-enclave.cpp

@@ -266,29 +267,22 @@ class WasmEngine
        return found->second.data(store.context());
    }
 };
-static std::unique_ptr<WasmEngine> wasmEngine;
+thread_local std::unique_ptr<WasmStore> wasmStore = std::make_unique<WasmStore>();


Potential problems with code like this on osx and efficiency of thread_local variables with non-trivial constructors.

I am not sure how the lifetime of threading works within thor jobs - but I suspect this must be more efficient than a lock on the call.

I'm not sure. If I remember correctly it means every access of the variable needs to check if it is initialised, and if not call the constructor. We deliberately removed some thread_local variables like this because they were generating terrible code. I'll see if I can remember where/why.

See https://track.hpccsystems.com/browse/HPCC-27587

Open new ticket for efficiency access to wasm store: Move std::make_unique<WasmStore>(); into secure function constructor (testing if it exists first)

https://track.hpccsystems.com/browse/HPCC-30694

ghalliday · 2023-10-25T12:17:05Z

plugins/wasmembed/secure-enclave.cpp

-        std::tie(strPtr, bytes) = load_string(data, ptr);
-        rtlUtf8ToStrX(chars, result, bytes, reinterpret_cast<const char *>(&data[strPtr]));
+        std::tie(strPtr, encoding, bytes) = load_string(data, ptr);
+        size32_t codepoints = rtlUtf8Length(bytes, &data[strPtr]);


Should encoding be being used?

Not currently as everything is always a utf8 - this is more to keep in sync with official ABI and in the future when other encodings are supported.

ghalliday · 2023-10-25T12:17:42Z

plugins/wasmembed/secure-enclave.cpp

@@ -614,55 +597,27 @@ class SecureFunction : public CInterfaceOf<IEmbedFunctionContext>
    virtual void compileEmbeddedScript(size32_t lenChars, const char *_utf) override
    {
        TRACE("WASM SE compileEmbeddedScript");


Should this fail?

dockerfiles/wasm32-wasi/Dockerfile

plugins/wasmembed/CMakeLists.txt

plugins/wasmembed/hpcc-platform.wit

GordonSmith · 2023-10-27T10:50:59Z

plugins/wasmembed/secure-enclave.cpp

+            throw makeStringException(100, "Faile to get engine context");
+
+        StringArray manifestModules;
+        engine->getManifestFiles("wasm", manifestModules);


Add ticket to support multiple manifests loading diff wasm files.

Especially for Roxie.

https://track.hpccsystems.com/browse/HPCC-30692

plugins/wasmembed/secure-enclave.cpp

GordonSmith · 2023-10-27T11:03:05Z

plugins/wasmembed/secure-enclave.cpp

@@ -266,29 +267,22 @@ class WasmEngine
        return found->second.data(store.context());
    }
 };
-static std::unique_ptr<WasmEngine> wasmEngine;
+thread_local std::unique_ptr<WasmStore> wasmStore = std::make_unique<WasmStore>();


Open new ticket for efficiency access to wasm store: Move std::make_unique<WasmStore>(); into secure function constructor (testing if it exists first)

https://track.hpccsystems.com/browse/HPCC-30694

plugins/wasmembed/secure-enclave.cpp

testing/regress/ecl/wasmembed.ecl

GordonSmith · 2023-10-27T14:01:39Z

@ghalliday last couple changes as per todays meeting.

github-actions · 2023-10-27T14:08:39Z

https://track.hpccsystems.com/browse/HPCC-29914
This pull request is already registered

ghalliday

@GordonSmith please squash. I'll then quickly scan and merge if I don't spot anything.

GordonSmith · 2023-11-13T12:19:12Z

@GordonSmith please squash. I'll then quickly scan and merge if I don't spot anything.

Squashed and rebased - will re-review sometime this week once the tests succeed

GordonSmith · 2023-11-14T05:16:53Z

@ghalliday - removed some obsolete files and should be ready for pull

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>

GordonSmith · 2023-11-20T15:57:01Z

@ghalliday rebased

GordonSmith requested a review from ghalliday July 10, 2023 15:30

GordonSmith force-pushed the EMBED_WASM branch 3 times, most recently from a633050 to 5c3ee22 Compare July 11, 2023 07:41

ghalliday requested changes Jul 11, 2023

View reviewed changes

GordonSmith force-pushed the EMBED_WASM branch 2 times, most recently from 3a5ddb9 to 22ec93e Compare July 31, 2023 06:55

GordonSmith requested a review from ghalliday July 31, 2023 07:05

ghalliday requested changes Jul 31, 2023

View reviewed changes

GordonSmith force-pushed the EMBED_WASM branch 2 times, most recently from ed298a8 to d2e4d48 Compare October 5, 2023 21:08

GordonSmith changed the base branch from master to candidate-9.4.x October 5, 2023 21:09

GordonSmith force-pushed the EMBED_WASM branch from d2e4d48 to 3d56d73 Compare October 6, 2023 06:34

GordonSmith requested a review from ghalliday October 6, 2023 06:35

GordonSmith force-pushed the EMBED_WASM branch 4 times, most recently from 3f5b428 to f707a3e Compare October 6, 2023 09:09

ghalliday reviewed Oct 10, 2023

View reviewed changes

GordonSmith requested a review from ghalliday October 11, 2023 15:43

GordonSmith force-pushed the EMBED_WASM branch from 2d1597a to b0c4755 Compare October 12, 2023 06:18

ghalliday reviewed Oct 13, 2023

View reviewed changes

GordonSmith requested a review from ghalliday October 23, 2023 08:17

ghalliday reviewed Oct 25, 2023

View reviewed changes

GordonSmith requested a review from ghalliday October 26, 2023 11:58

GordonSmith commented Oct 27, 2023

View reviewed changes

GordonSmith closed this Oct 27, 2023

GordonSmith reopened this Oct 27, 2023

ghalliday approved these changes Nov 9, 2023

View reviewed changes

GordonSmith force-pushed the EMBED_WASM branch from f62abce to 7a50f81 Compare November 13, 2023 12:18

GordonSmith force-pushed the EMBED_WASM branch from 7a50f81 to 7b47d4d Compare November 14, 2023 05:05

HPCC-29914 Add embedded wasm support

0820f74

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>

GordonSmith force-pushed the EMBED_WASM branch from 7b47d4d to 0820f74 Compare November 20, 2023 15:56

ghalliday merged commit 7094fa3 into hpcc-systems:candidate-9.4.x Nov 21, 2023
41 of 45 checks passed

		@@ -0,0 +1,67 @@
		project(wasmembed)

		set(CMAKE_CXX_STANDARD 20)

HPCC-29914 Add embedded wasm support #17570

HPCC-29914 Add embedded wasm support #17570

Conversation

GordonSmith commented Jul 10, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jul 10, 2023

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GordonSmith Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GordonSmith Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GordonSmith Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

GordonSmith Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

GordonSmith commented Jul 10, 2023 •

edited

Loading

GordonSmith Oct 27, 2023 •

edited

Loading

GordonSmith Oct 27, 2023 •

edited

Loading

GordonSmith Oct 27, 2023 •

edited

Loading

GordonSmith Oct 27, 2023 •

edited

Loading

GordonSmith commented Nov 13, 2023 •

edited

Loading