[PATCH] D118428: [clang-cl] Support the /JMC flag
Hans Wennborg via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 1 06:23:19 PST 2022
hans added inline comments.
================
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"),
----------------
ychen wrote:
> 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`.
Maybe the fact that -jmc-instrument is used by the IR pass is a hint that there shouldn't be an llc option for this, then? Looking at the other options here, they're all about codegen features, whereas the instrumentation in this patch really happens at a higher level.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:111
+
+void createDefaultCheckFunction(Module &M, const char *CheckFunctionName,
+ bool Is32Bit) {
----------------
aganea wrote:
> 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 □..
> ```
Thanks, I see. Doing the same as MSVC seems fine, but I'm curious why they chose this approach.. as far as I understand, JMC will not work for the binary in this scenario, so avoiding the link error is not obviously better.
================
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);
----------------
ychen wrote:
> 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 *>`.
Nice!
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145
+ LLVMContext &Ctx = M.getContext();
+ bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86;
+
----------------
I still worry a bit about the target-specific code here. Normally, IR passes don't have any target-specific knowledge, but ask classes such as TargetTransformInfo for target-specific details, or possibly take them as input to the pass. For example, see llvm/lib/Transforms/CFGuard/CFGuard.cpp
I'm also not sure that lib/CodeGen/ is the right place for this pass, since most files there seem to be machine-IR passes. Maybe the natural place for this would be lib/Transforms/Instrumentation/? Is there some good pass we can compare this with?
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:147
+
+ auto GetOrCreateCheckFunctionDecl = [&, Decl = FunctionCallee()]() mutable {
+ if (Decl)
----------------
This kind of mutable lambda feels pretty subtle to me. I think something like the below would be more straight-forward.
```
Function *CheckFunction = nullptr;
for (...) {
...
if (!CheckFunction) {
// Create the decl here.
}
}
```
But if you strongly prefer the lambda, I suppose it's okay.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:163
+
+ auto GetOrCreateFlag = [&, SavedFlags = DenseMap<DISubprogram *, Constant *>(
+ 8)](DISubprogram &SP) mutable {
----------------
Same comment as for the lambda above. I think it would be more straight-forward to just keep the DenseMap outside the loop and just put the code to use/create the Constant in the loop.
================
Comment at: llvm/test/CodeGen/X86/jmc-instrument.ll:2
+; Check that the flag symbol is not full-qualified.
+; RUN: llc < %s -enable-jmc-instrument | FileCheck %s
+
----------------
Since it's an IR pass, I think we don't need to have an llc test at all.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118428/new/
https://reviews.llvm.org/D118428
More information about the cfe-commits
mailing list