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

[Alexey Samsonov]

================
Comment at: include/clang/Driver/ToolChain.h:25
@@ -24,2 +24,3 @@
 namespace opt {
+  class Arg;
   class ArgList;
----------------
Why do you need this?

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

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

================
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
----------------
RTTI can be disabled by a different argument.

================
Comment at: lib/Driver/SanitizerArgs.cpp:206
@@ +205,3 @@
+        // Bail out since we already have an error to deal with
+        return;
+      }
----------------
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".

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

================
Comment at: lib/Driver/ToolChain.cpp:33
@@ +32,3 @@
+                      options::OPT_fno_rtti))
+    return ToolChain::RM_DisabledExplicitly;
+
----------------
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).

================
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,
----------------
You need to parse these arguments only if you're on PS4. Otherwise you can return RM_Enabled straight away.

================
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,
----------------
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.

================
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))
----------------
Wouldn't RM_EnabledExplicitly help here?

================
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);
----------------
(optional) Now that you have ToolChain, you probably don't need KernelOrKext.

http://reviews.llvm.org/D7525

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