This is an archive of the discontinued LLVM Phabricator instance.

Change how we deal with (implicit) -fno-rtti + -fsanitize=vptr
ClosedPublic

Authored by filcab on Feb 9 2015, 7:07 PM.

Details

Summary

We were trying to get cc1 to disable the vptr sanitizer, but cc1 only
looks at -fsanitize=, not at -fno-sanitize=

I'm changing that behavior (which is only relied on by us, AFAICT) to:
If we're on an “implicit -fno-rtti” platform and the vptr sanitizer is
enabled, warn that -frtti is being turned on for this compilation, and
turn it on.

I also refactored common tests and added -check-prefix=(NO-)?RTTI to some
RUN lines.

Diff Detail

Event Timeline

filcab updated this revision to Diff 19637.Feb 9 2015, 7:07 PM
filcab retitled this revision from to Change how we deal with (implicit) -fno-rtti + -fsanitize=vptr.
filcab updated this object.
filcab edited the test plan for this revision. (Show Details)
filcab added reviewers: chandlerc, echristo.
filcab added a subscriber: Unknown Object (MLST).
rsmith added a subscriber: rsmith.Feb 9 2015, 7:38 PM

I don't think this is the right approach; the -fsanitize= setting deliberately doesn't affect whether RTTI is enabled, and I don't think we should change that. I think we should do the following:

  • If -fsanitize=vptr is explicitly specified and RTTI is disabled, we should issue an error.
  • If -fsanitize=vptr is implied by some sanitizer group (such as -fsanitize=undefined) and RTTI is disabled, we should not enable the vptr sanitizer (perhaps with a warning).

That presumably means handling this when we parse the sanitizer arguments, which might require us to reorder the processing of the arguments a bit.

I don't think this is the right approach; the -fsanitize= setting deliberately doesn't affect whether RTTI is enabled, and I don't think we should change that. I think we should do the following:

  • If -fsanitize=vptr is explicitly specified and RTTI is disabled, we should issue an error.
  • If -fsanitize=vptr is implied by some sanitizer group (such as -fsanitize=undefined) and RTTI is disabled, we should not enable the vptr sanitizer (perhaps with a warning).

That presumably means handling this when we parse the sanitizer arguments, which might require us to reorder the processing of the arguments a bit.

Thanks for the comment.

What would your preferred way of getting this to work, though?

  • Have a (public) way to turn off specific sanitizers in SanitizerArgs, before the call to addArgs (and warn in the same place we had the warning)
  • Have the constructor of SanitizerArgs (or addArgs) deal with it (doesn't seem very nice, especially since we'll have to repeat all this RTTI-related logic)
  • Another way?

Thanks,

Filipe

samsonov added a subscriber: samsonov.

I agree with Richard here.

