ychen added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5372
+ // This controls whether or not we perform JustMyCode instrumentation.
+ if (TC.getTriple().isOSBinFormatELF() && Args.hasArg(options::OPT_fjmc)) {
+ if (DebugInfoKind >= codegenoptions::DebugInfoConstructor)
----------------
rnk wrote:
> Generally all flags have an fno- variant. Normally, this would be
> `hasFlag(OPT_fjmc, OPT_fno_jmc, false)`, which handles the behavior of making
> the last flag win. Any reason not to set that up?
I missed that. Before this port, `fjmc` is supposed to be used as cc1 option
only. With this port, it is also a driver option now. I'll add the no-variant.
================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:177
+ DefaultCheckFunc->setName(CheckFunctionName);
+ DefaultCheckFunc->setLinkage(GlobalValue::WeakODRLinkage);
+ CheckFunction = DefaultCheckFunc;
----------------
rnk wrote:
> This is a bit pedantic, but the idea is that this weak function will be
> overridden with a strong function, right? Technically, for that use case,
> `weak` linkage is the correct linkage. Because the JMC pass runs late after
> inlining, it is unlikely that this will ever cause issues in practice, but I
> think it expresses the intention better to use the linkage that matches the
> use case.
>
> ODR linkage is supposed to indicate that all definitions must be functionally
> equivalent. It just turns out that the only real power that grants the
> optimizer is the ability to inline.
Agreed. I'll change it to `weak` linkage.
================
Comment at: llvm/test/Instrumentation/JustMyCode/jmc-instrument-elf.ll:1
-; REQUIRES: system-windows
-; RUN: opt -jmc-instrument -mtriple=x86_64-pc-windows-msvc -S < %s |
FileCheck %s
-; RUN: opt -jmc-instrument -mtriple=aarch64-pc-windows-msvc -S < %s |
FileCheck %s
-; RUN: opt -jmc-instrument -mtriple=arm-pc-windows-msvc -S < %s |
FileCheck %s
+; REQUIRES: system-linux
+; RUN: opt -jmc-instrument -mtriple=x86_64-unknown-linux-gnu -S < %s |
FileCheck %s
----------------
rnk wrote:
> Please remove the REQUIRES line, this test should pass on Windows, and the
> other test should pass on Linux as well. Neither of these tests depend on
> anything from the system.
Yeah, this is a little tricky. It is about the host rather than the target. The
flag symbol name is computed using `sys::path` functions. It is different
between Windows and Linux. For testing purposes, I need to anchor them to a
specific system to check the result. I could've made this `; REQUIRES:
system-windows` and check the result, I thought it is more intuitive to test
the ELF on Linux.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119910/new/
https://reviews.llvm.org/D119910
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
