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

[Alexey Samsonov]

Looks better now.


================
Comment at: include/clang/Driver/ToolChain.h:84
@@ -76,2 +83,3 @@
 
+  const RTTIMode CachedRTTIMode;
   mutable std::unique_ptr<SanitizerArgs> SanitizerArguments;
----------------
please move this field upper, right after D/Triple/Args

================
Comment at: lib/Driver/SanitizerArgs.cpp:201
@@ +200,3 @@
+      // passed.
+      if (Add & static_cast<unsigned>(SanitizeKind::Vptr) &&
+          (RTTIMode == ToolChain::RM_DisabledImplicitly ||
----------------
I think you can remove static_cast<unsigned> here and below.

================
Comment at: lib/Driver/SanitizerArgs.cpp:209
@@ +208,3 @@
+        else {
+          llvm::opt::Arg *NoRTTIArg =
+              Args.getLastArg(options::OPT_mkernel, options::OPT_fapple_kext,
----------------
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.

================
Comment at: lib/Driver/SanitizerArgs.cpp:238
@@ -211,1 +237,3 @@
 
+  // Warn that we're disabling the vptr sanitizer if -fno-rtti and
+  // -fsanitize=undefined were passed.
----------------
Comment is now misleading.

================
Comment at: lib/Driver/ToolChain.cpp:32
@@ +31,3 @@
+  // Explicit rtti/no-rtti args
+  Arg *RTTIArg = Args.getLastArg(options::OPT_mkernel, options::OPT_fapple_kext,
+                                 options::OPT_fno_rtti, options::OPT_frtti);
----------------
you can use
  if (Arg *RTTIArg = Args.getLastArg(...)) {
    ...
  }
here

================
Comment at: lib/Driver/ToolChain.cpp:53
@@ +52,3 @@
+       Exceptions->getOption().matches(options::OPT_fcxx_exceptions))) {
+    return ToolChain::RM_EnabledImplicitly;
+  } else {
----------------
We generally don't use else-after-return:

  if (Exceptions && ...)
    return ToolChain::RM_EnabledImplicitly;
  return ToolChain::RM_DisabledImplicitly;

================
Comment at: lib/Driver/Tools.cpp:1937
@@ +1936,3 @@
+/// There's a master flag, -fexceptions and also language specific flags to
+/// enable/disable C++ and Objective-C exceptions./ This makes it possible to
+/// for example disable C++ exceptions but enable Objective-C exceptions.
----------------
Extra / ?

================
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,
----------------
now that you re-use this variable, it deserves a better name. ExceptionArg?

================
Comment at: lib/Driver/Tools.cpp:1984
@@ -1982,1 +1983,3 @@
     if (CXXExceptionsEnabled) {
+      ToolChain::RTTIMode RTTIMode = TC.getRTTIMode();
+      if (Triple.isPS4CPU()) {
----------------
You can hide this declaration under
  if (Triple.isPS4CPU()) {..}

================
Comment at: lib/Driver/Tools.cpp:1990
@@ +1989,3 @@
+          D.Diag(diag::err_drv_argument_not_allowed_with)
+              << "-fno-rtti" << A->getAsString(Args);
+        else if (RTTIMode == ToolChain::RM_EnabledImplicitly)
----------------
What if RTTI was disabled by a different argument?

================
Comment at: lib/Driver/Tools.cpp:4041
@@ -4040,3 @@
-    // -fno-rtti cannot usefully be combined with -fsanitize=vptr.
-    if (Sanitize.sanitizesVptr()) {
-      // If rtti was explicitly disabled and the vptr sanitizer is on, error
----------------
Does anyone uses `sanitizesVptr()` now? If not, we'd better delete this method.

================
Comment at: test/Driver/fsanitize.c:34
@@ -33,4 +33,3 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=vptr -fno-rtti %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-NO-RTTI
-// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fno-rtti %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-NO-RTTI
 // CHECK-VPTR-NO-RTTI: '-fsanitize=vptr' not allowed with '-fno-rtti'
 
----------------
Please also add a test for silent disabling of vptr sanitizer if we pass "-fsanitize=undefined -fno-rtti"

http://reviews.llvm.org/D7525

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