Page MenuHomePhabricator

[libunwind] Support DW_CFA_remember/restore_state without heap allocation.
AcceptedPublic

Authored by danielkiss on Jul 30 2020, 11:42 PM.

Details

Reviewers
rprichard
jgorbe
compnerd
lhames
phosek
mstorsjo
steven_wu
Group Reviewers
Restricted Project
Summary

This patch just reorganises the code to make possible to use alloca
instead of malloc. This makes possible to use .cfi_remember_state/.cfi_restore_state on
platforms without heap allocation.
Also it will be safe to backtrace/unwind faults related to the allocator behind malloc.
_LIBUNWIND_REMEMBER_HEAP_ALLOC option reenables the heap usage for .cfi_remember_state/.cfi_restore_state.
Define _LIBUNWIND_REMEMBER_STACK_ALLOC to force stack allocation.

Diff Detail

Event Timeline

danielkiss created this revision.Jul 30 2020, 11:42 PM
danielkiss requested review of this revision.Jul 30 2020, 11:42 PM

The change seems alright to me.

jgorbe added inline comments.Aug 6 2020, 5:52 PM
libunwind/src/DwarfParser.hpp
816–817

This construction with an unbounded loop where we continue after we set up another item, then break at the end, is a bit awkward. What do you think about putting p, instructionsEnd and pcoffset in a struct and doing something like this? (modulo whatever's allowed by llvm style, which I'm not very familiar with)

struct ParseInfo {
  pint_t instructions;
  pint_t instructionsEnd;
  pint_t pcoffset;
};
ParseInfo cie = {cieInfo.cieInstructions, cieInfo.cieStart + cieInfo.cieLength, (pint_t)(-1)};
ParseInfo fde = {fdeInfo.fdeInstructions, fdeInfo.fdeStart + fdeInfo.fdeLength, upToPC - fdeInfo.pcStart};

for (const auto& info : {cie, fde}) {
  // loop body goes here, possibly unpacking info into the existing loop variables
  // to avoid doing "info.whatever" everywhere, I don't know
}

IMO it would be more readable, as the different args to what used to be parseInstructions are together at the top (instead of one before the loop body and one after) and it makes it completely clear that the loop will always do two passes, first CIE then FDE.

danielkiss marked an inline comment as done.Aug 10 2020, 1:02 PM
danielkiss added inline comments.
libunwind/src/DwarfParser.hpp
816–817

Thanks for the great tip, I implemented it a bit differently due to here the initializer_list (stl at all) is not available but luckily we don't need it.

This looks good to me FWIW. I'd prefer if someone more experienced with the project would accept it, though.

libunwind/src/DwarfParser.hpp
816–817

Thanks! It might be interesting to have a comment near the definition of ParseInfo explaining that the point of doing it this way is to be able to use alloca instead of malloc, in case any future reader wonders if the loop body wouldn't be better factored into a separate function :)

danielkiss marked an inline comment as done.
danielkiss marked an inline comment as done.

And now uploading the new patch.

danielkiss added a reviewer: Restricted Project.Aug 27 2020, 4:33 AM

seems fine. The only worry is that if you have a stack that is close to the stack limit and the unwinder might just blow the stack away so it might have bincompat concerns.

It might be good to keep the option to use malloc so we can pick which one to use.

danielkiss edited the summary of this revision. (Show Details)

adding an option _LIBUNWIND_REMEMBER_HEAP to store the state on the heap.
I kept the default on the stack because it should be fine for most cases.

LGTM. Thanks.

Thanks for the review. It looks like I still need someone from the libunwind group to accept it.

rprichard added inline comments.Sep 24 2020, 2:49 PM
libunwind/src/DwarfParser.hpp
138

typo? should be RememberStack

danielkiss marked an inline comment as done.
danielkiss added inline comments.
libunwind/src/DwarfParser.hpp
138

Thanks.

compnerd added inline comments.Sep 28 2020, 4:02 PM
libunwind/src/DwarfParser.hpp
553

I think that a check to ensure that alloca is available should made. Not all targets (e.g. Windows) provide alloca since it is a potential security issue. Having an additional condition for _LIBUNWIND_REMEMBER_STACK_ALLOC would be reasonable.

danielkiss marked an inline comment as done.
danielkiss edited the summary of this revision. (Show Details)
danielkiss marked an inline comment as done.Sep 29 2020, 3:32 AM
danielkiss added inline comments.
libunwind/src/DwarfParser.hpp
553

Good point, I made it configurable with some default config.

danielkiss marked an inline comment as done.Mon, Oct 19, 7:53 AM

ping

The change in itself looks good to me (and @steven_wu seems to be ok with it as well), so if nobody else want to comment further on it (@compnerd?), I could approve it.

libunwind/src/DwarfParser.hpp
553

I think that a check to ensure that alloca is available should made. Not all targets (e.g. Windows) provide alloca since it is a potential security issue.

Just as a note - MSVC does provide it, both under the name alloca (which also can be disabled as they consider it breaking the C standards, and would rather have you use it under the underscored name) and _alloca, but the _malloca variant used here is meant to be safer. _malloca does a malloc instead of a stack alloc if the allocation is bigger than a certain threshold (less than 1024 bytes).

For this particular case, it doesn't really add any extra safety, because we allocate one single struct of sizeof(PrologInfoStackEntry) each time - so even if each individual alloc is less than 1024 bytes (I didn't check how big it actually is) we could still run out of stack space if iterating many times.

But in any case, I don't mind using _malloca on platforms that provide it; the option for calling malloc would end up optimized out if the allocation is known to be smaller than the threshold.

libunwind/src/config.h
127

FWIW, plain alloca do exist on windows platforms in mingw environments, so one could add defined(__MINGW32__) here, but the other windows specific cases below also works fine.

131

_WIN64 is redundant here; on 64 bit windows, _WIN32 is still defined as well.

rebase, add MINGW32

danielkiss marked 2 inline comments as done.Mon, Oct 26, 3:01 PM
This revision is now accepted and ready to land.Fri, Oct 30, 2:45 AM