[PATCH] D118428: [clang-cl] Support the /JMC flag
Yuanfang Chen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 31 21:18:23 PST 2022
ychen added a comment.
@hans @aganea I think all comments are addressed except that I don't know how to test the ARM/ARM64 JMC function (I don't have ARM hardware or is there any ARM-based Windows virtual machine?). PTAL.
================
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
----------------
aganea wrote:
> 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?
Removed. Yeah, I think "clang -fjmc" for an ELF implementation makes sense.
================
Comment at: clang/include/clang/Driver/Options.td:6333
+def _SLASH_JMC_ : CLFlag<"JMC-">,
+ HelpText<"Disable native C/C++ Just My Code debugging (default)">;
def _SLASH_LD : CLFlag<"LD">, HelpText<"Create DLL">;
----------------
hans wrote:
> I'm not sure "native C/C++" adds much info in the context of clang. Maybe just "Enable Just My Code debugging" is enough?
>
> (Also, how do we want to spell this? Other features are not typically capitalized in clang.. so maybe "enable just my code debugging" or "enable just-my-code debugging"?)
Used "enable just-my-code debugging"
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7458
+ /*Default=*/false)) {
+ if (*EmitCodeView && *DebugInfoKind >= codegenoptions::DebugInfoConstructor)
+ CmdArgs.push_back("-fjmc");
----------------
aganea wrote:
> 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
> ```
I think it needs enough debug info to enable MSVC stepping. The ones < DebugInfoConstructor seems not sufficient but I'm entirely sure.
================
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"),
----------------
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`.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:18
+// - (TODO) calls to __CheckForDebuggerJustMyCode should be a scheduling
+// boundary
+//
----------------
hans wrote:
> Is that necessary? I assume nothing interesting would get scheduled across such a call anyway?
Agreed that in practice, it is not very likely to cause trouble. But I still think it is nice to have.
Removed the TODO. Make it a comment down below.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:66
+ // `realpath` on Linux and `GetFullPathName()` on Windows
+ // (https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats#path-normalization).
+ SmallString<256> FilePath(SP.getDirectory());
----------------
hans wrote:
> I don't think we'd want to call realpath or GetFullPathName() as some builds may want to use relative paths, or paths with a specific prefix (see the -fdebug-compilation-dir flag)
Yep, missed that. Comments updated.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:75
+ // __D032E919_file at any@c. The hashing function or the naming convention match
+ // MSVC's behavior however the match is not required to make JMC work.
+
----------------
hans wrote:
> Should probably point out here (or below) that we're not using the same hash as MSVC.
yep. Updated the comment here.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:143
+ Function *Decl = M.getFunction(CheckFunctionName);
+ assert(Decl);
+ Decl->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
----------------
hans wrote:
> What if there are no function definitions in the module? In that case, I think we would not have created the function declaration, and this assert would fail?
>
> I think instead of this micro-optimization, it would be safer set the function attributes etc. when creating the decl, and cache it. It could be a lambda in runOnModule() or maybe a helper method in the class with a member variable to hold the cached decl?
>
> Or it could even just be declared outside the main loop in runOnModule() and in the loop you could do
>
> ```
> if (!CheckFunction) {
> CheckFunction = ...
> add attributes and stuff
> }
> ```
End up using a lambda in runOnModule().
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:156
+ const char *EntryAttr = "jmc-instrumented";
+ const char *CheckFunctionName = "__CheckForDebuggerJustMyCode";
+ bool Is32Bit = Triple(M.getTargetTriple()).getArch() == Triple::x86;
----------------
hans wrote:
> if this and EntryAttr can't change, maybe they should be const, and maybe declared outside the function?
Yes, they don't change. Made them const and move them into the anonymous namespace above.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:157
+ const char *CheckFunctionName = "__CheckForDebuggerJustMyCode";
+ bool Is32Bit = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
aganea wrote:
> 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
> ```
> That's a very x86 specific check. What about other targets? (Also the calling conventions are x86 specific..)
`Is32Bit` -> `UseFastCall` to make it explicit. Only X86 should use this.
@aganea it turns out ARM should just work. I'll add tests for ARM.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:168
+ // the attribute so that it's not inserted again if the pass should happen
+ // to run later for some reason.
+ if (F.hasFnAttribute(EntryAttr))
----------------
hans wrote:
> Do other instrumentation passes consume the attr like this? Why would the pass run again?
>
> Actually it looks like the code does the opposite of consuming: it looks for the attribute, and *adds* it if it's not there..
Initially, I put this pass at the end of the mid-end optimization pipeline which may have this issue for LTO, etc. Now, this pass is in IR CodeGen pipeline. I don't think this is needed anymore. Removed.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:175
+ M.getOrInsertFunction(CheckFunctionName, getCheckFunctionType(Ctx));
+ // FIXME: caching the flag name to prevent repetitive hashing?
+ std::string FlagName = getFlagName(*SP, Is32Bit);
----------------
hans wrote:
> Hashing is cheap, but since we're likely to use the same filename again and again, and it does take some string manipulation, maybe caching the result is a good idea. Actually, could we cache the flag Constant rather than the name?
Yep, used `DenseMap<DISubprogram *, Constant *>`.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:182
+ // Make it static linkage. One of the definition would win, the rest are
+ // dead in .msvcjmc section, but it is supposed be very cheap.
+ GlobalVariable *GV = new GlobalVariable(
----------------
hans wrote:
> I thought static linkage meant they all go into the binary.. how do you mean one of them wins and the others are dead?
Sorry for the confusion. I meant that they all go into the binary, but only one of the bytes is used at run time. Not that they are dead or stripped at linker time.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:190
+ attachDebugInfo(*GV, *SP);
+ appendToUsed(M, {GV});
+ return GV;
----------------
hans wrote:
> is the appendToUsed necessary? It will be used by the call instruction (and if that goes away for some reason, I guess we'd want the variable to go away too?)
Removed. I was afraid that the internal-linkage flags could get removed since they have no users. But they get their address taken by external-linkage `__CheckForDebuggerJustMyCode`, so yeah, no need mark them `used`.
================
Comment at: llvm/test/CodeGen/X86/jmc-instrument-32bit.ll:2
+; RUN: opt -jmc-instrument -S < %s | FileCheck %s
+; RUN: llc < %s -enable-jmc-instrument | FileCheck %s -check-prefix CODEGEN
+
----------------
hans wrote:
> Mixing opt and llc testing like this seems unusual. The pass is really an instrumentation pass, so I suppose llvm/test/Instrumentation/ would be a better place, and I don't think the llc test is needed?
There is a codegen change to make flag symbols not qualified. Hence the llc test.
Make the llc test smaller and move opt tests to llvm/test/Instrumentation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118428/new/
https://reviews.llvm.org/D118428
More information about the llvm-commits
mailing list