[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added a comment. In D118428#3312621 , @ychen wrote: > In D118428#3312440 , @paulkirth > wrote: > >> Hi, >> >> We have two failing test cases on Fuchsia's clang canary builder on Windows >> x64. >> >> LLVM

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added a comment. In D118428#3312440 , @paulkirth wrote: > Hi, > > We have two failing test cases on Fuchsia's clang canary builder on Windows > x64. > > LLVM :: Instrumentation/JustMyCode/jmc-instrument-x86.ll > LLVM ::

[PATCH] D118428: [clang-cl] Support the /JMC flag

paulkirth added a comment. Hi, We have two failing test cases on Fuchsia's clang canary builder on Windows x64. LLVM :: Instrumentation/JustMyCode/jmc-instrument-x86.ll LLVM :: Instrumentation/JustMyCode/jmc-instrument.ll First seen here:

[PATCH] D118428: [clang-cl] Support the /JMC flag

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. ychen marked an inline comment as done. Closed by commit rGbd3a1de683f8: [clang-cl] Support the /JMC flag (authored by ychen). Changed prior to commit:

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans accepted this revision. hans added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 ___

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen marked an inline comment as done. ychen added inline comments. Comment at: llvm/test/CodeGen/X86/jmc-instrument.ll:2 +; Check that the flag symbol is not full-qualified. +; RUN: llc < %s -enable-jmc-instrument | FileCheck %s + hans wrote: > ychen wrote: >

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 407384. ychen marked an inline comment as done. ychen added a comment. - Remove CodeViewDebug.cpp and the associated test. It was needed because the instrumented flag variables were scoped inside the associated method in the CodeView. The current patch puts

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1096 + if (TM->Options.JMCInstrument) +addPass(createJMCInstrumenterPass()); addCodeGenPrepare(); ychen wrote: > hans wrote: > > could this be moved into addIRPasses()? > Yes,

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 406927. ychen marked an inline comment as done. ychen added a comment. - address feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen marked an inline comment as done. ychen added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:180 + "JMC instrument more than once?"); + FunctionCallee Fn = + M.getOrInsertFunction(CheckFunctionName, getCheckFunctionType(Ctx));

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:180 + "JMC instrument more than once?"); + FunctionCallee Fn = + M.getOrInsertFunction(CheckFunctionName, getCheckFunctionType(Ctx)); might as well get the

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added a comment. In D118428#3294935 , @ychen wrote: > The instrumentation is per-function, ideally for each function that has > debuginfo and ends up in the executable. So I want it to happen the last in > the IR codegen pipeline (target could

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added a comment. In D118428#3294880 , @rnk wrote: > In D118428#3294411 , @ychen wrote: > >> The passes in `lib/Transforms/Instrumentation` runs with >> `EmitAssemblyHelper::RunOptimizationPipeline`. JMC

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added a comment. In D118428#3294411 , @ychen wrote: > The passes in `lib/Transforms/Instrumentation` runs with > `EmitAssemblyHelper::RunOptimizationPipeline`. JMC pass runs with > `EmitAssemblyHelper::RunCodegenPipeline`. Sure, but that's a

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added a comment. In D118428#3292918 , @hans wrote: > It seems Phabricator ate my comment, but I meant to say: > > My understanding is still that IR passes generally live in Transforms/ and > that CodeGen/ deals with lower levels such as

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added a comment. It seems Phabricator ate my comment, but I meant to say: My understanding is still that IR passes generally live in Transforms/ and that CodeGen/ deals with lower levels such as MachineInstrs, SelectionDAG, etc. with CodegenPrepare as the big exception. Thinking about it

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added inline comments. Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462 + static cl::opt EnableJMCInstrument( + "enable-jmc-instrument", + cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"), ychen wrote: > hans wrote:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 405520. ychen added a comment. - Remove a file that is included by accident. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files: clang/docs/ReleaseNotes.rst

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145 + LLVMContext = M.getContext(); + bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86; + ychen wrote: > ychen wrote: > > hans wrote: > > > I still worry a

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 405406. ychen marked an inline comment as done. ychen added a comment. - Address feeback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added inline comments. Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462 + static cl::opt EnableJMCInstrument( + "enable-jmc-instrument", + cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"), hans wrote: > ychen wrote:

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462 + static cl::opt EnableJMCInstrument( + "enable-jmc-instrument", + cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"), ychen wrote: > hans wrote:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145 + LLVMContext = M.getContext(); + bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86; + ychen wrote: > hans wrote: > > I still worry a bit about the

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added inline comments. Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462 + static cl::opt EnableJMCInstrument( + "enable-jmc-instrument", + cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"), hans wrote: > ychen wrote:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 405018. ychen marked 2 inline comments as done. ychen added a comment. - address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files:

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added inline comments. Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462 + static cl::opt EnableJMCInstrument( + "enable-jmc-instrument", + cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"), ychen wrote: > hans wrote:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen added a comment. @hans @aganea I think all comments are addressed except that I don't know how to test the ARM/ARM64 JMC function (I don't have ARM hardware or is there any ARM-based Windows virtual machine?). PTAL. Comment at: clang/docs/ReleaseNotes.rst:155 +- Add

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 404805. ychen marked 10 inline comments as done. ychen added a comment. - address hans's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files:

[PATCH] D118428: [clang-cl] Support the /JMC flag

aganea added inline comments. Comment at: clang/docs/ReleaseNotes.rst:155 +- Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl, and + equivalent -cc1 flag ``-fjmc``. ``/JMC`` could only be used when ``/Zi`` or + ``/Z7`` is turned on. With this addition,

[PATCH] D118428: [clang-cl] Support the /JMC flag

hans added a comment. This is great stuff, thanks! Comment at: clang/docs/ReleaseNotes.rst:155 +- Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl, and + equivalent -cc1 flag ``-fjmc``. ``/JMC`` could only be used when ``/Zi`` or + ``/Z7`` is turned on.

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 404307. ychen added a comment. - address Alexandre's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files: clang/docs/ReleaseNotes.rst

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen marked 2 inline comments as done. ychen added a comment. Thanks a lot for reviewing the patch! In D118428#3281721 , @aganea wrote: > Cool! :) Seems good generally. > Sounds like an expensive flag for the runtime perf :-D But I guess it makes >

[PATCH] D118428: [clang-cl] Support the /JMC flag

aganea added a reviewer: mstorsjo. aganea added a subscriber: belkiss. aganea added a comment. Cool! :) Seems good generally. Sounds like an expensive flag for the runtime perf :-D But I guess it makes the debugging experience a bit nicer. Comment at:

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen updated this revision to Diff 403870. ychen added a comment. - update Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118428/new/ https://reviews.llvm.org/D118428 Files: clang/docs/ReleaseNotes.rst

[PATCH] D118428: [clang-cl] Support the /JMC flag

ychen created this revision. ychen added reviewers: rnk, aganea, hans. Herald added subscribers: ormris, dexonsmith, dang, pengfei, hiraditya, mgorny. ychen requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. The introduction