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

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

I think that using assembly for the test is fine



================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:1
+# REQUIRES: linux
+# RUN: %build
----------------
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.


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:39
+.cfi_def_cfa_register %rbp
+subq $48, %rsp
+.cfi_remember_state
----------------
Mind extracting this value out into a macro (`SIZEOF_UNWIND_EXCEPTION`) so others can easily identify where the value comes from?


================
Comment at: libunwind/test/remember_state_leak.pass.sh.s:49
+movl %eax, -36(%rbp)         # 4-byte Spill
+movl %ecx, %eax
+addq $48, %rsp
----------------
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.


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

https://reviews.llvm.org/D66904