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

[Saleem Abdulrasool via Phabricator via libcxx-commits]

compnerd added inline comments.
Herald added a reviewer: mclow.lists.


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:1
+# REQUIRES: linux
+# RUN: %build
----------------
jgorbe wrote:
> 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 :(
Nope, it should be `x86`.  The backend is called x86 and supports (kinda) 16-bit, 32-bit and 64-bit code.

As to the triple, what I mean is that we should add a `-target x86_64-unknown-linux-gnu` to the build invocation (you should be able to add that after `%build`)


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:39
+.cfi_def_cfa_register %rbp
+subq $48, %rsp
+.cfi_remember_state
----------------
jgorbe wrote:
> 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?)
I think that the readability of the test is more important, so lets go with the two subs.


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

https://reviews.llvm.org/D66904