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.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 821–822 | 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. | |
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 821–822 | 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. | |
Comment Actions
This looks good to me FWIW. I'd prefer if someone more experienced with the project would accept it, though.
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 821–822 | 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 :) | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
Thanks for the review. It looks like I still need someone from the libunwind group to accept it.
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 137 | typo? should be RememberStack | |
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 137 | Thanks. | |
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 551 | 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. | |
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 551 | Good point, I made it configurable with some default config. | |
Comment Actions
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 | ||
|---|---|---|
| 551 |
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 ↗ | (On Diff #294925) | 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 ↗ | (On Diff #294925) | _WIN64 is redundant here; on 64 bit windows, _WIN32 is still defined as well. |
Comment Actions
Could you add a corresponding CMake option for setting _LIBUNWIND_REMEMBER_HEAP_ALLOC? I don't think it's common to having to enable macros manually when it should just be a configuration option.
| libunwind/src/DwarfParser.hpp | ||
|---|---|---|
| 15 | IIUC -- we should include <alloca.h> to get the declaration in cases where alloca is invoked below. | |
Comment Actions
Please include stdlib.h for alloca, unless you target SunOS. alloca.h is unavailable on NetBSD.
IIUC -- we should include <alloca.h> to get the declaration in cases where alloca is invoked below.