<div dir="ltr">Thanks for the quick reply, Vedant!<div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 19, 2018 at 7:01 PM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div><div><span class="gmail-"><blockquote type="cite"><div>On Mar 19, 2018, at 3:44 PM, Brian Gesiak <<a href="mailto:modocache@gmail.com" target="_blank">modocache@gmail.com</a>> wrote:</div><br class="gmail-m_-5599426050432439376Apple-interchange-newline"><div><div dir="ltr">Hello all!<div>(+cc Vedant Kumar, who I've been told knows a lot about UBSan!)<br></div><div><br></div><div>I am trying to fix an assert that occurs when the transforms in llvm/lib/Transforms/Coroutines are applied to LLVM IR that has been generated with UBSan enabled -- specifically, '-fsanitize=null'.</div><div><br></div><div>You can see an example of the assert in this 26-line C++ file here: <a href="https://godbolt.org/g/Gw9UZq" target="_blank">https://godbolt.org/g/Gw9UZq</a></div><div><br></div><div>Note that without the '-fsanitize=null' option this compiles fine, but when that option is used, Clang/LLVM crashes due to "error in backend: cannot move instruction since its users are not dominated by CoroBegin".</div><div><br></div><div>The coroutine pass coro-split is responsible for transforming instructions within a coroutine function. Because a coroutine function can be suspended and then resumed much later in a program's execution, accesses to stack variables much be rewritten so that they access variables stored on the "coroutine frame," a heap-allocated piece of state that represents the execution frame of the coroutine. As part of this process, load instructions, such as the one generated for the '*this' expression in the source file above, are moved down past a call to the '@llvm.coro.begin' intrinsic.</div><div><br></div><div>However, when UBSan is enabled, the load of '*this' is then immediately followed by a null check and, in the null case, a conditional branch to a call to '@ __ubsan_handle_type_mismatch_<wbr>v1'. The coro-split pass is not written to move these compare and branch instructions, and instead asserts.</div></div></div></blockquote><div><br></div></span><span>It looks like there was a FIXME about this issue introduced circa r280678.<br></span></div></div></div></blockquote><div><br></div><div>Yup! Perhaps the time has come, as the comment suggests, to "make this more robust" :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><span class="gmail-"><blockquote type="cite"><div><div dir="ltr"><div>You can see an example of the IR generated with and without '-fsanitize=null' here: <a href="https://gist.github.com/modocache/54a036c3bf9c06882fe85122e105d153" target="_blank">https://gist.github.com/<wbr>modocache/<wbr>54a036c3bf9c06882fe85122e105d1<wbr>53</a> -- PR36578.ll lines 82 to 104 show the problematic UBSan branches. Note that PR36578.nosan.ll, which was compiled without '-fsanitize=null', does not include these branches.</div><div><br></div><div>I have several questions:</div><div><br></div><div>* What's the best way for the LLVM coroutines transform passes to play nicely with the code generated with UBSan's '-fsanitize=null'?</div></div></div></blockquote><div><br></div></span>+ Gor, as he might be the best person to answer this.</div><div><br></div><div><span class="gmail-"><br><blockquote type="cite"><div><div dir="ltr"><div>* Are there other LLVM passes I could look at, to see examples of how they were modified to handle UBSan?</div></div></div></blockquote><div><br></div></span><div>No, no other llvm passes needed to be modified to handle ubsan instrumentation.</div><span class="gmail-"><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div>* I've seen code in Clang's codebase that adds 'SanitizerKind::Null' to a set of skipped UBSan checks; should lib/CodeGen/CGCoroutine.cpp also do something like this, to avoid generating UBSan checks altogether? Would that be sufficient to avoid the error?</div></div></div></blockquote><div><br></div></span><div>I don't think it would be sufficient. You might encounter the same issue with -fsanitize=alignment,object-<wbr>size,vptr and possibly -fsanitize=address. Instead of blacklisting known bad sanitizers, it might be better to whitelist known good ones. Over time you could expand that set, adding tests as needed.</div><span class="gmail-"><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div> Are there downsides to doing this?</div></div></div></blockquote><div><br></div></span><div>I think a whitelisting approach is a reasonable way to prevent crashes. It at least allows you to enable sanitizers for coroutines later on. The alternative is to force users to partially disable sanitization (either with source annotations or by dropping the build configuration entirely). If that happens, it's much less likely that they'd re-enable it in the future.</div></div></div></div></blockquote><div><br></div><div>Excellent, thanks for these suggestions! I think I'll try the whitelisting approach you suggested. I'm guessing I can accomplish this by using the 'clang::SanitizerBlacklist' class or 'clang::SanitizerSet' struct somehow...? 'clang::SanitizerSet::clear', in particular, looks promising. I'll look into it and see what I can do. Thanks!</div><div><br></div><div>- Brian</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div><span class="gmail-HOEnZb"><font color="#888888"><div><br></div><div>vedant</div></font></span><span class="gmail-"><br><blockquote type="cite"><div><div dir="ltr"><div><br></div><div>Any and all advice is appreciated -- thanks!</div><div><br></div><div>- Brian Gesiak</div><div><br></div></div>
</div></blockquote></span></div><br></div></div></blockquote></div><br></div></div>