[PATCH] D66904: [libunwind] Fix memory leak in handling of DW_CFA_remember_state and DW_CFA_restore_state

[Jorge Gorbe Moya via Phabricator via llvm-commits]

jgorbe added inline comments.


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:1
+# REQUIRES: linux
+# RUN: %build
----------------
compnerd wrote:
> Can you add a `REQUIRES: x86` as well please?  This shouldnt be executed on non-x86 platforms.  I think that explicitly listing the triple is a good idea in the build rules (you could default to x86 builds which wont work with x86_64).  I think that we *could* add a requirement on `ASAN` as well, but, that isn't as big of a concern.
I agree that this shouldn't run on non-x86. Thanks for catching this. That said, shouldn't it be `REQUIRES: x86_64` or similar? The asm code below uses 64-bit registers.

Anyway, I just tried both `REQUIRES: x86` and `REQUIRES: x86_64`, and when I run the tests with `ninja check-unwind` on my x86_64 machine the test doesn't run and it's counted within "Unsupported Tests" in the final summary instead.

What do you mean by listing the triple in the build rules? I can't find any example of it in the existing tests and my knowledge of the testing infrastructure has a lot of gaps :(


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:39
+.cfi_def_cfa_register %rbp
+subq $48, %rsp
+.cfi_remember_state
----------------
compnerd wrote:
> Mind extracting this value out into a macro (`SIZEOF_UNWIND_EXCEPTION`) so others can easily identify where the value comes from?
sizeof(_Unwind_Exception) seems to be 32 instead. I didn't build the original C++ file with optimizations, so there was probably a frame pointer in there too.

I have rebuilt the example with -O2 and now it's subtracting 40 from rsp, which I believe is equal to 8 bytes to align the stack to a 16-byte boundary plus 32 for the exception object. Would you prefer to have two `sub` instructions instead, one that adjusts the stack and another one that reserves 32 additional bytes using a `SIZEOF_UNWIND_EXCEPTION` macro? (and then two matching `add` instructions in the epilog?)


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:49
+movl %eax, -36(%rbp)         # 4-byte Spill
+movl %ecx, %eax
+addq $48, %rsp
----------------
compnerd wrote:
> I'm not sure I understand the need for the spill.  Did you form this from `-O0`?  I imagine that DSE would remove these three instructions.
I didn't specify any optimization options, so I guess it was `-O0` by default. I have rebuilt it with `-O2` now, which indeed produces more concise code here. Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66904/new/

https://reviews.llvm.org/D66904