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

[Filipe Cabecinhas]

I'll be uploading a new patch soon.


================
Comment at: include/clang/Driver/ToolChain.h:25
@@ -24,2 +24,3 @@
 namespace opt {
+  class Arg;
   class ArgList;
----------------
samsonov wrote:
> Why do you need this?
Left-over from when I was overcomplicating.

================
Comment at: include/clang/Driver/ToolChain.h:58
@@ +57,3 @@
+  enum RTTIMode {
+    RM_NotComputed,
+    RM_Enabled,
----------------
samsonov wrote:
> This value is never used.
Removed.

================
Comment at: include/clang/Driver/ToolChain.h:85
@@ -76,2 +84,3 @@
 
+  RTTIMode CachedRTTIMode;
   mutable std::unique_ptr<SanitizerArgs> SanitizerArguments;
----------------
samsonov wrote:
> looks like this should be either const, or mutable and calculated lazily.
It's const, calculated on object construction.

================
Comment at: lib/Driver/SanitizerArgs.cpp:204
@@ +203,3 @@
+        D.Diag(diag::err_drv_argument_not_allowed_with)
+            << "-fsanitize=vptr" << "-fno-rtti";
+        // Bail out since we already have an error to deal with
----------------
samsonov wrote:
> RTTI can be disabled by a different argument.
I'm now checking which of the three possible arguments was used, and use that one.

================
Comment at: lib/Driver/SanitizerArgs.cpp:206
@@ +205,3 @@
+        // Bail out since we already have an error to deal with
+        return;
+      }
----------------
samsonov wrote:
> 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".
I took the return out. I also added the Vptr sanitizer to AllRemove, since we don't want to warn about it, afterwards.

================
Comment at: lib/Driver/SanitizerArgs.cpp:227
@@ +226,3 @@
+  // -fsanitize=undefined were passed.
+  if (Sanitizers.has(SanitizerKind::Vptr) &&
+      (RTTIMode == ToolChain::RM_DisabledImplicitly ||
----------------
samsonov wrote:
> I'm inclined to silently disable -fsanitize=vptr if it was enabled implicitly by -fsanitize=undefined.
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.

================
Comment at: lib/Driver/ToolChain.cpp:33
@@ +32,3 @@
+                      options::OPT_fno_rtti))
+    return ToolChain::RM_DisabledExplicitly;
+
----------------
samsonov wrote:
> 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).
I'm accounting for -frtti, now, and added a testcase for that example.

================
Comment at: lib/Driver/ToolChain.cpp:42
@@ +41,3 @@
+  // We're assuming that, if we see -fexceptions, rtti gets turned on.
+  Arg *Exceptions = Args.getLastArg(
+      options::OPT_fcxx_exceptions, options::OPT_fno_cxx_exceptions,
----------------
samsonov wrote:
> You need to parse these arguments only if you're on PS4. Otherwise you can return RM_Enabled straight away.
Done.

================
Comment at: lib/Driver/Tools.cpp:1975
@@ -1973,5 +1974,3 @@
         Triple.getArch() != llvm::Triple::xcore && !Triple.isPS4CPU();
-    if (Arg *A = Args.getLastArg(options::OPT_fcxx_exceptions,
-                                 options::OPT_fno_cxx_exceptions,
-                                 options::OPT_fexceptions,
-                                 options::OPT_fno_exceptions))
+    Arg *A = Args.getLastArg(
+        options::OPT_fcxx_exceptions, options::OPT_fno_cxx_exceptions,
----------------
samsonov wrote:
> 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.
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.

================
Comment at: lib/Driver/Tools.cpp:1991
@@ +1990,3 @@
+              << "-fno-rtti" << A->getAsString(Args);
+        else if (RTTIMode == ToolChain::RM_Enabled &&
+                 !Args.hasArg(options::OPT_frtti))
----------------
samsonov wrote:
> Wouldn't RM_EnabledExplicitly help here?
Yes, of course. Added.

================
Comment at: lib/Driver/Tools.cpp:4204
@@ -4235,3 +4203,3 @@
   if (!C.getDriver().IsCLMode())
-    addExceptionArgs(Args, InputType, getToolChain().getTriple(), KernelOrKext,
+    addExceptionArgs(Args, InputType, getToolChain(), KernelOrKext,
                      objcRuntime, CmdArgs);
----------------
samsonov wrote:
> (optional) Now that you have ToolChain, you probably don't need KernelOrKext.
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.

http://reviews.llvm.org/D7525

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/