hans 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"), ---------------- ychen wrote: > 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`. Thanks for the pointer, the IR codegen passes is something I'm not very familiar with. Just looking at a few of the first ones you linked to, I see that they live under lib/Transforms/. And when I look in lib/CodeGen/ the vast majority of those work on MachineFunctions -- but not all. So I'm not really sure how we decide what should go where? I think the best way is to look for a similar pass and see how that works. Maybe llvm/lib/Transforms/CFGuard/CFGuard.cpp could be a good example, since it's also inserting Windows-specific instrumentation at the IR level, similar to this pass. ================ Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:60 +namespace { +const char *const CheckFunctionName = "__CheckForDebuggerJustMyCode"; + ---------------- nit: this could be ``` const char CheckFunctionName[] = "..." ``` ================ Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:158 + Constant *Flag = nullptr; + auto TheFlag = SavedFlags.find(SP); + if (TheFlag == SavedFlags.end()) { ---------------- Having both Flag and TheFlag might be confusing. Could we rely on the DenseMap default-constructing values on lookup and do: ``` Constant *&Flag = SavedFlags[SP]; if (!Flag) { ... Flag = ... } ``` 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits