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

Yuanfang Chen via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 8 12:11:19 PST 2022

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));
hans wrote:
> might as well get the Function* already here?
> ```
> CheckFunction = cast<Function>(M.getOrInsertFunction(...).getCallee());
> ```
sounds good.

Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1096
+  if (TM->Options.JMCInstrument)
+    addPass(createJMCInstrumenterPass());
hans wrote:
> could this be moved into addIRPasses()?
Yes, it could. `CFGuardCheckPass` is there for targets with COFF. I didn't put it there because I plan to make it work with ELF in the next step, putting it here is convenient. I don't feel strongly though.

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:
> hans wrote:
> > Since it's an IR pass, I think we don't need to have an llc test at all.
> I still think it's unusual to have an llc test for an IR pass. Are there any other examples where we do something similar?
yes, it is pretty rare. I wanted to test the `CodeViewDebug.cpp` change, however, the code path only triggers when the JMC pass is run. Using `llc` came to mind. I'm open to any alternative.

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list