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
