[PATCH] Change how we deal with (implicit) -fno-rtti + -fsanitize=vptr

[Filipe Cabecinhas]

Hi Alexey,

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.

 Filipe

On Thu, Feb 12, 2015 at 10:20 AM, Alexey Samsonov <vonosmas at gmail.com>
wrote:

> Hi,
>
> On Wed, Feb 11, 2015 at 8:10 PM, Filipe Cabecinhas <
> filcab+llvm.phabricator at gmail.com> wrote:
>
>> Hi Alexey,
>>
>> On Wed, Feb 11, 2015 at 6:59 PM, Alexey Samsonov <vonosmas at gmail.com>
>> wrote:
>>
>>> 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:
>>> a) we're on PS4
>>> b) we have a CXX input
>>> c) we've provided -fexceptions, but haven't provided -frtti
>>> In this case RTTI will still be implicitly enabled, but we would disable
>>> vptr sanitizer earlier at this point.
>>>
>> I'm fine with allowing this (it's not perfect, but it makes it much
>> easier to deal with this edge case).
>>
>> 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).
>>>
>> Unfortunately, we can't change that behavior. -fexceptions in C++ mode
>> _has_ to turn on rtti, even without an explicit -frtti.
>>
>
> I see, you probably already have a bunch of build rules that depend on
> that.
>
>
>> 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.
>>>
>> 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):
>>
>>   - 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).
>>
>
> 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".
>
>
>>
>>   - 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).
>>
>> I'm not really fond of these, but I think the second one wouldn't be
>> _too_ bad.
>>
>> What do you think?
>>
>>   Filipe
>>
>>
>> Does it all make sense?
>>>
>>>
>>> On Wed, Feb 11, 2015 at 6:08 PM, Filipe Cabecinhas <
>>> filcab+llvm.phabricator at gmail.com> wrote:
>>>
>>>> This is getting a bit hard to reason about.
>>>>
>>>> Here are the pieces:
>>>>
>>>> - Default RTTI behavior for target (depends on other arguments like the
>>>> type of input file)
>>>> - RTTI related options (explicit)
>>>> - Sanitizer options (explicit (vptr) or implicit (undefined))
>>>>
>>>> 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) .
>>>>
>>>> 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).
>>>>
>>>> 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)).
>>>>
>>>> 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()).
>>>>
>>>> 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).
>>>>
>>>> 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.
>>>>
>>>>
>>>> http://reviews.llvm.org/D7525
>>>>
>>>> EMAIL PREFERENCES
>>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> Alexey Samsonov
>>> vonosmas at gmail.com
>>>
>>
>>
>
>
> --
> Alexey Samsonov
> vonosmas at gmail.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150212/d9a79eff/attachment.html>