Update musl to 1.1.15 #4813

jgravelle-google · 2016-12-20T23:08:59Z

This updates Emscripten's musl libc from 1.0.5 to 1.1.15

This was done by copying 1.0.5 into the emscripten source in order to revert all emscripten-specific changes (jgravelle-google@6993081), copying the new 1.1.15 in (jgravelle-google@3f0a2b4), and then reverting the first commit to reapply all emscripten changes at once (jgravelle-google@60dacbf). After that, all that was left to do was fix merge conflicts, and change any emscripten-specific workarounds to apply to new musl.

Of note is the assumption in newer versions of musl that __pthread_self always exists. bpowers/musl@19a1fe6 makes this explicit, and caused problems when compiling without -s USE_PTHREADS=1, because that would use library_pthread_stub.js, which used to stub __pthread_self at 0. Now we need to explicitly allocate a pthread (jgravelle-google@cc078e5) and initialize its dynamic fields (jgravelle-google@cce0512). We'd also like to make sure the whole thing is available entirely in the asm module (jgravelle-google@db9ce62) so that -s EVAL_CTORS=1 can handle calls that use it, in particular most iostream constructors rely on CURRENT_LOCALE, which is thread-specific.

This passes the entire core testsuite on my machine (for asmjs, asm2wasm, and s2wasm).

kripken · 2016-12-21T18:47:11Z

Nice work!

We also need to run the browser test suite on a browser with SharedArrayBuffer support, to test for regressions on pthreads support. Also the other and sanity test suites should be tested. I can help with all that. We should wait for after this week's merge of incoming to master though.

We will also need to bump the version number when landing this, so libc gets auto-rebuilt for people.

kripken · 2016-12-21T18:49:41Z

@juj, please take a look at the pthreads parts here when you can.

kripken · 2016-12-21T18:49:56Z

src/library_syscall.js

