Page MenuHomePhabricator

[libc++abi] Use fallback_malloc to allocate __cxa_eh_globals in case of dynamic memory exhaustion.
ClosedPublic

Authored by ikudrin on Mar 2 2016, 9:07 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin updated this revision to Diff 49628.Mar 2 2016, 9:07 AM
ikudrin retitled this revision from to [libc++abi] Use fallback_malloc to allocate __cxa_eh_globals in case of dynamic memory exhaustion..
ikudrin updated this object.
ikudrin added a subscriber: cfe-commits.
asl added a subscriber: asl.Mar 9 2016, 10:17 AM
bcraig added a subscriber: bcraig.Mar 14 2016, 8:52 AM

LGTM. I don't know if that's good enough for a submit though :)

In my opinion, requiring heap allocation during an exception is one of the worst parts of the Itanium ABI. At least this particular path isn't triggered for platforms with true thread_local support.

EricWF edited edge metadata.Mar 15 2016, 3:49 PM

By including fallback_malloc.ipp in a new C++ file we now have 2 different OOM emergancy buffers. I think it would be preferable to use only one. This would require a fairly large restructuring of fallback_malloc.ipp though.

Thanks! I'll see what I can do about it.

ikudrin updated this revision to Diff 51782.Mar 28 2016, 6:09 AM
ikudrin updated this object.
ikudrin edited edge metadata.

Make cxa_exception.cpp and cxa_exception_storage.cpp to use the same emergency memory pool.

Ping! Could anyone check this patch, please?

Could you generate a diff against the normal directory layout? Your's is against "libcxxabi/trunk/" where 'trunk' is the actual libcxxabi directory.

ikudrin updated this revision to Diff 54151.Apr 18 2016, 7:15 PM

Regenerated the patch against project's root.

It's almost there.

Please move "fallback_malloc.ipp" into "fallback_malloc.cpp" and then delete it all together. We can't have other files trying to include it.

src/fallback_malloc.cpp
23 ↗(On Diff #54151)

I think we should have the pragma GCC visibility push(hidden) in this file as well, but I'm not 100% sure.

test/test_exception_storage_nodynmem.pass.cpp
13 ↗(On Diff #54151)

Let's check that we actually replace calloc here and assert that our replacement has been called at the end of main.

16 ↗(On Diff #54151)

Let's perform this test a couple of times so we're testing

  • The memory get's returned properly
  • The returned memory can be reused.
ikudrin updated this revision to Diff 54189.Apr 19 2016, 8:10 AM
  • Moved the content of fallback_malloc.ipp into falback_malloc.cpp.
  • Removed fallback_malloc.ipp.
  • Added pragma GCC visibility push(hidden) for the function's definitions.
  • Added a check that the overwritten calloc is called to the test.
  • Added a loop to run the test several times.

Please note that test_fallback_malloc.pass.cpp now has to #include fallback_malloc.cpp, because fallback_malloc.ipp was removed. I'd considered some other options, including declaring additional functions especially for testing purpose and even moving all fallback_malloc functionality into a separate class which could be tested independently. However, eventually, I found them more difficult and even much worse than the presented approach.

Ping...

This patch is under review for more than two months, and I believe I've addressed all the comments. Could someone take a look at it, please?

Hi,

Ping.

The issue in the current implementation of the libc++abi library looks like a time bomb. Even though its probability could be considered as very low, in fact, it depends on the type of an application. At least, we ran into it in our environment.

I'd guess that no one would expect their program to terminate when a legal exception is thrown, or when memory is allocated. I'd like this issue to be fixed in one or another way.

@EricWF, can you look at the patch again, please?

Hi,

Ping.

The patch is under review for a long time and the described problem is still here. As we've seen the issue in practice, I'm sure that others may also run into it, and I do believe it should be fixed, one way or another.

@EricWF, can I improve the patch somehow to pass the review? Or maybe you prefer it to be fixed in some other way?

EricWF accepted this revision.Sep 26 2016, 2:01 PM
EricWF edited edge metadata.

LGTM after addressing the inline comments.

src/fallback_malloc.cpp
58 ↗(On Diff #54189)

A static const variable would be better here.

59 ↗(On Diff #54189)

This should have __attribute__((aligned)) on it.

src/fallback_malloc.h
20 ↗(On Diff #54189)

Please don't use the __cxa prefix on these functions. It makes it seem like they are part of the Itanium spec but they're really internal details.

This revision is now accepted and ready to land.Sep 26 2016, 2:01 PM
This revision was automatically updated to reflect the committed changes.

@ikudrin: Looks like you've reverted this soon after. I'm just about to commit D24864, which will affect this slightly. D24864 basically refactors all pthread dependencies behind a separate API. It would be pretty straightforward for you to update this patch though, just replacing pthread calls with ones in thread_support.h (perhaps adding anything missing).

Hope you don't mind me going first? If you are going to commit this soon, I can hold off D24864. Let me know.

Cheers,

/ Asiri

ikudrin reopened this revision.Sep 30 2016, 4:54 AM

@rmaprath: There are some issues in the test which upset build bots and I need some time to fix them. You don't need to wait for me, I'll put my patch on the top of yours when my fix is ready.

This revision is now accepted and ready to land.Sep 30 2016, 4:54 AM
This revision was automatically updated to reflect the committed changes.