[PATCH] Fix -save-temp doesn't work well with ObjCARC, sanitizer and profiling
Steven Wu
stevenwu at apple.com
Fri Jul 17 13:11:13 PDT 2015
Thanks Justin.
Addressed the review and committed in r242565.
Steven
> On Jul 16, 2015, at 9:04 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
> Steven Wu <stevenwu at apple.com <mailto: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 <mailto: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"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150717/e48b66c8/attachment.html>
More information about the cfe-commits
mailing list