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 <[email protected]> wrote: > Hi, > > On Wed, Feb 11, 2015 at 8:10 PM, Filipe Cabecinhas < > [email protected]> wrote: > >> Hi Alexey, >> >> On Wed, Feb 11, 2015 at 6:59 PM, Alexey Samsonov <[email protected]> >> 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 < >>> [email protected]> 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 >>> [email protected] >>> >> >> > > > -- > Alexey Samsonov > [email protected] >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
