Skip to content

[EH] Rename format_exception.cpp - #17045

Merged
aheejin merged 2 commits into
emscripten-core:mainfrom
aheejin:format_filename
May 25, 2022
Merged

[EH] Rename format_exception.cpp#17045
aheejin merged 2 commits into
emscripten-core:mainfrom
aheejin:format_filename

Conversation

@aheejin

@aheejin aheejin commented May 24, 2022

Copy link
Copy Markdown
Member

I think we need a separate file in libc++abi for 'our' EH, meaning both
Emscripten EH and Wasm EH. (I think they can be in the same file, as
long as we have ifdefs) This also adds a file header.

I think this method can be in that file, rather than having a separate
file for this. I named this cxa_emscripten.cpp because it's the name
#16627 used and I hope we can put all Emscripten specific stuff there,
but maybe @sbc100 had another thought; if so let me know.

I think we need a separate file in libc++abi for 'our' EH, meaning both
Emscripten EH and Wasm EH. (I think they can be in the same file, as
long as we have `ifdef`s) This also adds a file header.

I think this method can be in that file, rather than having a separate
file for this. I named this `cxa_emscripten.cpp` because it's the name
 emscripten-core#16627 used and I hope we can put all Emscripten specific stuff there,
but maybe @sbc100 had another thought; if so let me know.
@aheejin
aheejin requested a review from sbc100 May 24, 2022 22:05
@aheejin

aheejin commented May 24, 2022

Copy link
Copy Markdown
Member Author

cc @hoodmane

@hoodmane

Copy link
Copy Markdown
Collaborator

By the way, I tried at some point to work out how to implement similar features with wasm exception handling but I got stuck -- in particular, exception.getArg(tag, 0) returns a number but I was unable to figure out how to interpret that number. When you do implement this for wasm exception handling I'm curious to see what the code looks like.

Comment thread tools/system_libs.py Outdated
'stdlib_typeinfo.cpp',
'private_typeinfo.cpp',
'format_exception.cpp',
'cxa_emscripten.cpp',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about cxa_exception_emscripten.cpp?

Also I noticed that the error message was change to terminating with uncaught exception.. where as before it could be used to get a description of an exception in general.

It seems like it might be useful to be able to decribe and exception without it being the an uncaught one. So we might need two different functions here.. one can call the other?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but is there a way to access Wasm exceptions from JS side unless they are uncaught exceptions? Hmm, but yeah, you're right, someone might want to catch them from JS and not actually terminate the program.. Will fix that in another PR.

Renamed the file to cxa_exception_emscripten.cpp.

@aheejin

aheejin commented May 24, 2022

Copy link
Copy Markdown
Member Author

@hoodmane

By the way, I tried at some point to work out how to implement similar features with wasm exception handling but I got stuck -- in particular, exception.getArg(tag, 0) returns a number but I was unable to figure out how to interpret that number. When you do implement this for wasm exception handling I'm curious to see what the code looks like.

Sorry for making you wait for so long. My next PR, which I will upload probably today or tomorrow, will address this. The reason you didn't get the right value is in libc++abi what you throw is not really the object pointer itself but __cxa_exception->unwindHeader:

_Unwind_RaiseException(&exception_header->unwindHeader);

@aheejin

aheejin commented May 25, 2022

Copy link
Copy Markdown
Member Author

Can we land this?

@hoodmane

Copy link
Copy Markdown
Collaborator

Sorry for making you wait for so long

No worries, we are not using wasm-exceptions anyways -- Safari support is too recent.

@aheejin
aheejin merged commit b2a0a83 into emscripten-core:main May 25, 2022
@aheejin
aheejin deleted the format_filename branch May 25, 2022 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants