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