[PATCH] Fix -save-temp doesn't work well with ObjCARC, sanitizer and profiling
Justin Bogner
mail at justinbogner.com
Thu Jul 16 21:04:47 PDT 2015
Steven Wu <stevenwu at apple.com> writes:
> Hi all
>
> Currently, -save-temp option doesn’t work well with ObjCARC, sanitizer
> and profiling instrumentation. It will drop all ObjCARC optimizations,
> runs sanitizer pass very early in the pipeline and profiling
> instrumentation will happen twice (which is a no-op second time). They
> are because of a combination of two issues:
> 1. Language options are not parsed and get dropped when the input is
> LLVM IR. Thus no ObjcARC pass and Sanitizer pass are run from bitcode
> input.
> 2. We use -disable-llvm-optzns to get pristine LLVM IR generated from
> front-end but the flag is not as strong as expected. It will run the
> instrumentation passes like sanitizer and profiling.
>
> In this patch, I added a new flag -disable-llvm-passes to disable
> every pass from optimization pipeline including instrumentations and
> use it in -save-temp. It should be useful for debugging clang CodeGen
> as well. I also change the clang front-end to parse ObjcARC and
> sanitizer flag in all conditions. I didn’t touch -disable-llvm-optzns
> because I am not sure if the flag is supposed to disable
> instrumentation passes and if someone is relying on something like:
> clang -sanitizer=address -disable-llvm-optzns
> Tightening up -disable-llvm-optzns might break them.
This seems pretty reasonable. LGTM with a couple of minor comments.
> Thanks
>
> Steven
>
> From 57369fb85529c943c394cfe3bf5bc35dcdbd2c59 Mon Sep 17 00:00:00 2001
> From: Steven Wu <stevenwu at apple.com>
> Date: Mon, 13 Jul 2015 11:12:28 -0700
> Subject: [PATCH] Fix -save-temp when using objc-arc, sanitizer and profiling
>
> Currently, -save-temp will cause ObjCARC optimization to be dropped,
> sanitizer pass to run early in the pipeline, and profiling
> instrumentation to run twice.
> Fix the issue by properly disable all passes in the optimization
> pipeline when generating bitcode output and parse some of the Language
> Options even when the input is bitcode so the passes can be setup
> correctly.
> ---
> include/clang/Driver/CC1Options.td | 3 +++
> include/clang/Frontend/CodeGenOptions.def | 3 +++
> lib/CodeGen/BackendUtil.cpp | 3 +++
> lib/Driver/Tools.cpp | 11 ++++------
> lib/Frontend/CompilerInvocation.cpp | 6 ++++++
> test/CodeGen/sanitize-address-field-padding.cpp | 1 +
> test/CodeGenObjC/arc.ll | 27 +++++++++++++++++++++++++
> test/Driver/save-temps.c | 8 ++++----
> 8 files changed, 51 insertions(+), 11 deletions(-)
> create mode 100644 test/CodeGenObjC/arc.ll
>
> diff --git a/include/clang/Driver/CC1Options.td b/include/clang/Driver/CC1Options.td
> index 60cc6ec..f7bb6f3 100644
> --- a/include/clang/Driver/CC1Options.td
> +++ b/include/clang/Driver/CC1Options.td
> @@ -153,6 +153,9 @@ def disable_llvm_optzns : Flag<["-"], "disable-llvm-optzns">,
> HelpText<"Don't run LLVM optimization passes">;
> def disable_llvm_verifier : Flag<["-"], "disable-llvm-verifier">,
> HelpText<"Don't run the LLVM IR verifier pass">;
> +def disable_llvm_passes : Flag<["-"], "disable-llvm-passes">,
> + HelpText<"Don't run any LLVM passes including instrumentation "
> + "in order to get the IR generated by frontend as it is">;
This wording's kind of confusing. I'd say something like "Emit pristine
IR from the frontend by not running any LLVM passes at all". Similarly
for the CODEGENOPT below.
> def disable_red_zone : Flag<["-"], "disable-red-zone">,
> HelpText<"Do not emit code that uses the red zone.">;
> def dwarf_column_info : Flag<["-"], "dwarf-column-info">,
> diff --git a/include/clang/Frontend/CodeGenOptions.def b/include/clang/Frontend/CodeGenOptions.def
> index 803d023..74378b4 100644
> --- a/include/clang/Frontend/CodeGenOptions.def
> +++ b/include/clang/Frontend/CodeGenOptions.def
> @@ -48,6 +48,9 @@ CODEGENOPT(DisableLLVMOpts , 1, 0) ///< Don't run any optimizations, for use i
> ///< getting .bc files that correspond to the
> ///< internal state before optimizations are
> ///< done.
> +CODEGENOPT(DisableLLVMPasses , 1, 0) ///< Don't run any optimizations
> + ///< inclduing instrumetation to get the
> + ///< IR generated by frontend as it it.
> CODEGENOPT(DisableRedZone , 1, 0) ///< Set when -mno-red-zone is enabled.
> CODEGENOPT(DisableTailCalls , 1, 0) ///< Do not emit tail calls.
> CODEGENOPT(EmitDeclMetadata , 1, 0) ///< Emit special metadata indicating what
> diff --git a/lib/CodeGen/BackendUtil.cpp b/lib/CodeGen/BackendUtil.cpp
> index 5ff760c..0c870ee 100644
> --- a/lib/CodeGen/BackendUtil.cpp
> +++ b/lib/CodeGen/BackendUtil.cpp
> @@ -272,6 +272,9 @@ static void addSymbolRewriterPass(const CodeGenOptions &Opts,
> }
>
> void EmitAssemblyHelper::CreatePasses() {
> + if (CodeGenOpts.DisableLLVMPasses)
> + return;
> +
> unsigned OptLevel = CodeGenOpts.OptimizationLevel;
> CodeGenOptions::InliningMethod Inlining = CodeGenOpts.getInlining();
>
> diff --git a/lib/Driver/Tools.cpp b/lib/Driver/Tools.cpp
> index 0757355..698a842 100644
> --- a/lib/Driver/Tools.cpp
> +++ b/lib/Driver/Tools.cpp
> @@ -4785,7 +4785,6 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
> // Forward -Xclang arguments to -cc1, and -mllvm arguments to the LLVM option
> // parser.
> Args.AddAllArgValues(CmdArgs, options::OPT_Xclang);
> - bool OptDisabled = false;
> for (const Arg *A : Args.filtered(options::OPT_mllvm)) {
> A->claim();
>
> @@ -4793,17 +4792,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
> // it and developers have been trained to spell it with -mllvm.
> if (StringRef(A->getValue(0)) == "-disable-llvm-optzns") {
> CmdArgs.push_back("-disable-llvm-optzns");
> - OptDisabled = true;
> } else
> A->render(Args, CmdArgs);
> }
>
> // With -save-temps, we want to save the unoptimized bitcode output from the
> - // CompileJobAction, so disable optimizations if they are not already
> - // disabled.
> - if (C.getDriver().isSaveTempsEnabled() && !OptDisabled &&
> - isa<CompileJobAction>(JA))
> - CmdArgs.push_back("-disable-llvm-optzns");
> + // CompileJobAction, use -disable-llvm-passes to get pristine IR generated
> + // by the frontend.
> + if (C.getDriver().isSaveTempsEnabled() && isa<CompileJobAction>(JA))
> + CmdArgs.push_back("-disable-llvm-passes");
>
> if (Output.getType() == types::TY_Dependencies) {
> // Handled with other dependency code.
> diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp
> index baee119..fed2717 100644
> --- a/lib/Frontend/CompilerInvocation.cpp
> +++ b/lib/Frontend/CompilerInvocation.cpp
> @@ -426,6 +426,7 @@ static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
> Opts.EmitLLVMUseLists = A->getOption().getID() == OPT_emit_llvm_uselists;
>
> Opts.DisableLLVMOpts = Args.hasArg(OPT_disable_llvm_optzns);
> + Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
> Opts.DisableRedZone = Args.hasArg(OPT_disable_red_zone);
> Opts.ForbidGuardVariables = Args.hasArg(OPT_fforbid_guard_variables);
> Opts.UseRegisterSizedBitfieldAccess = Args.hasArg(
> @@ -1887,6 +1888,11 @@ bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
> ParseLangArgs(*Res.getLangOpts(), Args, DashX, Diags);
> if (Res.getFrontendOpts().ProgramAction == frontend::RewriteObjC)
> Res.getLangOpts()->ObjCExceptions = 1;
> + } else {
> + if (Args.hasArg(OPT_fobjc_arc))
> + Res.getLangOpts()->ObjCAutoRefCount = 1;
> + parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
> + Diags, Res.getLangOpts()->Sanitize);
This deserves a comment. I'd probably also reverse the order of the
if/else conditions to avoid the double negative in the else case.
> }
> // FIXME: ParsePreprocessorArgs uses the FileManager to read the contents of
> // PCH file and find the original header name. Remove the need to do that in
> diff --git a/test/CodeGen/sanitize-address-field-padding.cpp b/test/CodeGen/sanitize-address-field-padding.cpp
> index d4eea1b..7bd368f 100644
> --- a/test/CodeGen/sanitize-address-field-padding.cpp
> +++ b/test/CodeGen/sanitize-address-field-padding.cpp
> @@ -5,6 +5,7 @@
> // RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.type.blacklist -Rsanitize-address -emit-llvm -o - %s -O1 -mconstructor-aliases 2>&1 | FileCheck %s --check-prefix=WITH_CTOR_ALIASES
> // RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.file.blacklist -Rsanitize-address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=FILE_BLACKLIST
> // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=NO_PADDING
> +// RUN: %clang -save-temps -fsanitize=address -emit-llvm -S -o - %s 2>&1 | FileCheck %s --check-prefix=NO_PADDING
> //
>
> // The reasons to ignore a particular class are not set in stone and will change.
> diff --git a/test/CodeGenObjC/arc.ll b/test/CodeGenObjC/arc.ll
> new file mode 100644
> index 0000000..caafcff
> --- /dev/null
> +++ b/test/CodeGenObjC/arc.ll
> @@ -0,0 +1,27 @@
> +; RUN: %clang_cc1 -Os -emit-llvm -fobjc-arc -o - %s | FileCheck %s
> +
> +target triple = "x86_64-apple-darwin10"
> +
> +declare i8* @objc_retain(i8*)
> +declare void @objc_release(i8*)
> +
> +; CHECK-LABEL: define void @test(
> +; CHECK-NOT: @objc_
> +; CHECK: }
> +define void @test(i8* %x, i1* %p) nounwind {
> +entry:
> + br label %loop
> +
> +loop:
> + call i8* @objc_retain(i8* %x)
> + %q = load i1, i1* %p
> + br i1 %q, label %loop.more, label %exit
> +
> +loop.more:
> + call void @objc_release(i8* %x)
> + br label %loop
> +
> +exit:
> + call void @objc_release(i8* %x)
> + ret void
> +}
> diff --git a/test/Driver/save-temps.c b/test/Driver/save-temps.c
> index 277a901..c974d15 100644
> --- a/test/Driver/save-temps.c
> +++ b/test/Driver/save-temps.c
> @@ -2,7 +2,7 @@
> // RUN: | FileCheck %s
> // CHECK: "-o" "save-temps.i"
> // CHECK: "-emit-llvm-uselists"
> -// CHECK: "-disable-llvm-optzns"
> +// CHECK: "-disable-llvm-passes"
> // CHECK: "-o" "save-temps.bc"
> // CHECK: "-o" "save-temps.s"
> // CHECK: "-o" "save-temps.o"
> @@ -14,7 +14,7 @@
> // RUN: | FileCheck %s -check-prefix=CWD
> // CWD: "-o" "save-temps.i"
> // CWD: "-emit-llvm-uselists"
> -// CWD: "-disable-llvm-optzns"
> +// CWD: "-disable-llvm-passes"
> // CWD: "-o" "save-temps.bc"
> // CWD: "-o" "save-temps.s"
> // CWD: "-o" "save-temps.o"
> @@ -63,7 +63,7 @@
> // RUN: %clang -target x86_64-apple-darwin -save-temps=obj -o obj/dir/a.out -arch x86_64 %s -### 2>&1 \
> // RUN: | FileCheck %s -check-prefix=CHECK-OBJ
> // CHECK-OBJ: "-o" "obj/dir{{/|\\\\}}save-temps.i"
> -// CHECK-OBJ: "-disable-llvm-optzns"
> +// CHECK-OBJ: "-disable-llvm-passes"
> // CHECK-OBJ: "-o" "obj/dir{{/|\\\\}}save-temps.bc"
> // CHECK-OBJ: "-o" "obj/dir{{/|\\\\}}save-temps.s"
> // CHECK-OBJ: "-o" "obj/dir{{/|\\\\}}save-temps.o"
> @@ -72,7 +72,7 @@
> // RUN: %clang -target x86_64-apple-darwin -save-temps=obj -arch x86_64 %s -### 2>&1 \
> // RUN: | FileCheck %s -check-prefix=CHECK-OBJ-NOO
> // CHECK-OBJ-NOO: "-o" "save-temps.i"
> -// CHECK-OBJ-NOO: "-disable-llvm-optzns"
> +// CHECK-OBJ-NOO: "-disable-llvm-passes"
> // CHECK-OBJ-NOO: "-o" "save-temps.bc"
> // CHECK-OBJ-NOO: "-o" "save-temps.s"
> // CHECK-OBJ-NOO: "-o" "save-temps.o"
More information about the cfe-commits
mailing list