<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 10, 2014 at 11:50 AM, Alexey Samsonov <span dir="ltr"><<a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Sep 9, 2014 at 4:39 AM, Jakub Jelinek <span dir="ltr"><<a href="mailto:jakub@redhat.com" target="_blank">jakub@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Mon, Sep 08, 2014 at 01:32:02PM -0700, Alexey Samsonov wrote:<br>
> OK. I've submitted the patches to Clang and UBSan runtime which implement<br>
> -fsanitize=nonnull-attribute and -fsanitize=returns-nonnull-attribute (the<br>
> last<br>
> commit is r217400). I've added source locations of attribute declarations to<br>
> the static data passed in UBSan handlers to print them in the error<br>
> reports: it makes<br>
> sense to actually show user the declaration which forbids passing/returning<br>
> null pointer.<br>
> This location is also added to returns-nonull attribute, because the<br>
> attribute might be<br>
> declared far from the actual function definition with incorrect<br>
> return-statement.<br>
<br>
</span>Thanks.  BTW, the recent changes to move Die() call out of the<br>
__ubsan_*_abort handlers look wrong to me, the functions are no longer<br>
noreturn if for whatever reason isDisabled would return true on the<br>
location, but the compiler rightfully assumes those functions never return.<br>
If you want to avoid dying without emitting messages, you could e.g. ignore<br>
isDisabled if abort is set in the opts.<br></blockquote><div><br></div></div></div><div>Ah, it's a great point. This one is tricky. Currently, "Loc.isDisabled()" is used only</div><div>for report deduplication so that we don't report the same error twice.</div><div><br></div><div>If we're building the code with fatal UBSan handlers (with -fno-sanitize-recover), then</div><div>the first error we report won't have disabled source location and we will crash the</div><div>program immediately after reporting it. There's still a possibility for undefined behavior, though:</div><div>1) Thread 1 enters __ubsan_handle_foo(), acquires the source location and starts to format error report</div><div>2) Thread 2 enters the same __ubsan_handle_foo(), sees that the source location is disabled, and exits</div><div>    from a noreturn function, causing an undefined behavior.</div><div><br></div><div>So, looks like we should indeed skip "Loc.isDisabled()" check in _abort mode - in the case described above</div><div>we will just spin on the report mutex in Thread 2.<br></div><div><br></div><div>That said, I dislike all these _abort functions - it would be cleaner to control UBSan behavior with a runtime flag,</div><div>but probably we have to live with it, as I guess NORETURN helps a lot in terms of code size and runtime overhead.</div></div></div></div></blockquote><div><br></div><div>r217542 adds "noreturn" attribute to fatal handlers declarations and adds back "Die()" calls to the end of them.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- <br></div></font></span></div><span class="HOEnZb"><font color="#888888"><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</font></span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div>
</div></div>