Skip to content

[EH] Tidy up format_exception.cpp (NFC) - #16995

Merged
aheejin merged 1 commit into
emscripten-core:mainfrom
aheejin:format_exception_tidy
May 20, 2022
Merged

[EH] Tidy up format_exception.cpp (NFC)#16995
aheejin merged 1 commit into
emscripten-core:mainfrom
aheejin:format_exception_tidy

Conversation

@aheejin

@aheejin aheejin commented May 20, 2022

Copy link
Copy Markdown
Member

This makes small cosmetic changes that do not affect the functionality.

  • Use using namespace __cxxabiv1. This namespace is used many times
    and I think it's OK to put this at the top. Other libc++abi cpp files
    also do this.
  • Use static_cast instead of the old cast. This is also in line with
    the rest of libc++abi.
  • Change some variable names. This tries to make variable names in line
    with libc++abi's other files; personally I don't particularly like
    their long names such as exception_header, but I think keeping them
    in line with the rest of the code is generally good and possibly
    helpful when we upstream the changes.
  • Add cxa_exception_from_thrown_object and use it. The reason I copied
    this function from cxa_exception.cpp is I'm planning to use all these
    three when I add Wasm EH support for this function, and it will be
    easier to read if we use these than just doing the arthmetic
    ourselves.
    // Utility routines
    static
    inline
    __cxa_exception*
    cxa_exception_from_thrown_object(void* thrown_object)
    {
    return static_cast<__cxa_exception*>(thrown_object) - 1;
    }
    // Note: This is never called when exception_header is masquerading as a
    // __cxa_dependent_exception.
    static
    inline
    void*
    thrown_object_from_cxa_exception(__cxa_exception* exception_header)
    {
    return static_cast<void*>(exception_header + 1);
    }
    // Get the exception object from the unwind pointer.
    // Relies on the structure layout, where the unwind pointer is right in
    // front of the user's exception object
    static
    inline
    __cxa_exception*
    cxa_exception_from_exception_unwind_exception(_Unwind_Exception* unwind_exception)
    {
    return cxa_exception_from_thrown_object(unwind_exception + 1 );
    }

    The reason I copied them is they are static functions in
    cxa_exception.cpp. We possibly can move them to a header file and
    include it both places, but that means further changes to libc++abi
    codebase, which I think will make this harder to upstream.
  • Use __shim_type_info::can_catch directly instead of __cxa_catch,
    which is a wrapper created to be called from JS. This removes the need
    to include the declaration of __cxa_catch in this file.

This makes small cosmetic changes that do not affect the functionality.
- Use `using namespace __cxxabiv1`. This namespace is used many times
  and I think it's OK to put this at the top. Other libc++abi cpp files
  also do this.
- Use `static_cast` instead of the old cast. This is also in line with
  the rest of libc++abi.
- Change some variable names. This tries to make variable names in line
  with libc++abi's other files; personally I don't particularly like
  their long names such as `exception_header`, but I think keeping them
  in line with the rest of the code is generally good and possibly be
  helpful when we upstream the changes.
- Add `cxa_exception_from_thrown_object` and use it. The reason I copied
  this function from cxa_exception.cpp is I'm planning to use all these
  three when I add Wasm EH support for this function, and it will be
  easier to read if we use these than just doing the arthmetic
  ourselves.
  https://github.com/emscripten-core/emscripten/blob/d5ef6937fe395488e23a82c1e582a7ea5c2dab83/system/lib/libcxxabi/src/cxa_exception.cpp#L39-L67
  The reason I copied them is they are static functions in
  cxa_exception.cpp. We possibly can move them to a header file and
  include it both places, but that means further changes to libc++abi
  codebase, which I think will make this harder to upstream.
- Use `__shim_type_info::can_catch` directly instead of `__cxa_catch`,
  which is a wrapper created to be called from JS. This removes the need
  to include the declaration of `__cxa_catch` in this file.
@aheejin
aheejin requested a review from sbc100 May 20, 2022 05:30
@aheejin

aheejin commented May 20, 2022

Copy link
Copy Markdown
Member Author

cc @hoodmane

static inline __cxa_exception*
cxa_exception_from_thrown_object(void* thrown_object) {
return static_cast<__cxa_exception*>(thrown_object) - 1;
}

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.

I have another change that also adds this function: #16627

But I don't mind merging them when I land that.

@aheejin
aheejin merged commit 2b45f62 into emscripten-core:main May 20, 2022
@aheejin
aheejin deleted the format_exception_tidy branch May 20, 2022 17:32
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