As for the implementation, I think we need to delete the code in Clang::ConstructJob and decide whether we should enable/disable -fsanitize=vptr when we parse sanitizer arguments. This means we should factor out the function, that would tell us, if RTTI is (a) enabled, (b) disabled explicitly by certain argument (e.g. -fno-rtti, -mkernel, ...), (c) disabled implicitly (e.g. if we're targeting PS4). Then this function can be used both in Clang::ConstructJob and in SanitizerArgs constructor. Probably it can be a method of ToolChain, though I'm not sure here.

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.

samsonov edited edge metadata.Feb 11 2015, 6:59 PM

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'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).
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.

Does it all make sense?

filcab updated this revision to Diff 19935.Feb 13 2015, 3:02 PM
filcab edited edge metadata.

Changed it to retain current behavior (never enable rtti just because of a sanitizer), but fix the failing case (we were assuming -cc1 would care about -fno-sanitize=).

Refactored the code so the rtti/no-rtti decision was made in one place, and the other code that cares just asks the ToolChain.

samsonov added inline comments.Feb 13 2015, 3:56 PM
include/clang/Driver/ToolChain.h
25

Why do you need this?

58

This value is never used.

85

looks like this should be either const, or mutable and calculated lazily.

lib/Driver/SanitizerArgs.cpp
204

RTTI can be disabled by a different argument.

206

Please don't: we generally try to emit as many errors as possible. Also, this loop iterates over arguments in the reverse order: if we have
clang -fno-rtti -fsanitize=foobarbaz -fsanitize=vptr

It would be weird to fail reporting unrecognized "foobarbaz".

227

I'm inclined to silently disable -fsanitize=vptr if it was enabled implicitly by -fsanitize=undefined.

lib/Driver/ToolChain.cpp
33

This doesn't seem right. You will return RM_DisabledExplicitly for "clang -fno-rtti -frtti a.cc" (which probably means you need a test for that case).

42

You need to parse these arguments only if you're on PS4. Otherwise you can return RM_Enabled straight away.

lib/Driver/Tools.cpp
1975

Looks like you can factor out function, that would take "Args" and return if exceptions are enabled explicitly, and if yes, which argument enables them.

1991

Wouldn't RM_EnabledExplicitly help here?

4204

(optional) Now that you have ToolChain, you probably don't need KernelOrKext.

I'll be uploading a new patch soon.

include/clang/Driver/ToolChain.h
25

Left-over from when I was overcomplicating.

58

Removed.

85

It's const, calculated on object construction.

lib/Driver/SanitizerArgs.cpp
204

I'm now checking which of the three possible arguments was used, and use that one.

206

I took the return out. I also added the Vptr sanitizer to AllRemove, since we don't want to warn about it, afterwards.

227

I moved this warning to only show up if we have -fsanitize=vptr and rtti is disabled implicitly, since that is the case we really need a heads-up.
If we used -fsanitize=undefined and rtti is disabled, then we silently disable vptr.

lib/Driver/ToolChain.cpp
33

I'm accounting for -frtti, now, and added a testcase for that example.

42

Done.

lib/Driver/Tools.cpp
1975

In the current state, I'd rather not.
To get a 100% correct answer to that question, we need an InputFile, and need to know if it's C++. We don't have that in the ToolChain, AFAICT.

The getRTTIMode function doesn't do an exact match. It assumes we “want rtti if in any mode with -fexceptions”, which is good enough for its use-case, because we don't need to pass anything to enable rtti, and -cc1 will do “what's right” if we're compiling a C program, pass -fexceptions, and don't pass any rtti-related argument.

addExceptionArgs() actually has to know about the input file and its type, to know if it should pass -fcxx-exceptions to the program. Which means it can't have a false positive when we used -fexceptions for a C file.

1991

Yes, of course. Added.

4204

KernelOrKext is there only to avoid calling for Args.hasArg(...) so many times. It's not absolutely necessary, but having access to ToolChain doesn't make a difference, since addExceptionArgs already had access to Args and still got that boolean passed.

I would tend to keep it, but don't feel strongly. Will remove if you want.

filcab updated this revision to Diff 19957.Feb 13 2015, 7:23 PM

Updated with Alexey's comments.

Looks better now.

include/clang/Driver/ToolChain.h
85

please move this field upper, right after D/Triple/Args

lib/Driver/SanitizerArgs.cpp
201

I think you can remove static_cast<unsigned> here and below.

209

Do you think you can cache argument that is believed to explicitly disable/enable RTTI next to ToolChain::CachedRTTIMode? Then you will be able to use it here and in addExceptionArgs() diagnostics.

225

Comment is now misleading.

lib/Driver/ToolChain.cpp
32

you can use

if (Arg *RTTIArg = Args.getLastArg(...)) {
  ...
}

here

53

We generally don't use else-after-return:

if (Exceptions && ...)
  return ToolChain::RM_EnabledImplicitly;
return ToolChain::RM_DisabledImplicitly;
lib/Driver/Tools.cpp
1937

Extra / ?

1975–1978

now that you re-use this variable, it deserves a better name. ExceptionArg?

1984

You can hide this declaration under

if (Triple.isPS4CPU()) {..}
1990

What if RTTI was disabled by a different argument?

4041

Does anyone uses sanitizesVptr() now? If not, we'd better delete this method.

test/Driver/fsanitize.c
34–36

Please also add a test for silent disabling of vptr sanitizer if we pass "-fsanitize=undefined -fno-rtti"

filcab updated this revision to Diff 20232.Feb 18 2015, 3:54 PM

Addressed the latest comments from Alexey

samsonov accepted this revision.Feb 18 2015, 4:29 PM
samsonov edited edge metadata.

LGTM. Feel free to submit this after addressing comments below. Thank you for working on this!

include/clang/Driver/ToolChain.h
69

Looks like it should be

const llvm::opt::Arg *const CachedRTTIArg;
lib/Driver/SanitizerArgs.cpp
217

Remove static_cast.

225

We disable the vptr sanitizer if it was enabled by group expansion, but RTTI is disabled.

230

Remove this comment.

lib/Driver/Tools.cpp
1996

add an assert that RTTIMode is not DisabledImplicitly?

This revision is now accepted and ready to land.Feb 18 2015, 4:29 PM
This revision was automatically updated to reflect the committed changes.