@@ -227,7 +227,7 @@ var SyscallsLibrary = {
  },
  __syscall5: function(which, varargs) { // open
    var pathname = SYSCALLS.getStr(), flags = SYSCALLS.get(), mode = SYSCALLS.get() // optional TODO
-    var stream = FS.open(pathname, flags, mode);
+    var stream = FS.open(pathname, flags | {{{ cDefine('O_LARGEFILE') }}}, mode);


Why is this change needed?

Because https://github.com/kripken/emscripten/pull/4813/files#diff-cffbb10ecdf95ae5cfcc9a524a924a75R246 : sys_open calls changed to automatically | O_LARGEFILE, and calls to sys_open that explicitly set that flag now do so implicitly (https://github.com/kripken/emscripten/pull/4813/files/b34ee72dd065960805c911ea10bd4d1ce30c8b09#diff-3042107f3f2299532432901f2c0200ffL13)

This seemed like the right place to do this because we previously forwarded syscall_cp as just syscall (https://github.com/kripken/emscripten/blob/incoming/system/lib/libc/musl/src/internal/syscall.h#L60), so it seemed sane to forward sys_open directly to syscall as well.
The better place to do this is probably in the syscall.h file itself, rewriting __sys_open2 and __sys_open3.

Looks like musl started to default to largefile, so you changed emscripten to as well?

I wonder if that affects performance, does it change all offsets to 64-bit? Seems like in asm.js and wasm32, we don't need largefile?

I think the behavior is the same; previously the open() wrapper around sys_open would always add the flag, so this just changes where the flag is added, right?

Moved the flag back to libc in 2c36bf0

That required some shenaniganry to get around cyclical macro expansions, but overall I think that makes the change clearer.

kripken · 2016-12-21T18:50:14Z

src/library_syscall.js

@@ -660,7 +666,9 @@ var SyscallsLibrary = {
  __syscall140: function(which, varargs) { // llseek
    var stream = SYSCALLS.getStreamFromFD(), offset_high = SYSCALLS.get(), offset_low = SYSCALLS.get(), result = SYSCALLS.get(), whence = SYSCALLS.get();
    var offset = offset_low;
+#if WASM_BACKEND == 0


Because https://github.com/kripken/emscripten/blob/incoming/system/lib/libc/musl/src/unistd/lseek.c calls this with an off_t shifted right by 32, to split what it assumes is a 64bit value into high and low bits. Because asmjs doesn't handle 64bit integers, we define off_t as just an int (3aa7fde#diff-f5c20f80e42860c7a8b52346283d5048R226), making the rightshift undefined behavior.
I don't remember what specifically about the wasm backend makes the value be garbage here (because it gets the same definition of off_t as an int), but it tripped this assert a bunch. Now I thought I remembered lseek.c changed in this PR to introduce the undefined shift, but it didn't so I'm not entirely sure why this assert gets hit now when it didn't previously.

I think for asm.js and wasm32 in general we don't need the higher bits anyhow, so how about making the assert unconditional? less differences between the wasm backend and others is better.

Right, the high bits are assumed to be unset and unused, hence the assert. I'll need to investigate more I think and figure out why wasm_backend has a value here, as opposed to just writing it off as undefined behavior misbehaving.

It might be equally sane to explicitly pass 0 for the high bits #ifdef __EMSCRIPTEN__ in the C source.

Fixed in 7fd78fb

This was due to undefined behavior: asm2wasm was compiling offset >> 32 to (i32.const 0), s2wasm was compiling it to a no-op, thus it wasn't writing to the vararg buffer that we pass to the syscalls, thus it was getting a garbage value from whatever was on the stack there first.

Fixed it by always just passing 0.

kripken · 2016-12-21T18:55:43Z

system/lib/libc/musl/arch/emscripten/syscall_arch.h

@@ -138,3 +138,4 @@ long __syscall333(int which, ...);
 long __syscall334(int which, ...);
 long __syscall340(int which, ...);

+#undef SYS_futimesat


https://github.com/kripken/emscripten/pull/4813/files#diff-49e70a6cdbc6da14094b7d6d57f11789R10 calls out to syscall(SYS_futimesat if it's been defined, and we don't have an implementation for it.

That can't be the only syscall we lack an impl for though, I think? :) is something special about this one? how did you notice this was needed?

Not fully sure.
Seems like because the implementation of utimensat, which we actually use, changed to include this reference. The other syscalls we don't use presumably get stripped by DCE before causing compile errors.
I still think undefining SYS_futimesat here is reasonable, because it eliminates a new codepath in utimensat that we don't support yet.

kripken · 2016-12-21T18:57:08Z

tests/langinfo/output.txt

@@ -1,4 +1,4 @@
-         CODESET: "UTF-8"
+         CODESET: "ASCII"


This is an interesting change, hmm. Is this expected?

I believe so: bpowers/musl@2d51c4a

I see, thanks. Hopefully users won't be affected by this.

kripken · 2016-12-21T18:59:34Z

Should I read the commits one by one? I see some apply/revert stuff which suggests not, i.e., they are temp commits that will be removed before merge? If it's practical to have each commit be useful though, that might be nice.

jgravelle-google · 2016-12-21T19:19:18Z

I ran the other browser and sanity tests and they came out mostly fine, but I was finding myself fixing issues with my setup and not the actual libc integration so I figured I'd get this up for review before spending too much time on it.

I am under no illusions that this gets merged before January :P

In general reviewing this one-by-one is probably fine; there's only a handful of small things that go back and forth (I think just the "UTF-8" -> "ASCII" change?), and the apply/revert stuff at the beginning is the bulk of the actual upgrade.

jgravelle-google · 2017-01-03T19:04:13Z

Ping @juj for a look at pthreads
Thanks!

dschuff · 2017-01-03T22:56:31Z

system/include/libc/assert.h

@@ -16,6 +16,9 @@
 extern "C" {
 #endif

+#if __EMSCRIPTEN__


Should probably be #ifdef instead of #if

Should this be done across the whole project in this PR, or just this one in libc for now?

I think this is the only place in libc, so just this one for now. Doesn't look like there are too many others in the project, but yeah, not in this PR.

kripken · 2017-01-10T22:09:37Z

lgtm for the parts I reviewed and commented on, thanks.

dschuff · 2017-01-10T22:59:52Z

LGTM too. Might be a good time to resolve the conflicts.

jgravelle-google · 2017-01-12T19:43:13Z

Rebased, ran test_core against asm2 and binaryen2 (asm2wasm), which passed modulo V8 failures on incoming (https://wasm-stat.us/builders/linux/builds/14988/steps/Execute%20emscripten%20testsuite%20%28asm2wasm%29/logs/stdio)

Will run browser tests this afternoon. @kripken, did you want to run more comprehensive tests on this before merging, or should we let the Emscripten buildbots handle it?

kripken · 2017-01-12T22:20:41Z

Bots sound ok. But I would like to get @juj's feedback here first. I pinged him on irc now.

juj · 2017-01-12T22:26:38Z

system/include/libc/wchar.h

 #undef iswdigit
-#define iswdigit(a) ((unsigned)(a)-'0' < 10)
+#define iswdigit(a) (0 ? iswdigit(a) : ((unsigned)(a)-'0') < 10)


Hmm, I wonder what this is all about? iswdigit(a) (0 ? iswdigit(a) : ((unsigned)(a)-'0') < 10)... looks really odd.

Apparently it's about better compile-time diagnostics for misuse: bpowers/musl@2ca55a9

The functions (including iswdigit) are declared, but only defined in terms of macros, and then the (unsigned) cast loses type information. So if someone calls them with an invalid argument type the frontend doesn't complain.

Ah that makes sense, thanks!

juj · 2017-01-12T22:27:26Z

system/lib/libc/musl/arch/emscripten/atomic_arch.h

@@ -5,6 +5,7 @@
 #include <emscripten.h>
 #include <emscripten/threading.h>

+#define a_ctz_l a_ctz_l


What is the rationale here in defining these to point to themselves?

Because bpowers/musl@1315596#diff-4c18db28385d6925bd8462b3a13fbce1 changed atomic.h to expect the archs to #define these, and if not it will provide the implementation. a_cas in particular raises an #error if not #defined

juj · 2017-01-12T22:29:36Z

system/lib/libc/musl/arch/emscripten/bits/alltypes.h

@@ -86,9 +86,9 @@ typedef long suseconds_t;
 #ifdef __EMSCRIPTEN__
 // For canvas transfer implementation in Emscripten, use an extra 10th control field


Please update this comment to say "extra 11th control field", so that it is clear that the extra variable is at index [10], and not the second to last element.

For the source of where this added field comes from, see this commit: 1b27891 and in particular, search for the string _a_transferredcanvases. That has the value

// Extra pthread_attr_t field: #define _a_transferredcanvases __u.__s[9]

in system/lib/pthread/library_pthread.c, which I believe should be updated to __u.__s[10] because musl added one field of their own.

Will do, thanks

Addressed in d08eb3d
... And moved _b_lock and _b_waiters back to where they were before that commit in c360ca5

juj · 2017-01-12T22:29:52Z

system/lib/libc/musl/arch/emscripten/bits/alltypes.h

@@ -97,28 +97,31 @@ typedef struct { union { int __i[9]; unsigned __s[9]; } __u; } pthread_attr_t;
 #ifdef __EMSCRIPTEN__
 // For mutex implementation in Emscripten, need to use an extra seventh control field
 // to hold a temporary futex wait & wake location, designated as mutex->_m_addr.
-typedef struct { union { int __i[7]; void *__p[7]; } __u; } pthread_mutex_t;
+typedef struct { union { int __i[8]; volatile int __vi[8]; volatile void *__p[8]; } __u; } pthread_mutex_t;


Since also here musl has added one field, then the Emscripten-specific field needs to also be bumped up by one:

#ifdef __EMSCRIPTEN__ #define _m_addr __u.__i[6] #endif

should now be __i[7] I think. See this commit which originally introduced it: e4d3138

Done in d08eb3d

juj · 2017-01-12T22:32:41Z

system/lib/libc/musl/src/aio/aio.c

+			.si_pid = getpid(),
+			.si_uid = getuid()
+		};
+#ifndef __EMSCRIPTEN__


Instead of #ifndeffing these out, let's just have them in, and create a no-op syscall for these, which would in #if ASSERTIONS mode console.log() out a warning of a no-op syscall being called.

Sounds good

Added stubs in d58db19, 593c068, and 44f9742

juj · 2017-01-12T22:33:26Z

system/lib/libc/musl/src/aio/aio_readwrite.c

@@ -20,7 +20,9 @@ static void notify_signal(struct sigevent *sev)
 		.si_pid = __pthread_self()->pid,
 		.si_uid = getuid()
 	};
+#ifndef __EMSCRIPTEN__


Here also, we don't need to #ifndef this out, but let's have a no-op stub syscall for this.

juj · 2017-01-12T22:39:17Z

system/lib/libc/musl/src/internal/libc.h

 struct __libc {
-	void *main_thread;
+	int can_do_threads;


This looks interesting. Perhaps we want to make sure that this gets initialized appropriately in -s USE_PTHREADS=0/1/2 builds.

On further inspection, not sure if that's worth doing. Currently, and when the field was added (bpowers/musl@dab441a), it only serves to return ENOSYS from pthread_create, which gets provided by library JS code.

Alright, makes sense.

juj · 2017-01-12T22:43:19Z

system/lib/libc/musl/src/internal/pthread_impl.h

 #ifdef __EMSCRIPTEN__
 // XXX Emscripten: The spec allows detecting when multiple write locks would deadlock, so use an extra field
 // _rw_wr_owner to record which thread owns the write lock in order to avoid hangs.
 // Points to the pthread that currently has the write lock.
-#define _rw_wr_owner __u.__i[2]
+#define _rw_wr_owner __u.__vi[2]


This is now incorrect. It looks like musl has added one field to the rwlock structure, old fields were _rw_lock at index 0, and _rw_waiters at index 1. In new musl there's _rw_lock and _rw_waiters at indices 0 and 1 as before, but a new field _rw_shared at index 2, so our Emscripten-specific deadlock detection control field at index 2 now conflicts with _rw_shared. We need to change _rw_wr_owner to index 3, and make sure that the size of that field is appropriate in Emscripten builds as well.

Done in d08eb3d

juj · 2017-01-12T22:45:01Z

system/lib/libc/musl/src/internal/pthread_impl.h

 #ifdef __EMSCRIPTEN__
-#define __wake(addr, cnt, priv) emscripten_futex_wake((void*)addr, (cnt)<0?INT_MAX:(cnt))
+	emscripten_futex_wake((void*)addr, (cnt)<0?INT_MAX:(cnt));


I see you changed the signature of emscripten_futex_wake to take in a volatile void* instead of just a void*. Doesn't that cause this to give a build error, if anything calls __wake, because here the cast is still to void*. Should cast to volatile void * then?

Btw, what was the rationale to change to volatile void*?

Not sure. I seem to have resolved my issues building the browser tests, and do indeed see lots of breakage. I imagine this is in there.

Re: rationale, bpowers/musl@56fbaa3 is the musl commit that changed the pthread structs to be volatile.

Ok, let's switch to volatile void* then. This will break all code that uses futexes, but there shouldn't be too many users of these low level objects out there, as everyone is using mutexes mainly.

Ok, I dropped the cast in ff131dc
I also learned that C doesn't much care about CV-qualifiers, which is why this cast worked in the first place.

juj · 2017-01-12T22:46:43Z

system/lib/libc/musl/src/internal/pthread_impl.h

+#define __ATTRP_C11_THREAD ((void*)(uintptr_t)-1)
+
+#if !__EMSCRIPTEN_PTHREADS__
+void __emscripten_init_pthread_stub(void);


Because the pthread_t structure for main thread is now unconditionally needed in all builds, I think we want to have a machinery that initializes the pthread control field in main thread identical to whether we are building with threads or not. Is it possible to reuse the same code and avoid these pthread_stub fields?

Turns out we weren't initializing locale in non-stub versions. Added a browser test to verify that both stub and non-stub versions have nonzero locale fields, and made the initialization consistent here d01f14d

juj · 2017-01-12T22:52:29Z

tests/core/test_simd_float32x4.c

@@ -5,9 +5,25 @@
 #include <math.h>
 #include <assert.h>

+void printFloat(float n) {


I presume the print format changed here somehow, and printf("%f", nan); no longer prints the string "nan"? What does it print now?

bpowers/musl@0f859fc

Now it will include the sign for negative nan, and prints "-nan". asmjs and wasm have different ideas of which nans to use when, so this just normalizes them all the time, because the tests don't really care about which nan exactly.

Do you know what is the root cause of the divergence? Looks ok to have this then, but I'm surprised that this even differs?

I'm not totally sure, my rough-cut guess is that it's due to wasm not being strictly IEEE 754 compliant with respect to NaN values (https://github.com/WebAssembly/design/blob/master/Semantics.md#floating-point-operators and https://github.com/WebAssembly/design/blob/master/FutureFeatures.md#full-ieee-754-2008-conformance), in particular with respect to NaN signs.

juj · 2017-01-12T23:32:50Z

Testing out browser suite with pthreads tests, there's some unresolved symbol errors:

unresolved symbol: __pthread_setcancelstate
unresolved symbol: __pthread_testcancel

juj · 2017-01-13T17:53:09Z

Ran the browser suite on this, some failures. Hopefully these are caused by the above things though that were already known.

ERROR: test_idbstore_sync_worker (test_browser.browser)
ERROR: test_pthread_file_io (test_browser.browser)
ERROR: test_pthread_gcc_spinlock (test_browser.browser)
ERROR: test_pthread_iostream (test_browser.browser)
ERROR: test_pthread_malloc (test_browser.browser)
ERROR: test_pthread_malloc_free (test_browser.browser)
ERROR: test_pthread_mutex (test_browser.browser)
ERROR: test_pthread_nested_spawns (test_browser.browser)
ERROR: test_pthread_printf (test_browser.browser)
ERROR: test_pthread_proxying_in_futex_wait (test_browser.browser)
ERROR: test_pthread_run_on_main_thread (test_browser.browser)
ERROR: test_pthread_run_on_main_thread_flood (test_browser.browser)
ERROR: test_pthread_sbrk (test_browser.browser)
FAIL: test_asmfs_dirent_test_readdir (test_browser.browser)
FAIL: test_asmfs_dirent_test_readdir_empty (test_browser.browser)
FAIL: test_asmfs_fopen_write (test_browser.browser)
FAIL: test_asmfs_hello_file (test_browser.browser)
FAIL: test_asmfs_mkdir_create_unlink_rmdir (test_browser.browser)
FAIL: test_asmfs_read_file_twice (test_browser.browser)
FAIL: test_asmfs_relative_paths (test_browser.browser)
FAIL: test_asmfs_test_fcntl_open (test_browser.browser)
FAIL: test_asmfs_unistd_access (test_browser.browser)
FAIL: test_asmfs_unistd_close (test_browser.browser)
FAIL: test_asmfs_unistd_unlink (test_browser.browser)
FAIL: test_fetch_idb_delete (test_browser.browser)
FAIL: test_fetch_idb_store (test_browser.browser)
FAIL: test_fetch_sync_xhr (test_browser.browser)
FAIL: test_pthread_join (test_browser.browser)

…le_map.c

…stub case

…alization

We previously statically allocated a single locale in the event that we needed to dynamically allocate one during startup. musl changed their implementation to statically allocate C_LOCALE and UTF8_LOCALE, so we already get that behavior. In addition this was causing invalid locales to get allocated instead of returning NULL, because the preceding logic changed. That was caught by other.test_locale_wrong

kripken · 2017-03-13T21:26:25Z

Thanks, and I added tags in the other 2 repos.

Interestingly, I got an email notification about the 2 tags you created. I don't think I've ever seen that before. Did you create them in some other way than git tag ; git push --tags etc? Just curious.

jgravelle-google · 2017-03-13T22:15:12Z

Yeah, I made them through the GitHub interface, which probably has deeper notification hooks? Haven't worked with tags before, didn't think to check the command line.

Thanks for tagging fastcomp&clang by the way!

When musl was updated to version 1.1.5 in #4813 it looks like files that were removed upstream were not removed locally. Because we recursively build some directories this means we were build old files. For example the src/locale/is*.c family of functions were removed from musl in d89fdec51b5849ebdf8000ff1c2fb49878004f39 but remained in our tree after the update. This fixes some strange failures we've been seeing on the CI builders where the name of a symbol alternates between __isxdigit_l (the new name) and isxdigit_l (the old name). This is due to the fact that this symbol was defined in two different object files and filesystem ordering was determining which object was included first by the linker.

When musl was updated to version 1.1.5 in #4813 it looks like files that were removed upstream were not removed locally. Because we recursively build some directories this means we were build old files. For example the src/locale/is*.c family of functions were removed from musl in d89fdec51b5849ebdf8000ff1c2fb49878004f39 but remained in our tree after the update. This fixes some strange failures we've been seeing on the CI builders where the name of a symbol alternates between __isxdigit_l (the new name) and isxdigit_l (the old name). This is due to the fact that this symbol was defined in two different object files and filesystem ordering was determining which object was included first by the linker. This change was automatically generated by checking out musl at v1.1.15 and running: `for f in $(find . -type f); do if [[ ! -e ../../../../../musl/$f ]]; then echo $f; fi; done | grep -v emscripten | xargs git rm`

When musl was last upgraded this malloc line was silently removed.

When musl was last upgraded this malloc line removed from newlocale.c, perhaps by accident. This restores the behaviour of musl which is to allow `newlocale` with arbitrary names to succeed. I verified that this is the case using musl on my desktop.

When musl was last upgraded this malloc line removed from newlocale.c, perhaps by accident. This restores the behaviour of musl which is to allow `newlocale` with arbitrary names to succeed. I verified that this is the case using musl on my desktop. Its not clear to me if/how test_locale_wrong passed prior to #4813 given that this test seems to expect `newlocale` to fail in way that musl does not.

When musl was last upgraded this malloc line removed from newlocale.c, perhaps by accident. This restores the behaviour of musl which is to allow `newlocale` with arbitrary names to succeed. I verified that this is the case using musl on my desktop. The reason that test_locale_wrong was passing prior to #4813 is that the musl older version of musl we were using prior to #4813 rejected all locales except for "C" and "POSIX". Upstream version that we are currently based on: https://github.com/emscripten-core/musl/blob/v1.1.15/src/locale/newlocale.c#L44 Upstream version of musl that we were previously using: https://github.com/emscripten-core/musl/blob/0b44a0315b47dd8eced9f3b7f31580cf14bbfc01/src/locale/newlocale.c#L5

kripken reviewed Dec 21, 2016

View reviewed changes

dschuff reviewed Jan 3, 2017

View reviewed changes

jgravelle-google mentioned this pull request Jan 5, 2017

_uselocale() overwrites data over a variable allocated near 0 #4423

Closed

jgravelle-google mentioned this pull request Jan 12, 2017

Add leading underscore to implemented functions in wasm_backend #4854

Merged

jgravelle-google force-pushed the update_musl branch from 4e01d95 to 05f2c3f Compare January 12, 2017 19:14

juj reviewed Jan 12, 2017

View reviewed changes

jgravelle-google added 14 commits March 13, 2017 09:08

Move pthread_self in stub case to libc C code

52d8026

Move emscripten_pthread initialization to its own file, out from loca…

3079114

…le_map.c

Statically allocate the pthread struct and not just a pointer in the …

9205ce2

…stub case

Remove commented-out test args

fc8907c

Remove misleading __asm annotation for _register_pthread_ptr

6832f20

Do module-side pthread initialization in-module

f85038d

Don't cast away volatile when calling emscripten_futex_wake in __wake

7dcbd45

Expose libc struct to emscripten, use that for non-main pthread initi…

a82a6a1

…alization

Re-add pthread_impl.h import to library_pthread.c

2df5518

Update align_moar alignment post-rebase

828e49c

Update emscripten header include in libc.c

d23d4b6

Update test_llvm_lto

8b47a49

Bump version to 1.37.5

49756ca

jgravelle-google force-pushed the update_musl branch from a701f8e to 49756ca Compare March 13, 2017 16:10

jgravelle-google merged commit 31cf2bb into emscripten-core:incoming Mar 13, 2017

sbc100 mentioned this pull request Sep 23, 2019

Update vswprintf to mussl 1.1.23 #9387

Merged

sbc100 mentioned this pull request Feb 17, 2020

Remove old files from musl source tree #10492

Merged

sbc100 added a commit that referenced this pull request Mar 3, 2021

Restore line in newlocale.c that was lost in #4813

bfd096b

When musl was last upgraded this malloc line was silently removed.

sbc100 mentioned this pull request Mar 3, 2021

Restore line in newlocale.c that was lost in #4813 #13584

Merged

sbc100 mentioned this pull request Sep 13, 2021

Upgrade musl to latest release #15035

Closed

		@@ -86,9 +86,9 @@ typedef long suseconds_t;
		#ifdef __EMSCRIPTEN__
		// For canvas transfer implementation in Emscripten, use an extra 10th control field

Update musl to 1.1.15 #4813

Update musl to 1.1.15 #4813

Uh oh!

Conversation

jgravelle-google commented Dec 20, 2016

Uh oh!

kripken commented Dec 21, 2016

Uh oh!

kripken commented Dec 21, 2016

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!

kripken Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kripken commented Dec 21, 2016

Uh oh!

jgravelle-google commented Dec 21, 2016

Uh oh!

jgravelle-google commented Jan 3, 2017

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!

kripken commented Jan 10, 2017

Uh oh!

dschuff commented Jan 10, 2017

Uh oh!

jgravelle-google commented Jan 12, 2017

Uh oh!

kripken commented Jan 12, 2017

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!

kripken Dec 22, 2016 •

edited

Loading