[PATCH] D118428: [clang-cl] Support the /JMC flag
Hans Wennborg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 31 05:45:51 PST 2022
hans added a comment.
This is great stuff, thanks!
================
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
----------------
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?
================
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">;
----------------
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"?)
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7458
+ /*Default=*/false)) {
+ if (*EmitCodeView && *DebugInfoKind >= codegenoptions::DebugInfoConstructor)
+ CmdArgs.push_back("-fjmc");
----------------
Why that level specifically and not e.g. *DebugInfoKind > NoDebugInfo?
================
Comment at: clang/test/Driver/cl-options.c:775
+// RUN: %clang_cl /JMC /c -- %s 2>&1 | FileCheck %s --check-prefix JMCWARN
+// JMCWARN: /JMC requires debug info. Use '/Zi', '/Z7' or other debug options; option ignored
+
----------------
-### is missing from this run line?
================
Comment at: clang/test/Driver/cl-options.c:779
+// RUN: %clang_cl /JMC /Z7 /JMC- /c -### -- %s 2>&1 | FileCheck %s --check-prefix JMC
+// JMC-NOT: -fjmc
+
----------------
It seemed odd to me that the JMC check prefix is used for tests which check that JMC is *not* enabled.
How about using "NOJMC" for these, and "JMC" for the test below where it's enabled?
================
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"),
----------------
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?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:18
+// - (TODO) calls to __CheckForDebuggerJustMyCode should be a scheduling
+// boundary
+//
----------------
Is that necessary? I assume nothing interesting would get scheduled across such a call anyway?
================
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());
----------------
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)
================
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.
+
----------------
Should probably point out here (or below) that we're not using the same hash as MSVC.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:111
+
+void createDefaultCheckFunction(Module &M, const char *CheckFunctionName,
+ bool Is32Bit) {
----------------
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?)
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:143
+ Function *Decl = M.getFunction(CheckFunctionName);
+ assert(Decl);
+ Decl->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
----------------
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
}
```
================
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;
----------------
if this and EntryAttr can't change, maybe they should be const, and maybe declared outside the function?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:157
+ const char *CheckFunctionName = "__CheckForDebuggerJustMyCode";
+ bool Is32Bit = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
That's a very x86 specific check. What about other targets? (Also the calling conventions are x86 specific..)
================
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))
----------------
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..
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:174
+ FunctionCallee Fn =
+ M.getOrInsertFunction(CheckFunctionName, getCheckFunctionType(Ctx));
+ // FIXME: caching the flag name to prevent repetitive hashing?
----------------
As mentioned above, the decl could be cached instead, there's no need to look it up each time.
================
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);
----------------
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?
================
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(
----------------
I thought static linkage meant they all go into the binary.. how do you mean one of them wins and the others are dead?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:190
+ attachDebugInfo(*GV, *SP);
+ appendToUsed(M, {GV});
+ return GV;
----------------
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?)
================
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
+
----------------
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?
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