[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

[Yuanfang Chen via Phabricator via cfe-commits]

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