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

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:462
+  static cl::opt<bool> EnableJMCInstrument(
+      "enable-jmc-instrument",
+      cl::desc("Instrument functions with a call to __CheckForDebuggerJustMyCode"),
----------------
hans wrote:
> ychen wrote:
> > hans wrote:
> > > Other "on/off" options don't seem to have "enable" in the name or flag spelling, e.g. "-strict-dwarf", not "-enable-strict-dwarf". Maybe this should be just "-jmc-instrument" and JMCInstrument?
> > The "-jmc-instrument" is already used by the pass itself (`#define DEBUG_TYPE "jmc-instrument"`). The `DEBUG_TYPE` one enables `opt -jmc-instrument`; this makes `llc -enable-jmc-instrument` to run the pass in IR codegen pipeline.
> > 
> > Just renamed `cl::opt<bool> EnableJMCInstrument` to `cl::opt<bool> JMCInstrument`.
> Maybe the fact that -jmc-instrument is used by the IR pass is a hint that there shouldn't be an llc option for this, then? Looking at the other options here, they're all about codegen features, whereas the instrumentation in this patch really happens at a higher level.
There are three kinds of passes (each in a separate pipeline), in the order of execution: IR optimization passes, IR codegen passes (some examples are here: https://github.com/llvm/llvm-project/blob/1d0244aed78114d5bd03ec4930d7687d6e587f99/llvm/include/llvm/CodeGen/MachinePassRegistry.def#L38-L51 and https://github.com/llvm/llvm-project/blob/1d0244aed78114d5bd03ec4930d7687d6e587f99/llvm/include/llvm/CodeGen/MachinePassRegistry.def#L106-L121) and MIR codegen passes. The JMC pass is an IR codegen passes. It runs within the codegen phase, but it transforms IR and it is tested using the `opt` tool. The llc option is for testing that the pass could be enabled using `TargetOptions::JMCInstrument` (clang uses this), the change in `llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp` and this option enables LTO+JMC with `lld -mllvm -enable-jmc-instrument`. 


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145
+  LLVMContext &Ctx = M.getContext();
+  bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
hans wrote:
> I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
> 
> I'm also not sure that lib/CodeGen/ is the right place for this pass, since most files there seem to be machine-IR passes. Maybe the natural place for this would be lib/Transforms/Instrumentation/? Is there some good pass we can compare this with?
> I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
Understood. `TargetTransformInfo` is mostly for the "IR optimization passes". The JMC pass is "IR codegen passes", it is more similar to `CodeGenPrepare` pass than any "IR optimization passes". I think we could move the target-specific stuff into the `TargetPassConfig` & its derived classes, not in this patch, but the following ELF port one. WDYT?


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:147
+
+  auto GetOrCreateCheckFunctionDecl = [&, Decl = FunctionCallee()]() mutable {
+    if (Decl)
----------------
hans wrote:
> This kind of mutable lambda feels pretty subtle to me. I think something like the below would be more straight-forward.
> 
> ```
> Function *CheckFunction = nullptr;
> for (...) {
>   ...
>   if (!CheckFunction) {
>     // Create the decl here.
>   }
> }
> ```
> 
> But if you strongly prefer the lambda, I suppose it's okay.
yep, sounds good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118428/new/

https://reviews.llvm.org/D118428