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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
