Thanks Justin. Addressed the review and committed in r242565.
Steven > On Jul 16, 2015, at 9:04 PM, Justin Bogner <m...@justinbogner.com> wrote: > > Steven Wu <steve...@apple.com <mailto:steve...@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 <steve...@apple.com <mailto:steve...@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"
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits