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@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@cpp
> 000000000000000C: 91000100 add x0,x8,__BB04C4AD_main@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
