<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 25 May 2016, at 14:50, Filipe Cabecinhas <<a href="mailto:filcab+llvm.phabricator@gmail.com" class="">filcab+llvm.phabricator@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On Mon, May 23, 2016 at 4:48 PM, Kuba Brecka <</span><a href="mailto:kuba.brecka@gmail.com" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">kuba.brecka@gmail.com</a><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">> wrote:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">kubabrecka updated this revision to Diff 58101.<br class="">kubabrecka added a comment.<br class="">Herald added a subscriber: kubabrecka.<br class=""><br class="">Adding a unit test. Changing GetPageSizeCached into an inlined function.<br class=""><br class=""><br class=""><a href="http://reviews.llvm.org/D14656" class="">http://reviews.llvm.org/D14656</a><br class=""><br class="">Files:<br class=""> lib/sanitizer_common/sanitizer_common.cc<br class=""> lib/sanitizer_common/sanitizer_common.h<br class=""> lib/sanitizer_common/sanitizer_stacktrace.cc<br class=""> lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc<br class=""> lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc<br class=""><br class="">Index: lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc<br class="">===================================================================<br class="">--- lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc<br class="">+++ lib/sanitizer_common/tests/sanitizer_stacktrace_test.cc<br class="">@@ -136,6 +136,19 @@<br class=""> EXPECT_EQ(PC(1), trace.trace[1]);<br class="">}<br class=""><br class="">+TEST_F(FastUnwindTest, CloseToZeroFrame) {<br class="">+ // Make one pc a NULL pointer.<br class="">+ fake_stack[5] = 0x0;<br class="">+ if (!TryFastUnwind(kStackTraceMax))<br class="">+ return;<br class="">+ // The stack should be truncated at the NULL pointer (and not include it).<br class="">+ EXPECT_EQ(3U, trace.size);<br class="">+ EXPECT_EQ(start_pc, trace.trace[0]);<br class="">+ for (uptr i = 1; i < 3U; i++) {<br class="">+ EXPECT_EQ(PC(i*2 - 1), trace.trace[i]);<br class="">+ }<br class="">+}<br class="">+<br class="">TEST(SlowUnwindTest, ShortStackTrace) {<br class=""> if (StackTrace::WillUseFastUnwind(false))<br class=""> return;<br class="">Index: lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc<br class="">===================================================================<br class="">--- lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc<br class="">+++ lib/sanitizer_common/sanitizer_unwind_linux_libcdep.cc<br class="">@@ -108,6 +108,8 @@<br class=""> UnwindTraceArg *arg = (UnwindTraceArg*)param;<br class=""> CHECK_LT(arg->stack->size, arg->max_depth);<br class=""> uptr pc = Unwind_GetIP(ctx);<br class="">+ const uptr kPageSize = GetPageSizeCached();<br class="">+ if (pc < kPageSize) return UNWIND_STOP;<br class=""> arg->stack->trace_buffer[arg->stack->size++] = pc;<br class=""> if (arg->stack->size == arg->max_depth) return UNWIND_STOP;<br class=""> return UNWIND_CONTINUE;<br class="">Index: lib/sanitizer_common/sanitizer_stacktrace.cc<br class="">===================================================================<br class="">--- lib/sanitizer_common/sanitizer_stacktrace.cc<br class="">+++ lib/sanitizer_common/sanitizer_stacktrace.cc<br class="">@@ -66,6 +66,7 @@<br class=""><br class="">void BufferedStackTrace::FastUnwindStack(uptr pc, uptr bp, uptr stack_top,<br class=""> uptr stack_bottom, u32 max_depth) {<br class="">+ const uptr kPageSize = GetPageSizeCached();<br class=""> CHECK_GE(max_depth, 2);<br class=""> trace_buffer[0] = pc;<br class=""> size = 1;<br class="">@@ -92,6 +93,8 @@<br class="">#else<br class=""> uhwptr pc1 = frame[1];<br class="">#endif<br class="">+ if (pc1 < kPageSize)<br class="">+ break;<br class=""> if (pc1 != pc) {<br class=""> trace_buffer[size++] = (uptr) pc1;<br class=""> }<br class="">Index: lib/sanitizer_common/sanitizer_common.h<br class="">===================================================================<br class="">--- lib/sanitizer_common/sanitizer_common.h<br class="">+++ lib/sanitizer_common/sanitizer_common.h<br class="">@@ -63,7 +63,12 @@<br class="">}<br class=""><br class="">uptr GetPageSize();<br class="">-uptr GetPageSizeCached();<br class="">+extern uptr PageSizeCached;<br class="">+INLINE uptr GetPageSizeCached() {<br class="">+ if (!PageSizeCached)<br class="">+ PageSizeCached = GetPageSize();<br class="">+ return PageSizeCached;<br class="">+}<br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">This will race and trigger UB.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Would it be possible to get the page size only on one or two places</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">when initializing?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">That way, we'd only need an atomic load, and nothing else. We'd miss a</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">few places, where we could end up unwinding the stack, and</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">PageSizeCached is still 0, but those should be minor edge cases, no?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>This doesn’t currently race, because the first call to this function happens when only one thread exists. This behavior already exists and I’m not trying to change it in this patch. I know this guarantee is weird and subtle, but I don’t think this is related to what I’m trying to achieve in this patch.</div><div><br class=""></div><div>Kuba</div><div><br class=""></div></div></body></html>