This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Refactor exception type demangling into a separate function
ClosedPublic

Authored by ldionne on May 9 2022, 2:15 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rG6089fd6c0b1c: [libc++abi] Refactor exception type demangling into a separate function
Summary

As a fly-by fix, also let __cxa_demangle allocate its buffer alone,
since we are not allowed to pass a non-malloc'd buffer to it.

Diff Detail

Event Timeline

ldionne created this revision.May 9 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 2:15 PM
ldionne requested review of this revision.May 9 2022, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 2:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
urnathan added inline comments.
libcxxabi/src/cxa_default_handlers.cpp
61–62

This isn't code you're changing the meaning of, but this is broken. the buffer passed to __cxa_demangle must come from malloc. If ever a thrown type demangles to more than 1024 chars, people are going to be sad.

urnathan added inline comments.May 10 2022, 7:07 AM
libcxxabi/src/cxa_default_handlers.cpp
31–33
if (auto *res = ...)
  return res;
else
  return str;

? No need to pass in status -- we don't care why it can't demangle.

ldionne updated this revision to Diff 428366.May 10 2022, 7:14 AM
ldionne marked an inline comment as done.
ldionne retitled this revision from [libc++abi][NFC] Refactor exception type demangling into a separate function to [libc++abi] Refactor exception type demangling into a separate function.
ldionne edited the summary of this revision. (Show Details)

Address review comment.

libcxxabi/src/cxa_default_handlers.cpp
61–62

Thank you, that is an excellent observation.

ldionne updated this revision to Diff 428369.EditedMay 10 2022, 7:17 AM
ldionne marked an inline comment as done.

Address additional review comment.

libcxxabi/src/cxa_default_handlers.cpp
31–33

I updated before seeing this comment, but yes, I agree, I can drop status as well.

ldionne updated this revision to Diff 428466.May 10 2022, 12:32 PM

Fix CI with exceptions disabled.

urnathan added inline comments.May 11 2022, 6:09 AM
libcxxabi/src/cxa_default_handlers.cpp
61–62

thanks, but you're now going to leak. you need something like:

if (name != thrown_type->name())
  std::free (name);

somewhere. (I'm going to presume the optimizer can spot that's always false in the LIBCXXABI_NON_DEMANGLING_TERMINATE case).

N.B. our demangler calls std::terminate in the event of memory exhaustion, rather than return null. I'm slowly trying to resolve that problem.

ldionne updated this revision to Diff 428656.May 11 2022, 7:13 AM
ldionne marked an inline comment as done.

Never leak!

libcxxabi/src/cxa_default_handlers.cpp
61–62

I know we're leaking, but I was thinking it didn't matter because we were terminating anyways. This is actually what we were doing previously if the demangled name was more than 1024 characters long. __cxa_demangle would realloc and the resulting pointer would be leaked (assuming realloc doesn't try to free if the given buffer wasn't allocated with malloc).

But hey, I can fix this easily with unique_ptr anyways.

urnathan added inline comments.May 11 2022, 8:13 AM
libcxxabi/src/cxa_default_handlers.cpp
61–62

oh, good point. maybe a comment about leak don't care?

ldionne marked an inline comment as done.May 12 2022, 7:21 AM
ldionne added inline comments.
libcxxabi/src/cxa_default_handlers.cpp
61–62

There was a comment before above the demangle() function, but I removed it since I'm not leaking anymore (I use std::unique_ptr). Please LMK if that looks OK to you.

urnathan added inline comments.May 12 2022, 8:37 AM
libcxxabi/src/cxa_default_handlers.cpp
61–62

This is fine. Sorry for not noticing that leaking doesn't matter and causing unnecessary rework. I hope the rework is not bloaty.

ldionne accepted this revision as: Restricted Project.May 12 2022, 10:16 AM
ldionne marked an inline comment as done.

Looks like @urnathan is OK with the patch now (thanks for the review!).

The AIX CI has been failing due to unrelated reasons, so ignoring that.

This revision was not accepted when it landed; it landed in state Needs Review.May 12 2022, 10:17 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.