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

[Hans Wennborg via Phabricator via llvm-commits]

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