<div dir="ltr">HI Matthew,<div><br></div><div>I think you may be correct. It makes sense that an unwinder would need to visit the frame that called _Unwind_Resume again when unwinding. And when it does, there will be no landing pad associated with the _Unwind_Resume call, so the unwinder will be called with _URC_CONTINUE_UNWIND and the stack frame will be cleared. I verified that with your change to the LLVM pass and this change to the hwasan test suite:</div><div><br></div><div><div>diff --git a/compiler-rt/test/hwasan/TestCases/try-catch.cpp b/compiler-rt/test/hwasan/TestCases/try-catch.cpp<br>index 8e35a9dd2a5..4d186bf17a6 100644<br>--- a/compiler-rt/test/hwasan/TestCases/try-catch.cpp<br>+++ b/compiler-rt/test/hwasan/TestCases/try-catch.cpp<br>@@ -23,6 +23,11 @@ void h() {<br> <br> __attribute__((noinline))<br> void g() {<br>+ // Make sure that we need to resume when unwinding past this function.<br>+ struct S {<br>+ ~S() { optimization_barrier((void *)this); }<br>+ } s;<br>+<br> char x[1000];<br> optimization_barrier(x);<br> h();<br></div><div></div></div><div><br></div><div>the hwasan tests continue to pass on Android (with both the libgcc unwinder and LLVM libunwind).</div><div><br></div><div>There are a couple of things that I can think of that could make things go wrong here:</div><div>(1) The function's landing pad adjusts SP such that it no longer covers the local variables before calling _Unwind_Resume. This would mean that some local variables won't get their tags cleared.</div><div>(2) The landing pad tail calls _Unwind_Resume (which would probably require doing (1) first). That would mean that the unwinder wouldn't see the _Unwind_Resume caller's stack frame at all.</div><div><br></div><div>I'm not sure why a compiler would do (1) since it would seem to result in larger code and more unwind info. And I've been unable to provoke the compiler into tail calling _Unwind_Resume either. Tim, do you know of any circumstances where the AArch64 backend would do either of these things? If not, I think we might consider going ahead with your proposed change.</div><div><br></div><div>Peter</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 12, 2019 at 6:06 AM Matthew Malcomson via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
When looking into the way that hwasan handles C++ exceptions I found<br>
myself wondering about the need for landing pad cleanups (as documented<br>
in the code<br>
<a href="https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/hwasan/hwasan_exceptions.cpp#L51" rel="noreferrer" target="_blank">https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/hwasan/hwasan_exceptions.cpp#L51</a><br>
).<br>
<br>
The comment above that code says:<br>
// We only untag frames without a landing pad because landing pads are<br>
// responsible for untagging the stack themselves if they resume.<br>
<br>
I think the code as implemented untags all frames as it goes past<br>
whether or not they have a landing pad, and am hoping people can check<br>
or correct my understanding.<br>
<br>
It seems that personality routine will eventually return<br>
_URC_CONTINUE_UNWIND if this stack frame is to be unwound past (and<br>
hence the frame will eventually get untagged). If the frame has landing<br>
pads then that just means the personality routine will be called<br>
multiple times for this frame before returning that value.<br>
<br>
<br>
I took a look by testing code generated with a clang patched to avoid<br>
adding instrumentation to landing pads and it seems that the personality<br>
wrapper does indeed clear stack frames with a landing pad in the basic<br>
case.<br>
<br>
<br>
Is there an edge case where the personality wrapper is known to not be<br>
sufficient? If not would removing that extra instrumentation sound<br>
reasonable?<br>
<br>
<br>
The test I ran was to generate code with a clang patched with the diff<br>
below, and run it in a debugger.<br>
I'm attaching the annotated gdb session to demonstrate my reasoning. I<br>
checked that the shadow memory is not untagged in a landing pad and that<br>
after _Unwind_Resume the personality wrapper is run again, eventually<br>
returning _URC_CONTINUE_UNWIND and untagging the shadow memory.<br>
<br>
<br>
diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp<br>
b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp<br>
index df7606d..ca97d72 100644<br>
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp<br>
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp<br>
@@ -1110,8 +1110,7 @@ bool HWAddressSanitizer::sanitizeFunction(Function<br>
&F) {<br>
continue;<br>
}<br>
<br>
- if (isa<ReturnInst>(Inst) || isa<ResumeInst>(Inst) ||<br>
- isa<CleanupReturnInst>(Inst))<br>
+ if (isa<ReturnInst>(Inst) || isa<CleanupReturnInst>(Inst))<br>
RetVec.push_back(&INST);<br>
<br>
if (auto *DDI = dyn_cast<DbgDeclareInst>(&Inst))<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>