<div dir="ltr">Hi Alexey,<div><br></div><div>Thanks for helping with this. I was over-complicating a lot. I managed to clean it up. I'll finish the patch and send it tonight or tomorrow.</div><div class="gmail_extra"><br></div><div class="gmail_extra"> Filipe</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 12, 2015 at 10:20 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">Hi,</div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Feb 11, 2015 at 8:10 PM, Filipe Cabecinhas <span dir="ltr"><<a href="mailto:filcab+llvm.phabricator@gmail.com" target="_blank">filcab+llvm.phabricator@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">Hi Alexey,<div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Feb 11, 2015 at 6:59 PM, 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">Note that currently types::IsCXX(InputType) is not used to determine -fsanitize flag values at all - we just look at the explicit flags (-frtti, -fno-rtti, -fapple-kext, -mkernel), and at the Triple (is it PS4?). Does it make sense to leave it that way? There is a special case:<div>a) we're on PS4</div><div>b) we have a CXX input</div><div>c) we've provided -fexceptions, but haven't provided -frtti</div><div>In this case RTTI will still be implicitly enabled, but we would disable vptr sanitizer earlier at this point.</div></div></blockquote></span><div>I'm fine with allowing this (it's not perfect, but it makes it much easier to deal with this edge case).</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I'd say it's fine to sacrifice this special case :) (for that matter, to simplify things we could even make PS4 driver to issue a hard error if one is building CXX code with explicit -fexceptions but no explicit -frtti).<br></div></div></blockquote></span><div>Unfortunately, we can't change that behavior. -fexceptions in C++ mode _has_ to turn on rtti, even without an explicit -frtti.</div></div></div></div></blockquote><div><br></div></span><div>I see, you probably already have a bunch of build rules that depend on that.</div><span class=""><div><br></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><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>That is, if we only look at Triple and input arguments, we can probably have ToolChain::getRTTIMode() much like we have ToolChain::getSanitizerArgs(), and use former in the latter.</div></div></blockquote></span><div>My problem is: If we make a getRTTIMode() that only looks at the triple and args, it will have to, either (taking as an example, c++ mode + -fexceptions):</div><div><br></div><div>  - Not be true all the time (It would return “disabled”, but then, when we notice that we have IsCXX() and -fexceptions, it would be wrong, because RTTI would then be turned on, which would make that value wrong for all the calls after that).</div></div></div></div></blockquote><div><br></div></span><div>I don't see anything except vptr-sanitizer that should depend on the true information of whether we have RTTI enabled (that is, currently all the calculations you do inside Clang::ConstructJob are used to solely decide if we need to provide -fno-rtti flag, and are not reused afterwards). Maybe, we can get away with getRTTIMode(), and then, if we're on PS4 + CXX input + -fexceptions, we'll just say "no, we will override it and enable RTTI anyway".</div><span class=""><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"><div><br></div><div>  - Have to have its value cached in ToolChain (or elsewhere), and have a way to directly manipulate that cache when we finally figure out we want RTTI enabled (by calling some method to tell the cache that we actually want RTTI).</div><div><br></div><div>I'm not really fond of these, but I think the second one wouldn't be _too_ bad.</div><div><br></div><div>What do you think?</div><span><font color="#888888"><div><br></div><div>  Filipe</div></font></span><span><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>Does it all make sense?</div><div><br></div></div><div class="gmail_extra"><div><div><br><div class="gmail_quote">On Wed, Feb 11, 2015 at 6:08 PM, Filipe Cabecinhas <span dir="ltr"><<a href="mailto:filcab+llvm.phabricator@gmail.com" target="_blank">filcab+llvm.phabricator@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is getting a bit hard to reason about.<br>
<br>
Here are the pieces:<br>
<br>
- Default RTTI behavior for target (depends on other arguments like the type of input file)<br>
- RTTI related options (explicit)<br>
- Sanitizer options (explicit (vptr) or implicit (undefined))<br>
<br>
SanitizerArgs _wants_ to hide details like “which sanitizers exist/are enabled”, which means it will have to sort out whether to warn/error or not by itself (it needs to know if there's an explicit RTTI-related argument) .<br>
<br>
The RTTI-related code needs to know if C++ exceptions are on to decide if it implicitly turns on RTTI or not (this implies knowing not just the arguments, but also the input file).<br>
<br>
To decide whether we need additional args for RTTI or not, and which ones to pass, we need to know the RTTI-related arguments passed and the input type (actually, we need IsCXX(Input)).<br>
<br>
Easiest (kind of hacky) solution: Add enough arguments to getSanitizerArgs and to the SanitizerArgs constructor to allow it to decide whether to warn/error due to the vptr sanitizer (We'd parse the RTTI-related args, and use those results (enabled/disabledexplicitly/disabledimplicitly + an RTTI-related arg, if necessary) when calling getSanitizerArgs()).<br>
<br>
Easy (kind of hacky) solution (with a major drawback): Make it easy to, via SanitizerArgs, add/remove sanitizers. It would be hard to know if the vptr sanitizer was enabled explicitly or via -fsanitize=undefined, with this solution (we'd have to re-parse everything, if we wanted to keep it working that way).<br>
<br>
Non-hacky solutions: Sorry, couldn't think of one. Additional ones included stashing stuff in the ToolChain object… It doesn't seem like it's the proper thing to do.<br>
<div><div><br>
<br>
<a href="http://reviews.llvm.org/D7525" target="_blank">http://reviews.llvm.org/D7525</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div></div></div><span><font color="#888888">-- <br><div><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</font></span></div>
</blockquote></span></div><br></div></div>
</blockquote></span></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br><div><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</font></span></div></div>
</blockquote></div><br></div></div>