[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
libcxx-commits at lists.llvm.org
Wed Sep 25 10:10:31 PDT 2019
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
+subq $48, %rsp
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
More information about the libcxx-commits