Changes to LLVM ExecutionEngine wrapper

[murarth]

• Removes RustJITMemoryManager from public API.
  This was really sort of an implementation detail to begin with.
• __morestack is linked to C++ wrapper code and this pointer
  is used when resolving the symbol for ExecutionEngine code.
• __morestack_addr is also resolved for ExecutionEngine code.
  This function is sometimes referenced in LLVM-generated code,
  but was not able to be resolved on Mac OS systems.
• Added Windows support to ExecutionEngine API.
• Added a test for basic ExecutionEngine functionality.

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[murarth]

I think that this will close #23050 but I want to confirm with Mac users of rusti once this lands.

Also, I'm pretty sure that the use of LLVM ExecutionEngine will fail on Windows. If anyone with Windows and LLVM experience wants to help me solve that, I'll gladly accept their help. Otherwise, perhaps the test could be temporarily disabled for Windows? Not sure how to proceed with that.

[achanda]

The last part should be a #[cfg(not(windows))] for main

[murarth]

@achanda: What I meant to say was that I'm not sure whether disabling a test for Windows is an acceptable solution. Windows is a supported platform for Rust. This API should work on Windows, if at all possible.

@huonw or other core developer: Should I disable the test for Windows and have an issue filed for implementing Windows support?

[alexcrichton]

We haven't had JIT support in the compiler for quite some time now, so could you elaborate on why this is adding a test for it? I'd personally expect these wrappers to get removed.

[murarth]

@alexcrichton: The rustc executable doesn't support JIT (and I can't think of a reason why it should), but that doesn't mean that the rustc crate's llvm module shouldn't provide access to this feature of LLVM. rusti has been using this API and it would be quite a bit more painful (if not impossible) to add the C++ wrapper code into the rusti project and compile/link with LLVM in order to use the ExecutionEngine API.

I added a test because users of rusti have been experiencing an issue which is actually caused by the code that lives in rustc::llvm. It seems reasonable to me that an API provided by the rustc crate ought to have a test for basic functionality.

[huonw]

r? @alexcrichton

[alexcrichton]

@murarth yeah I'd believe that it's easier for the compiler to build in support for this, but this is an extra hunk of code for us to maintain and fix if it breaks. I also don't think the compiler should necessarily already contain support for all sorts of tools and just not expose them in a stable location.

[murarth]

@alexcrichton: I'd be glad to put my name on it and be the one responsible for fixing it whenever it breaks. As it's basically a thin wrapper around the LLVM API, I don't anticipate much future breakage.

As for stability concerns, the use of ExecutionEngine is directly dependent upon public rustc APIs that process Rust code and produce a rustc_llvm::ModuleRef. As such, the stability of these low-level APIs is effectively tied to that of the public rustc API.
I've considered developing a high-level API that could become stable (while internally depending upon unstable APIs), but I don't think that rusti is yet sufficiently feature complete for this to be a viable plan at this time. Implementation of major features will likely require changes to rustc APIs that would enable deeper access to information produced during the compile process.

[alexcrichton]

@murarth sure, but the problem with an API like this that I'm worried about is upgrading LLVM. It's fine to have support for extra features that don't block others, but supporting this would block an LLVM upgrade from otherwise happening (possibly).

cc @rust-lang/compiler, there may be some other opinions about whether this should be supported from the compiler.

[nrc]

I'd be keen to only support what the compiler directly needs. The rustc_* libs shouldn't be used by end users at all, IMO. It would be more appropriate to have a 'good' LLVM wrapper on crates.io, rather than bundling with the compiler.

@murarth is that possible? Or does that break some of your use cases?

[murarth]

@nrc: It might be possible to use an external LLVM crate, but requiring an installation of LLVM is a pretty big dependency for a REPL. Also, I don't know what changes have been applied to Rust's LLVM fork and what problems may arise from interactions between Rust's LLVM and a mainline LLVM library.

The use of rustc* crates is unavoidable. The compiler functions are needed to produce compiled code without linking having taken place. Rusti also makes use of CrateAnalysis and at least one planned feature would require more access to the resolver during compilation.

If a full-featured REPL (which Rusti is not yet) is still a goal for Rust, it seems reasonable to me to provide support for such a thing at least until some future possible breakage actually happens. As long as the rustc_llvm crate remains unstable, there is no guarantee that the ExecutionEngine API must exist or function correctly into the future. I would also note that at least one upgrade of LLVM has happened since the introduction of the ExecutionEngine wrapper API and only a handful of lines were changed in the C++ wrapper code.

[murarth]

After thinking about this a bit more, I don't think using an external LLVM crate is an option at all. Doing so would require passing an LLVM Module structure generated by Rust's LLVM into an external LLVM library with the assumption that Module and all contained data structures have not changed at all.

Edit: The Module structure is also owned by the receiving ExecutionEngine, which has to deallocate it at some point and that could cause problems of its own.

[sanxiyn]

I started a Discourse thread on this.

[alexcrichton]

r? @nrc

[nrc]

@bors r+

[bors]

📌 Commit d81e5bf has been approved by nrc

[bors]

⌛ Testing commit d81e5bf with merge cee2594...

[bors]

💔 Test failed - auto-win-gnu-64-nopt-t

[alexcrichton]

@bors: retry

On Wed, Jun 3, 2015 at 10:46 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/187

—
Reply to this email directly or view it on GitHub
#25627 (comment).

[bors]

⌛ Testing commit d81e5bf with merge 450a3da...

[bors]

💔 Test failed - auto-win-gnu-64-opt

[murarth]

Okay, that one's a real failure. libmorestack isn't linked to the src/rustllvm wrapper code on Windows? Is there a way to fix that?

[murarth]

@nrc: Fixed compile error on Windows (I think).

[nrc]

@bors r+

[bors]

📌 Commit d39d80e has been approved by nrc

[bors]

⌛ Testing commit d39d80e with merge e38dd64...

[bors]

💔 Test failed - auto-win-gnu-64-nopt-t

[murarth]

That was the error I expected on Windows and I believed it's now solved, too.
@nrc: Retry?
Edit: Not retry. I mean try the new one, of course.

[eddyb]

@bors r=nrc

[bors]

📌 Commit 021e483 has been approved by nrc

[bors]

⌛ Testing commit 021e483 with merge b4fc5da...

[bors]

💔 Test failed - auto-linux-64-opt

[murarth]

Never seen an error in the git stage before. Interesting.
@eddyb: retry?

[nrc]

@bors: retry

[bors]

⌛ Testing commit 021e483 with merge 71a8d31...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt