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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.
Herald added a project: All.


================
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)
----------------
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?


================
Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:177
+        DefaultCheckFunc->setName(CheckFunctionName);
+        DefaultCheckFunc->setLinkage(GlobalValue::WeakODRLinkage);
+        CheckFunction = DefaultCheckFunc;
----------------
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.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119910/new/

https://reviews.llvm.org/D119910