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

Sat Jan 29 15:33:11 PST 2022

ychen marked 2 inline comments as done.
ychen added a comment.

Thanks a lot for reviewing the patch!

In D118428#3281721 <https://reviews.llvm.org/D118428#3281721>, @aganea wrote:

> Cool! :) Seems good generally.
> Sounds like an expensive flag for the runtime perf :-D But I guess it makes the debugging experience a bit nicer.

Exactly. I had the same thoughts. For MSVC it is what it is. For ELF, the other choice from using the same scheme as MSVC is that we could inline the `__CheckForDebuggerJustMyCode` call (its definition is here: https://github.com/ojdkbuild/tools_toolchain_vs2017bt_1416/blob/master/VC/Tools/MSVC/14.16.27023/crt/src/vcruntime/debugger_jmc.c) before the function ABI prologue. For example, to instrument a function `_foo`:

    andb r11l, _jmc_global_flag, _jmc_enable_per_thread_step
    andb r11l, r11, _jmc_per_module_flag
    andb r11l, r11, _jmc_per_file_flag
    cmpb r11l, 0
    jne _foo 
    int 3
  _foo:

The parameter of `__CheckForDebuggerJustMyCode` is `_jmc_per_file_flag` above. `int 3` is basically a per-function flag. `_jmc_per_module_flag` and `_jmc_global_flag` control the JMC behavior at module-level or program-level for better performance. Otherwise, the debugger needs the gather this information using debuginfo. This scheme is target-dependent and increases the .text slightly. But it eliminates the call and the flag modification at different granularity is fast. With that being said, experts working on ELF debuggers have ideas about which scheme is better. I'll send an RFC for ELF after this patch goes in.

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7457
+                   /*Default=*/false)) {
+    if (EnabledZ7)
+      CmdArgs.push_back("-fjmc");
----------------
aganea wrote:
> Can we do here `if (*EmitCodeView && *DebugInfoKind >= codegenoptions::DebugInfoConstructor)`? Is it worth introducing a new variable?
Yeah, I think that works better. No need for a new variable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118428