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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:155
+- Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl, and 
+  equivalent -cc1 flag ``-fjmc``. ``/JMC`` could only be used when ``/Zi`` or
+  ``/Z7`` is turned on. With this addition, clang-cl can be used in Visual
----------------
hans wrote:
> The cc1 flag is more of an implementation detail, so not sure it's worth mentioning in the release notes. Or do we want "clang -fjmc" to work?
@ychen @hans An additional question is whether we want to use the same cc1 flag for an potential ELF port?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7458
+                   /*Default=*/false)) {
+    if (*EmitCodeView && *DebugInfoKind >= codegenoptions::DebugInfoConstructor)
+      CmdArgs.push_back("-fjmc");
----------------
hans wrote:
> Why that level specifically and not e.g. *DebugInfoKind > NoDebugInfo?
@hans MSVC does this check too, not sure why.
```
D:\git\llvm-project>cl main.cpp /c /JMC
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30139 for ARM64
Copyright (C) Microsoft Corporation.  All rights reserved.

cl : Command line warning D9007 : '/JMC' requires '/Zi, /ZI or /Z7'; option ignored
main.cpp
```


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:111
+
+void createDefaultCheckFunction(Module &M, const char *CheckFunctionName,
+                                bool Is32Bit) {
----------------
hans wrote:
> What's the purpose of the default check function? Doesn't the CRT always provide one? (And if it doesn't, will JMC work at all?)
@hans That's what MSVC does, to avoid a linker error if the CRT isn't linked in:
```
D:\git\llvm-project>cat main.cpp
int main() { }
D:\git\llvm-project>cl main.cpp /JMC /Z7 /c
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30139 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

main.cpp

D:\git\llvm-project>dumpbin /disasm main.obj
Microsoft (R) COFF/PE Dumper Version 14.29.30139.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file main.obj

File Type: COFF OBJECT

main:
  0000000000000000: 48 83 EC 28        sub         rsp,28h
  0000000000000004: 48 8D 0D 00 00 00  lea         rcx,[__BB04C4AD_main at cpp]
                    00
  000000000000000B: E8 00 00 00 00     call        __CheckForDebuggerJustMyCode
  0000000000000010: 33 C0              xor         eax,eax
  0000000000000012: 48 83 C4 28        add         rsp,28h
  0000000000000016: C3                 ret

__JustMyCode_Default:
  0000000000000000: C2 00 00           ret         0

  Summary

          50 .chks64
         228 .debug$S
         3EC .debug$T
          70 .drectve
           1 .msvcjmc
           C .pdata
          1A .text$mn
           8 .xdata

D:\git\llvm-project>dumpbin /all main.obj | grep -B12 -A5 'COMDAT; sym= __JustMyCode_Default'
SECTION HEADER #5
.text$mn name
       0 physical address
       0 virtual address
       3 size of raw data
     438 file pointer to raw data (00000438 to 0000043A)
       0 file pointer to relocation table
       0 file pointer to line numbers
       0 number of relocations
       0 number of line numbers
60501020 flags
         Code
         COMDAT; sym= __JustMyCode_Default
         16 byte align
         Execute Read

RAW DATA #5
  00000000: C2 00 00                                         □..
```


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:157
+  const char *CheckFunctionName = "__CheckForDebuggerJustMyCode";
+  bool Is32Bit = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
hans wrote:
> That's a very x86 specific check. What about other targets? (Also the calling conventions are x86 specific..)
@ychen Worth nothing that /JMC works on ARM/ARM64 as well. Should we support that too, not sure that's what @hans is implying?
```
D:\git\llvm-project>cl main.cpp /c /JMC /Z7
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30139 for ARM64
Copyright (C) Microsoft Corporation.  All rights reserved.

main.cpp

D:\git\llvm-project>dumpbin /disasm main.obj
Microsoft (R) COFF/PE Dumper Version 14.29.30139.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file main.obj

File Type: COFF OBJECT

main:
  0000000000000000: A9BF7BFD  stp         fp,lr,[sp,#-0x10]!
  0000000000000004: 910003FD  mov         fp,sp
  0000000000000008: 90000008  adrp        x8,__BB04C4AD_main at cpp
  000000000000000C: 91000100  add         x0,x8,__BB04C4AD_main at cpp
  0000000000000010: 94000000  bl          __CheckForDebuggerJustMyCode
  0000000000000014: 52800000  mov         w0,#0
  0000000000000018: A8C17BFD  ldp         fp,lr,[sp],#0x10
  000000000000001C: D65F03C0  ret

__JustMyCode_Default:
  0000000000000000: D65F03C0  ret

  Summary

          48 .chks64
         228 .debug$S
         3F0 .debug$T
          70 .drectve
           1 .msvcjmc
           8 .pdata
          24 .text$mn
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118428