[PATCH] Change how we deal with (implicit) -fno-rtti + -fsanitize=vptr
Alexey Samsonov
vonosmas at gmail.com
Fri Feb 13 15:56:27 PST 2015
================
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/
More information about the cfe-commits
mailing list