Store exceptions metadata in wasm memory (#11503). #11518

vagran · 2020-06-29T13:06:29Z

This patch is accepted in our private fork we use in our project and it seems to be working fine.
(I have switched to my private account).

welcome · 2020-06-29T13:06:31Z

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

sbc100 · 2020-06-29T19:56:41Z

Can you explain why this useful/better as part of the PR descrption? Also include Fixes: #XXXX referencing the bug where this was discussed.

vagran · 2020-06-29T20:42:15Z

This fixes #11503.
In short, there is a design failure - exceptions cannot be passed between threads, all their metadata were stored in thread-private storage, this fix moves them into wasm memory so that the exceptions now are not thread-local.

kripken · 2020-06-29T23:58:52Z

In general I think this is a good direction!

First step is to get it passing the test suite. I see a bunch of existing failures, many of them seem to be minor things like unresolved symbols that can be easily fixed. The bigger issue is that as I think I mentioned on the other issue, the adjustment/deadjustment is needed for virtual inheritance. There are tests for that so after you fix other things eventually I think you'll end up with a broken test that shows an example of the code that needs that adjustment stuff.

src/library_exceptions.js

vagran · 2020-06-30T07:26:03Z

src/library_exceptions.js

    // A rethrown exception can reach refcount 0; it must not be discarded
    // Its next handler will clear the rethrown flag and addRef it, prior to
    // final decRef and destruction here
-    if (info.refcount === 0 && !info.rethrown) {
-      if (info.destructor) {
+    if (info.release_ref() && !info.rethrown) {


oops.
< info.rethrown

info.get_rethrown()

vagran · 2020-06-30T07:27:24Z

OK, I will try to sort out tests a bit later.

vagran · 2020-07-01T08:43:23Z

src/library_exceptions.js

@@ -265,52 +309,53 @@ var LibraryExceptions = {
  // functionality boils down to picking a suitable 'catch' block.
  // We'll do that here, instead, to keep things simpler.

-  __cxa_find_matching_catch__deps: ['__exception_last', '__exception_infos', '__resumeException'],
+  __cxa_find_matching_catch__deps: ['__exception_last', 'ExceptionInfo', '__resumeException'],


Found that this declarations actually had no effect because addCxaCatch() function below did not handle it.

vagran · 2020-07-01T10:08:23Z

I have reworked exceptions handling to (hopefully) proper way. New entity introduced - CatchInfo which is used as catching context and returned by __cxa_find_matching_catch().
Unfortunately this code still cannot be considered thread-safe because single exception object can be simultaneously thrown in several threads and its state (except reference counter) is not protected from that. Also protection is not enough, separate state should be allocated. libcxxabi has concept of dependent exception which is used for that purpose, it references the primary exception. Now I do not feel enough inspiration to implement it, may be it can be done in related #11233 issue.

vagran · 2020-07-01T14:49:43Z

Now several tests are failed and seems to be unrelated. No idea how to proceed.

kripken · 2020-07-01T18:24:17Z

Looking at the first of the errors, I see

building:ERROR: Closure compiler run failed:
 
building:ERROR: /tmp/tmp62xozmnv/emscripten_temp_gpkl6bql/a.out.bc.o.js.pp.js.mem.js.jso.js:42: ERROR - [JSC_UNDEFINED_VARIABLE] variable ___cxa_is_pointer_type is undeclared

1 error(s), 0 warning(s)

Investigating it, it looks like a bug in fastcomp. While the tests run with exceptions disabled, some JS exceptions code is included somehow, and it wants cxa_is_pointer_type but that hasn't been compiled in (since exceptions are off).

We'll be removing fastcomp soon, so not sure it makes sense to debug this much. As a workaround, you can ifdef out that code,

    this.get_exception_ptr = function() {
#if !WASM_BACKEND && DISABLE_EXCEPTION_CATCHING != 1 // work around a fastcomp bug
      var isPointer = {{{ exportedAsmFunc('___cxa_is_pointer_type') }}}
        (this.get_exception_info().get_type());
      if (isPointer) {
        return {{{ makeGetValue('this.get_base_ptr()', '0', '*') }}};
      }
      var adjusted = this.get_adjusted_ptr();
      if (adjusted !== 0) return adjusted;
      return this.get_base_ptr();
#else
      abort("no exceptions support");
#endif
    }

vagran · 2020-07-01T20:18:49Z

#if !WASM_BACKEND && DISABLE_EXCEPTION_CATCHING != 1 // work around a fastcomp bug

Have you meant #if WASM_BACKEND && DISABLE_EXCEPTION_CATCHING != 1? Otherwise it aborts in regular clang wasm build.

kripken · 2020-07-01T20:43:32Z

Hmm, no, I think that code was right? We want to emit that logic if fastcomp (so "not wasm backend") and if exceptions are enabled ("so "disabled != 1).

What error do you see?

kripken · 2020-07-01T20:44:45Z

Oh, wait, yeah, that's not right... it should be an or with flipped backend.

So

#if WASM_BACKEND || DISABLE_EXCEPTION_CATCHING != 1 // work around a fastcomp bug

vagran · 2020-07-01T20:50:37Z

Could not it be that CatchInfo should be added to library_exceptions_stub.js? Is that stub file completely replaces library_exceptions.js or it may take missing functions from the non-stub library?

(function from exception library is taken in build without exception support).

kripken

Nice work!

First set of review comments. I don't see anything worrying, but I didn't read all the details in each function yet, still trying to properly understand the big picture.

kripken · 2020-07-01T23:34:14Z

src/library_exceptions.js

+    RETHROWN_OFFSET: Runtime.POINTER_SIZE * 2 + 5,
+
+    // Should be multiple of allocation alignment.
+    SIZE: alignMemory(Runtime.POINTER_SIZE * 2 + 6, 16)


16 is the default alignment, so the last parameter is not needed.

kripken · 2020-07-01T23:34:58Z

src/library_exceptions.js

+  __ExceptionInfoAttrs: {
+    TYPE_OFFSET: 0,
+    DESTRUCTOR_OFFSET: Runtime.POINTER_SIZE,
+    REFCOUNT_OFFSET: Runtime.POINTER_SIZE * 2,


Does the refcount take 8 bytes? Based on the next field's offset it seems like it is, but based on the operations on it it seems otherwise.

Refcount offset is Runtime.POINTER_SIZE * 2, next field offset is Runtime.POINTER_SIZE * 2 + 4 so it takes 4 bytes. The only question is type field which takes POINTER_SIZE I do not remember why, but probably because of alignment (pointers indeed should be aligned on pointer size). I will change fields order, it is common practice to sort them in descending order by size to minimize paddings.

kripken · 2020-07-01T23:36:23Z

src/library_exceptions.js

-#if EXCEPTION_DEBUG
-          err('de-adjusted exception ptr ' + adjusted + ' to ' + ptr);
+
+  __ExceptionInfoAttrs: {


please add a comment here explaining what an ExceptionInfo is in general, and maybe some notes on where it's allocated and freed, etc. - the big picture of ExceptionInfo and CatchInfo and their relationship would be good to describe.

kripken · 2020-07-01T23:38:35Z

src/library_exceptions.js

+    }
+
+    this.set_refcount = function(refcount) {
+      {{{ makeSetValue('this.ptr', '___ExceptionInfoAttrs.REFCOUNT_OFFSET', 'refcount', '*') }}};


I think this isn't a pointer, so it should be 'i32' for the type?

kripken · 2020-07-01T23:39:04Z

src/library_exceptions.js

+
+    this.set_type = function(type) {
+      {{{ makeSetValue('this.ptr', '___ExceptionInfoAttrs.TYPE_OFFSET', 'type', '*') }}};
+    }


please add ; after these assignments.

kripken · 2020-07-01T23:43:00Z

src/library_exceptions.js

+      {{{ makeSetValue('this.ptr', '___ExceptionInfoAttrs.REFCOUNT_OFFSET', 'prev - 1', 'i32') }}};
+#endif
+      if (prev === 0) {
+        err('Exception reference counter underflow');


these 3 lines can be like they were before,

#if ASSERTIONS assert(prev > 0); #endif

In general we shouldn't emit logging code in a normal build. In a build with assertions we can assert on things, and if we want logging, that should be behind EXCEPTION_DEBUG.

kripken · 2020-07-01T23:47:20Z

src/library_exceptions.js

+    this.get_exception_ptr = function() {
+#if WASM_BACKEND || DISABLE_EXCEPTION_CATCHING != 1 // work around a fastcomp bug
+      var isPointer = {{{ exportedAsmFunc('___cxa_is_pointer_type') }}}
+        (this.get_exception_info().get_type());


please put the ( on the previous line. (I'm actually unsure what JS ASI does on something like this, but it worries me...)

kripken · 2020-07-01T23:50:53Z

src/library_exceptions.js

-      rethrown: false
-    };
+    var info = new _ExceptionInfo(ptr);
+    info.init(type, destructor);


It's not clear to me why init is called here but not in other places. Can we add the two parameters to the constructor? Or if there is a fundamental reason, perhaps init needs a more meaningful name or comment.

Here it is first created and initialized, all the other places just wrap the pointer to previously created ExceptionInfo instance. Will add the parameters to the constructor.

Or may be leave it as is with some comment? Otherwise it will look like new _ExceptionInfo(ptr, type, destructor); thus relying purely on constructor side effects which does not look nice.

Hmm, interesting.

How about the constructor has no side effects, and after it we can either create(..) (instead of init(..)) to create a new underlying instance, or wrap(..) to wrap an existing one? That seems like it would be clear.

kripken · 2020-07-01T23:51:28Z

src/library_exceptions.js

 #if WASM_BACKEND == 0
  , 'setThrew'
 #endif
  ],
  __cxa_end_catch: function() {
    // Clear state flag.
    _setThrew(0);
+    if (!___exception_caught.length) {
+      err("Exceptions stack underflow");


(same as earlier - please make this an assertion)

src/library_exceptions.js

kripken · 2020-07-01T23:58:09Z

Could not it be that CatchInfo should be added to library_exceptions_stub.js? Is that stub file completely replaces library_exceptions.js or it may take missing functions from the non-stub library?

Either library_exceptions.js is included, or library_exceptions_stub.js (and not both).

I don't think this PR needs to change anything with the stubs one, though, unless I'm missing something?

kripken · 2020-07-28T23:35:23Z

Hi @vagran , is there something I can do to help move this forward? Please let me know if some of the last comments were not clear.

vagran · 2020-07-29T07:04:27Z

Hi, sorry, I had several weeks vacation. Now I checked it, and seems the only thing left is this one:

How about the constructor has no side effects, and after it we can either create(..) (instead of init(..)) to create a new underlying instance, or wrap(..) to wrap an existing one? That seems like it would be clear.

I tried to make factory methods in ExceptionInfo class (static ones) but failed because of code transformations applied when the js is compiled, it discards all the code which is not the function body itself. As I understand, your proposal is to have something like this: new _ExceptionInfo().wrap(ptr) and new _ExceptionInfo().create(ptr) however I do not think it is better than current approach.

kripken · 2020-07-29T22:58:59Z

I see, thanks @vagran - It could be you're hitting some bug in the JS preprocessor. Anyhow, let's leave that for later refactoring perhaps, I agree the current approach is good enough as a starting point.

Please merge in latest master to resolve the conflicts.

…s merge from master).

vagran · 2020-07-30T07:12:13Z

I have merged the master. There was some names refactoring there, so I have discarded changes in this file in the merge, and made same names refactoring in a separate commit to not mess the things up.

kripken · 2020-08-03T16:47:12Z

Reading through __cxa_increment_exception_refcount. In the old code, that does a deadjust, then adds a reference. I don't see a deadjustment in the new code there? Was that never needed? (If so, it would be better to do that in a PR that lands before this one, to separate the issues, that way if a problem is discovered later, bisection can be more helpful as it would show that small PR instead of this larger one.)

And looking at the deadjustment code itself, the old code had an array of adjustments, info.adjusted which it pushes to in __cxa_find_matching_catch. In the new code, __cxa_find_matching_catch calls set_adjusted_ptr which has just a single value, not an array? Is that because the deadjusted pointer is now in the new CatchInfo?

vagran · 2020-08-04T14:33:12Z

Reading through __cxa_increment_exception_refcount. In the old code, that does a deadjust, then adds a reference. I don't see a deadjustment in the new code there? Was that never needed? (If so, it would be better to do that in a PR that lands before this one, to separate the issues, that way if a problem is discovered later, bisection can be more helpful as it would show that small PR instead of this larger one.)

The old code messed thrown object pointer and catch context, both were represented by exception pointer. When these entities are separated, increment and decrement methods are never called for adjusted pointer. As I remember, I checked this approach with libcxxabi implementation.

And looking at the deadjustment code itself, the old code had an array of adjustments, info.adjusted which it pushes to in __cxa_find_matching_catch. In the new code, __cxa_find_matching_catch calls set_adjusted_ptr which has just a single value, not an array? Is that because the deadjusted pointer is now in the new CatchInfo?

Yes, the pointer can be adjusted only once per catch context.

kripken · 2020-08-05T20:32:51Z

I think I see. Then it probably doesn't make sense to split this up, as it's tied together with the splitting up of the two things (thrown pointers and catch contexts), IIUC. Re-reading the old code, yes, that was definitely mixed up and incorrect.

kripken

Ok, this looks good I think, thanks @vagran !

I wish we had more tests for this, but we do have quite a bit, so I'm not very worried. Also I just verified this happens to fix #11776 which was filed recently, which is a very good sign.

The code style is not 100% consistent with the rest of our code, but we can improve that separately, and also this is something we hope to rewrite eventually in C probably.

I don't think this has any interactions with the new wasm backend exceptions support, but just to make sure, ping @aheejin before landing.

aheejin · 2020-08-06T08:38:32Z

All files changed here are not used for wasm exception handling, so I don't think it will interact with it.

kripken · 2020-08-06T16:28:09Z

Thanks @aheejin , landing.

Store exceptions metadata in wasm memory (#11503).

7f58595

vagran mentioned this pull request Jun 29, 2020

C++ exceptions are not properly shared between threads #11503

Closed

sbc100 reviewed Jun 30, 2020

View reviewed changes

src/library_exceptions.js Show resolved Hide resolved

vagran commented Jun 30, 2020

View reviewed changes

vagran added 2 commits June 30, 2020 10:48

Fix missing symbol ref and bug in field access.

e042e22

Fix single threaded EH.

f99318e

vagran commented Jul 1, 2020

View reviewed changes

vagran added 2 commits July 1, 2020 12:54

Properly handle adjusted exception pointer.

9a1e1e4

Merge branch 'master' into shared_exceptions_11503

6942d44

vagran added 2 commits July 1, 2020 15:45

Fix some EH bugs.

bfab6ed

Fix thrown pointer handling.

98c120b

Workaround for fastcomp probable bug

7b4396e

(function from exception library is taken in build without exception support).

kripken reviewed Jul 1, 2020

View reviewed changes

Fix review findings.

c72590d

vagran added 3 commits July 30, 2020 08:00

Drop some unnecessary dependencies.

5277e58

Merge branch 'master' into shared_exceptions_11503

b0435e6

Names refactoring re-done (was intentionally discarded during previou…

fb474b7

…s merge from master).

kripken mentioned this pull request Aug 5, 2020

std::uncaught_exception() is true after the exception has been caught #11776

Closed

kripken approved these changes Aug 5, 2020

View reviewed changes

kripken merged commit 7f41d43 into emscripten-core:master Aug 6, 2020

vagran deleted the shared_exceptions_11503 branch August 6, 2020 17:16

This was referenced Aug 24, 2020

Possible change in exception behaviour after #11518? #12025

Closed

Wasm: upgrade emscripten for an llvm and exception fixes. dotnet/corert#8281

Merged

This was referenced Aug 25, 2020

is exception handling thread safe? #11233

Closed

Exception Handling - exception on the wrong thread when using pthreads #12035

Closed

Store exceptions metadata in wasm memory (#11503). #11518

Store exceptions metadata in wasm memory (#11503). #11518

Uh oh!

Conversation

vagran commented Jun 29, 2020

Uh oh!

welcome bot commented Jun 29, 2020

Uh oh!

sbc100 commented Jun 29, 2020

Uh oh!

vagran commented Jun 29, 2020

Uh oh!

kripken commented Jun 29, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagran commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagran commented Jul 1, 2020

Uh oh!

vagran commented Jul 1, 2020

Uh oh!

kripken commented Jul 1, 2020

Uh oh!

vagran commented Jul 1, 2020

Uh oh!

kripken commented Jul 1, 2020

Uh oh!

kripken commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vagran commented Jul 1, 2020

Uh oh!

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

kripken Jul 6, 2020 • 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!

Uh oh!

kripken commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Jul 28, 2020

Uh oh!

vagran commented Jul 29, 2020

Uh oh!

kripken commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vagran commented Jul 30, 2020

Uh oh!

kripken commented Jul 1, 2020 •

edited

Loading

kripken Jul 6, 2020 •

edited

Loading

kripken commented Jul 1, 2020 •

edited

Loading

kripken commented Jul 29, 2020 •

edited

Loading