Skip to content

Commit

Permalink
Protect shared JIT variables from being modified unsafely (#44914)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pchintalapudi
pchintalapudi authored Apr 19, 2022
1 parent b5871eb commit ab11173
Show file tree
Hide file tree
Showing 7 changed files with 179 additions and 93 deletions.
2 changes: 2 additions & 0 deletions doc/src/devdocs/locks.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ The following are definitely leaf locks (level 1), and must not try to acquire a
> * flisp
> * jl_in_stackwalk (Win32)
> * ResourcePool<?>::mutex
> * RLST_mutex
> * jl_locked_stream::mutex
>
> > flisp itself is already threadsafe, this lock only protects the `jl_ast_context_list_t` pool
> > likewise, the ResourcePool<?>::mutexes just protect the associated resource pool
Expand Down
28 changes: 14 additions & 14 deletions src/aotcompile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,11 @@ void jl_dump_native_impl(void *native_code,
TheTriple.setOS(llvm::Triple::MacOSX);
#endif
std::unique_ptr<TargetMachine> TM(
jl_ExecutionEngine->getTargetMachine().getTarget().createTargetMachine(
jl_ExecutionEngine->getTarget().createTargetMachine(
TheTriple.getTriple(),
jl_ExecutionEngine->getTargetMachine().getTargetCPU(),
jl_ExecutionEngine->getTargetMachine().getTargetFeatureString(),
jl_ExecutionEngine->getTargetMachine().Options,
jl_ExecutionEngine->getTargetCPU(),
jl_ExecutionEngine->getTargetFeatureString(),
jl_ExecutionEngine->getTargetOptions(),
#if defined(_OS_LINUX_) || defined(_OS_FREEBSD_)
Reloc::PIC_,
#else
Expand All @@ -481,7 +481,7 @@ void jl_dump_native_impl(void *native_code,
));

legacy::PassManager PM;
addTargetPasses(&PM, TM.get());
addTargetPasses(&PM, TM->getTargetTriple(), TM->getTargetIRAnalysis());

// set up optimization passes
SmallVector<char, 0> bc_Buffer;
Expand All @@ -502,7 +502,7 @@ void jl_dump_native_impl(void *native_code,
PM.add(createBitcodeWriterPass(unopt_bc_OS));
if (bc_fname || obj_fname || asm_fname) {
addOptimizationPasses(&PM, jl_options.opt_level, true, true);
addMachinePasses(&PM, TM.get(), jl_options.opt_level);
addMachinePasses(&PM, jl_options.opt_level);
}
if (bc_fname)
PM.add(createBitcodeWriterPass(bc_OS));
Expand Down Expand Up @@ -595,14 +595,14 @@ void jl_dump_native_impl(void *native_code,
delete data;
}

void addTargetPasses(legacy::PassManagerBase *PM, TargetMachine *TM)
void addTargetPasses(legacy::PassManagerBase *PM, const Triple &triple, TargetIRAnalysis analysis)
{
PM->add(new TargetLibraryInfoWrapperPass(Triple(TM->getTargetTriple())));
PM->add(createTargetTransformInfoWrapperPass(TM->getTargetIRAnalysis()));
PM->add(new TargetLibraryInfoWrapperPass(triple));
PM->add(createTargetTransformInfoWrapperPass(std::move(analysis)));
}


void addMachinePasses(legacy::PassManagerBase *PM, TargetMachine *TM, int optlevel)
void addMachinePasses(legacy::PassManagerBase *PM, int optlevel)
{
// TODO: don't do this on CPUs that natively support Float16
PM->add(createDemoteFloat16Pass());
Expand Down Expand Up @@ -857,9 +857,9 @@ class JuliaPipeline : public Pass {
(void)jl_init_llvm();
PMTopLevelManager *TPM = Stack.top()->getTopLevelManager();
TPMAdapter Adapter(TPM);
addTargetPasses(&Adapter, &jl_ExecutionEngine->getTargetMachine());
addTargetPasses(&Adapter, jl_ExecutionEngine->getTargetTriple(), jl_ExecutionEngine->getTargetIRAnalysis());
addOptimizationPasses(&Adapter, OptLevel, true, dump_native, true);
addMachinePasses(&Adapter, &jl_ExecutionEngine->getTargetMachine(), OptLevel);
addMachinePasses(&Adapter, OptLevel);
}
JuliaPipeline() : Pass(PT_PassManager, ID) {}
Pass *createPrinterPass(raw_ostream &O, const std::string &Banner) const override {
Expand Down Expand Up @@ -993,9 +993,9 @@ void *jl_get_llvmf_defn_impl(jl_method_instance_t *mi, size_t world, char getwra
static legacy::PassManager *PM;
if (!PM) {
PM = new legacy::PassManager();
addTargetPasses(PM, &jl_ExecutionEngine->getTargetMachine());
addTargetPasses(PM, jl_ExecutionEngine->getTargetTriple(), jl_ExecutionEngine->getTargetIRAnalysis());
addOptimizationPasses(PM, jl_options.opt_level);
addMachinePasses(PM, &jl_ExecutionEngine->getTargetMachine(), jl_options.opt_level);
addMachinePasses(PM, jl_options.opt_level);
}

// get the source code for this function
Expand Down
12 changes: 6 additions & 6 deletions src/codegen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,10 @@ typedef Instruction TerminatorInst;
#include "processor.h"
#include "julia_assert.h"

JL_STREAM *dump_emitted_mi_name_stream = NULL;
extern "C" JL_DLLEXPORT
void jl_dump_emitted_mi_name_impl(void *s)
{
dump_emitted_mi_name_stream = (JL_STREAM*)s;
**jl_ExecutionEngine->get_dump_emitted_mi_name_stream() = (JL_STREAM*)s;
}

extern "C" {
Expand Down Expand Up @@ -7978,15 +7977,16 @@ jl_llvm_functions_t jl_emit_code(
"functions compiled with custom codegen params must not be cached");
JL_TRY {
decls = emit_function(m, li, src, jlrettype, params);
if (dump_emitted_mi_name_stream != NULL) {
jl_printf(dump_emitted_mi_name_stream, "%s\t", decls.specFunctionObject.c_str());
auto stream = *jl_ExecutionEngine->get_dump_emitted_mi_name_stream();
if (stream) {
jl_printf(stream, "%s\t", decls.specFunctionObject.c_str());
// NOTE: We print the Type Tuple without surrounding quotes, because the quotes
// break CSV parsing if there are any internal quotes in the Type name (e.g. in
// Symbol("...")). The \t delineator should be enough to ensure whitespace is
// handled correctly. (And we don't need to worry about any tabs in the printed
// string, because tabs are printed as "\t" by `show`.)
jl_static_show(dump_emitted_mi_name_stream, li->specTypes);
jl_printf(dump_emitted_mi_name_stream, "\n");
jl_static_show(stream, li->specTypes);
jl_printf(stream, "\n");
}
}
JL_CATCH {
Expand Down
5 changes: 3 additions & 2 deletions src/disasm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1208,9 +1208,10 @@ jl_value_t *jl_dump_function_asm_impl(void *F, char raw_mc, const char* asm_vari
f2.deleteBody();
}
});
LLVMTargetMachine *TM = static_cast<LLVMTargetMachine*>(&jl_ExecutionEngine->getTargetMachine());
auto TMBase = jl_ExecutionEngine->cloneTargetMachine();
LLVMTargetMachine *TM = static_cast<LLVMTargetMachine*>(TMBase.get());
legacy::PassManager PM;
addTargetPasses(&PM, TM);
addTargetPasses(&PM, TM->getTargetTriple(), TM->getTargetIRAnalysis());
if (raw_mc) {
raw_svector_ostream obj_OS(ObjBufferSV);
if (TM->addPassesToEmitFile(PM, obj_OS, nullptr, CGFT_ObjectFile, false, nullptr))
Expand Down
143 changes: 84 additions & 59 deletions src/jitlayers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,15 @@ using namespace llvm;
#define DEBUG_TYPE "jitlayers"

// Snooping on which functions are being compiled, and how long it takes
JL_STREAM *dump_compiles_stream = NULL;
extern "C" JL_DLLEXPORT
void jl_dump_compiles_impl(void *s)
{
dump_compiles_stream = (JL_STREAM*)s;
**jl_ExecutionEngine->get_dump_compiles_stream() = (JL_STREAM*)s;
}
JL_STREAM *dump_llvm_opt_stream = NULL;
extern "C" JL_DLLEXPORT
void jl_dump_llvm_opt_impl(void *s)
{
dump_llvm_opt_stream = (JL_STREAM*)s;
**jl_ExecutionEngine->get_dump_llvm_opt_stream() = (JL_STREAM*)s;
}

static void jl_add_to_ee(orc::ThreadSafeModule &M, StringMap<orc::ThreadSafeModule*> &NewExports);
Expand Down Expand Up @@ -108,7 +106,8 @@ static jl_callptr_t _jl_compile_codeinst(
// caller must hold codegen_lock
// and have disabled finalizers
uint64_t start_time = 0;
if (dump_compiles_stream != NULL)
bool timed = !!*jl_ExecutionEngine->get_dump_compiles_stream();
if (timed)
start_time = jl_hrtime();

assert(jl_is_code_instance(codeinst));
Expand Down Expand Up @@ -198,17 +197,18 @@ static jl_callptr_t _jl_compile_codeinst(
}

uint64_t end_time = 0;
if (dump_compiles_stream != NULL)
if (timed)
end_time = jl_hrtime();

// If logging of the compilation stream is enabled,
// then dump the method-instance specialization type to the stream
jl_method_instance_t *mi = codeinst->def;
if (jl_is_method(mi->def.method)) {
if (dump_compiles_stream != NULL) {
jl_printf(dump_compiles_stream, "%" PRIu64 "\t\"", end_time - start_time);
jl_static_show(dump_compiles_stream, mi->specTypes);
jl_printf(dump_compiles_stream, "\"\n");
auto stream = *jl_ExecutionEngine->get_dump_compiles_stream();
if (stream) {
jl_printf(stream, "%" PRIu64 "\t\"", end_time - start_time);
jl_static_show(stream, mi->specTypes);
jl_printf(stream, "\"\n");
}
}
return fptr;
Expand Down Expand Up @@ -480,13 +480,6 @@ CodeGenOpt::Level CodeGenOptLevelFor(int optlevel)
#endif
}

static void addPassesForOptLevel(legacy::PassManager &PM, TargetMachine &TM, int optlevel)
{
addTargetPasses(&PM, &TM);
addOptimizationPasses(&PM, optlevel);
addMachinePasses(&PM, &TM, optlevel);
}

static auto countBasicBlocks(const Function &F)
{
return std::distance(F.begin(), F.end());
Expand Down Expand Up @@ -899,7 +892,9 @@ namespace {
}
std::unique_ptr<legacy::PassManager> operator()() {
auto PM = std::make_unique<legacy::PassManager>();
addPassesForOptLevel(*PM, *TM, optlevel);
addTargetPasses(PM.get(), TM->getTargetTriple(), TM->getTargetIRAnalysis());
addOptimizationPasses(PM.get(), optlevel);
addMachinePasses(PM.get(), optlevel);
return PM;
}
};
Expand All @@ -910,24 +905,27 @@ namespace {
OptimizerResultT operator()(orc::ThreadSafeModule TSM, orc::MaterializationResponsibility &R) {
TSM.withModuleDo([&](Module &M) {
uint64_t start_time = 0;
if (dump_llvm_opt_stream != NULL) {
// Print LLVM function statistics _before_ optimization
// Print all the information about this invocation as a YAML object
jl_printf(dump_llvm_opt_stream, "- \n");
// We print the name and some statistics for each function in the module, both
// before optimization and again afterwards.
jl_printf(dump_llvm_opt_stream, " before: \n");
for (auto &F : M.functions()) {
if (F.isDeclaration() || F.getName().startswith("jfptr_")) {
continue;
{
auto stream = *jl_ExecutionEngine->get_dump_llvm_opt_stream();
if (stream) {
// Print LLVM function statistics _before_ optimization
// Print all the information about this invocation as a YAML object
jl_printf(stream, "- \n");
// We print the name and some statistics for each function in the module, both
// before optimization and again afterwards.
jl_printf(stream, " before: \n");
for (auto &F : M.functions()) {
if (F.isDeclaration() || F.getName().startswith("jfptr_")) {
continue;
}
// Each function is printed as a YAML object with several attributes
jl_printf(stream, " \"%s\":\n", F.getName().str().c_str());
jl_printf(stream, " instructions: %u\n", F.getInstructionCount());
jl_printf(stream, " basicblocks: %lu\n", countBasicBlocks(F));
}
// Each function is printed as a YAML object with several attributes
jl_printf(dump_llvm_opt_stream, " \"%s\":\n", F.getName().str().c_str());
jl_printf(dump_llvm_opt_stream, " instructions: %u\n", F.getInstructionCount());
jl_printf(dump_llvm_opt_stream, " basicblocks: %lu\n", countBasicBlocks(F));
}

start_time = jl_hrtime();
start_time = jl_hrtime();
}
}

JL_TIMING(LLVM_OPT);
Expand All @@ -936,20 +934,23 @@ namespace {
(***PMs).run(M);

uint64_t end_time = 0;
if (dump_llvm_opt_stream != NULL) {
end_time = jl_hrtime();
jl_printf(dump_llvm_opt_stream, " time_ns: %" PRIu64 "\n", end_time - start_time);
jl_printf(dump_llvm_opt_stream, " optlevel: %d\n", optlevel);

// Print LLVM function statistics _after_ optimization
jl_printf(dump_llvm_opt_stream, " after: \n");
for (auto &F : M.functions()) {
if (F.isDeclaration() || F.getName().startswith("jfptr_")) {
continue;
{
auto stream = *jl_ExecutionEngine->get_dump_llvm_opt_stream();
if (stream) {
end_time = jl_hrtime();
jl_printf(stream, " time_ns: %" PRIu64 "\n", end_time - start_time);
jl_printf(stream, " optlevel: %d\n", optlevel);

// Print LLVM function statistics _after_ optimization
jl_printf(stream, " after: \n");
for (auto &F : M.functions()) {
if (F.isDeclaration() || F.getName().startswith("jfptr_")) {
continue;
}
jl_printf(stream, " \"%s\":\n", F.getName().str().c_str());
jl_printf(stream, " instructions: %u\n", F.getInstructionCount());
jl_printf(stream, " basicblocks: %lu\n", countBasicBlocks(F));
}
jl_printf(dump_llvm_opt_stream, " \"%s\":\n", F.getName().str().c_str());
jl_printf(dump_llvm_opt_stream, " instructions: %u\n", F.getInstructionCount());
jl_printf(dump_llvm_opt_stream, " basicblocks: %lu\n", countBasicBlocks(F));
}
}
});
Expand Down Expand Up @@ -1166,7 +1167,7 @@ uint64_t JuliaOJIT::getFunctionAddress(StringRef Name)

StringRef JuliaOJIT::getFunctionAtAddress(uint64_t Addr, jl_code_instance_t *codeinst)
{
static int globalUnique = 0;
std::lock_guard<std::mutex> lock(RLST_mutex);
std::string *fname = &ReverseLocalSymbolTable[(void*)(uintptr_t)Addr];
if (fname->empty()) {
std::string string_fname;
Expand All @@ -1186,7 +1187,7 @@ StringRef JuliaOJIT::getFunctionAtAddress(uint64_t Addr, jl_code_instance_t *cod
stream_fname << "jlsys_";
}
const char* unadorned_name = jl_symbol_name(codeinst->def->def.method->name);
stream_fname << unadorned_name << "_" << globalUnique++;
stream_fname << unadorned_name << "_" << RLST_inc++;
*fname = std::move(stream_fname.str()); // store to ReverseLocalSymbolTable
addGlobalMapping(*fname, Addr);
}
Expand Down Expand Up @@ -1232,16 +1233,6 @@ const DataLayout& JuliaOJIT::getDataLayout() const
return DL;
}

TargetMachine &JuliaOJIT::getTargetMachine()
{
return *TM;
}

const Triple& JuliaOJIT::getTargetTriple() const
{
return TM->getTargetTriple();
}

std::string JuliaOJIT::getMangledName(StringRef Name)
{
SmallString<128> FullName;
Expand Down Expand Up @@ -1412,6 +1403,40 @@ void JuliaOJIT::shareStrings(Module &M)
GV->eraseFromParent();
}

//TargetMachine pass-through methods

std::unique_ptr<TargetMachine> JuliaOJIT::cloneTargetMachine() const
{
return std::unique_ptr<TargetMachine>(getTarget()
.createTargetMachine(
getTargetTriple().str(),
getTargetCPU(),
getTargetFeatureString(),
getTargetOptions(),
TM->getRelocationModel(),
TM->getCodeModel(),
TM->getOptLevel()));
}

const Triple& JuliaOJIT::getTargetTriple() const {
return TM->getTargetTriple();
}
StringRef JuliaOJIT::getTargetFeatureString() const {
return TM->getTargetFeatureString();
}
StringRef JuliaOJIT::getTargetCPU() const {
return TM->getTargetCPU();
}
const TargetOptions &JuliaOJIT::getTargetOptions() const {
return TM->Options;
}
const Target &JuliaOJIT::getTarget() const {
return TM->getTarget();
}
TargetIRAnalysis JuliaOJIT::getTargetIRAnalysis() const {
return TM->getTargetIRAnalysis();
}

static void jl_decorate_module(Module &M) {
#if defined(_CPU_X86_64_) && defined(_OS_WINDOWS_)
// Add special values used by debuginfo to build the UnwindData table registration for Win64
Expand Down
Loading

9 comments on commit ab11173

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@DilumAluthge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vtjnash Looks like PkgEval is broken?

@maleadt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've restarted the service, let's try again:

@nanosoldier runtests(ALL, isdaily = true)

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanosoldier runbenchmarks(ALL, isdaily = true)

@maleadt
Copy link
Member

@maleadt maleadt commented on ab11173 Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't help / That won't help. #44883 broke PkgEval.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pkg bug filed: JuliaLang/Pkg.jl#3062

The problem appears to be that we may transparently call close(in) on you for IO objects. Previously that might sometimes work and rarely fail here, or it might access memory after freeing. Now it will always fail here. We could carefully swap in devnull if we observe it will fail (since we can hold the lock on that, which you are prohibited from holding there), but I am not certain that is a desired improvement. We really should improve the error message when we detect that this may fail though. Currently rawhandle has not even a check_open call to detect the race-free cases of this occuring.

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.