Allow Self::Module to be mutated. #58609

gabi-250 · 2019-02-20T22:07:00Z

This only changes &Self::Module to &mut Self::Module in a couple of places.

codegen_allocator and write_metadata from ExtraBackendMethods mutate the underlying LLVM module. As such, it makes sense for these two functions to receive a mutable reference to the module (as opposed to an immutable one).

I am trying to implement codegen_allocator for my backend, and I need to be able to mutate Self::Module:

rust/src/librustc_codegen_ssa/traits/backend.rs

Line 41 in f66e469

    
           fn codegen_allocator(&self, tcx: TyCtxt, mods: &Self::Module, kind: AllocatorKind);

Modifying the module in codegen_allocator/write_metadata is not a problem for the LLVM backend, because ModuleLlvm contains a raw pointer to the underlying LLVM module, so it can easily be mutated through FFI calls.

I am trying to avoid interior mutability and unsafe as much as I can. What do you think? Does this change make sense, or is there a reason why this should stay the way it is?

`codegen_allocator` and `write_metadata` mutate the underlying LLVM module. As such, it makes sense for these two functions to receive a mutable reference to the module (as opposed to an immutable one).

rust-highfive · 2019-02-20T22:07:05Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

zackmdavis · 2019-02-20T23:22:37Z

With regrets, I don't think I'm the most qualified reviewer for this. Maybe ...

r? @eddyb

oli-obk

The rationale is that the llvm backend actually does mutate some state in

rust/src/librustc_codegen_llvm/allocator.rs

Line 53 in f66e469

let llfn = llvm::LLVMRustGetOrInsertFunction(llmod,

via

http://llvm.org/doxygen/classllvm_1_1Module.html#abb107e4edbf05eae92936cba6801c2d9

Same with write_metadata:

rust/src/librustc_codegen_llvm/base.rs

Line 47 in f66e469

pub fn write_metadata<'a, 'gcx>(

The rabbit hole goes even deeper than that, we should probably start giving functions like

rust/src/librustc_codegen_llvm/llvm/ffi.rs

Line 771 in f66e469

    
           pub fn LLVMAddGlobal(M: &'a Module, Ty: &'a Type, Name: *const c_char) -> &'a Value;

&mut arguments.

oli-obk · 2019-02-21T09:22:18Z

src/librustc_codegen_llvm/lib.rs

    ) -> EncodedMetadata {
        base::write_metadata(tcx, metadata)
    }
-    fn codegen_allocator(&self, tcx: TyCtxt, mods: &ModuleLlvm, kind: AllocatorKind) {
+    fn codegen_allocator(&self, tcx: TyCtxt, mods: &mut ModuleLlvm, kind: AllocatorKind) {
        unsafe { allocator::codegen(tcx, mods, kind) }


please change the codegen function to also take a &mut ModuleLlvm

oli-obk · 2019-02-21T09:25:20Z

src/librustc_codegen_llvm/lib.rs

@@ -120,11 +120,11 @@ impl ExtraBackendMethods for LlvmCodegenBackend {
    fn write_metadata<'b, 'gcx>(
        &self,
        tcx: TyCtxt<'b, 'gcx, 'gcx>,
-        metadata: &ModuleLlvm
+        metadata: &mut ModuleLlvm
    ) -> EncodedMetadata {
        base::write_metadata(tcx, metadata)


write_metadata should also take &mut ModuleLlvm to properly signal the mutability.

oli-obk · 2019-02-21T09:31:04Z

cc @irinagpopa

eddyb · 2019-02-21T11:19:23Z

So the explanation for the FFI signatures (which should've probably been left in a comment somewhere) is that:

&Foo is a shared (mutable) reference
&'a mut Foo<'a> is an "owning" reference

The safe APIs can be changed freely as long as the FFI signatures don't need to be adjusted.

gabi-250 · 2019-02-21T13:43:57Z

@eddyb @oli-obk Thank you for reviewing this!

I've addressed your comments in my last commit.

oli-obk · 2019-02-21T15:22:20Z

@bors r+

Thanks!

bors · 2019-02-21T15:22:21Z

📌 Commit e5d1fa5 has been approved by oli-obk

Allow Self::Module to be mutated. This only changes `&Self::Module` to `&mut Self::Module` in a couple of places. `codegen_allocator` and `write_metadata` from `ExtraBackendMethods` mutate the underlying LLVM module. As such, it makes sense for these two functions to receive a mutable reference to the module (as opposed to an immutable one). I am trying to implement `codegen_allocator` for my backend, and I need to be able to mutate `Self::Module`: https://github.com/rust-lang/rust/blob/f66e4697ae286985ddefc53c3a047614568458bb/src/librustc_codegen_ssa/traits/backend.rs#L41 Modifying the module in `codegen_allocator`/`write_metadata` is not a problem for the LLVM backend, because [ModuleLlvm](https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_llvm/lib.rs#L357) contains a raw pointer to the underlying LLVM module, so it can easily be mutated through FFI calls. I am trying to avoid interior mutability and `unsafe` as much as I can. What do you think? Does this change make sense, or is there a reason why this should stay the way it is?

@ghost

Rollup of 16 pull requests Successful merges: - #58100 (Transition librustdoc to Rust 2018) - #58122 (RangeInclusive internal iteration performance improvement.) - #58199 (Add better error message for partial move) - #58227 (Updated RELEASES.md for 1.33.0) - #58353 (Check the Self-type of inherent associated constants) - #58453 (SGX target: fix panic = abort) - #58476 (Remove `LazyTokenStream`.) - #58526 (Special suggestion for illegal unicode curly quote pairs) - #58595 (Turn duration consts into associated consts) - #58609 (Allow Self::Module to be mutated.) - #58628 (Optimise vec![false; N] to zero-alloc) - #58643 (Don't generate minification variables if minification disabled) - #58648 (Update tests to account for cross-platform testing and miri.) - #58654 (Do not underflow after resetting unmatched braces count) - #58658 (Add expected/provided byte alignments to validation error message) - #58667 (Reduce Miri-related Code Repetition `like (n << amt) >> amt`) Failed merges: r? @ghost

Allow Self::Module to be mutated.

36f18f2

`codegen_allocator` and `write_metadata` mutate the underlying LLVM module. As such, it makes sense for these two functions to receive a mutable reference to the module (as opposed to an immutable one).

rust-highfive assigned zackmdavis Feb 20, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2019

rust-highfive assigned eddyb and unassigned zackmdavis Feb 20, 2019

oli-obk reviewed Feb 21, 2019

View reviewed changes

eddyb approved these changes Feb 21, 2019

View reviewed changes

codegen and write_metadata can mutate ModuleLLvm.

e5d1fa5

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2019

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58664

Closed

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58665

Closed

Centril mentioned this pull request Feb 23, 2019

Rollup of 15 pull requests #58666

Closed

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58668

Closed

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58669

Merged

bors merged commit e5d1fa5 into rust-lang:master Feb 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Self::Module to be mutated. #58609

Allow Self::Module to be mutated. #58609

gabi-250 commented Feb 20, 2019

rust-highfive commented Feb 20, 2019

zackmdavis commented Feb 20, 2019

oli-obk left a comment

oli-obk Feb 21, 2019

oli-obk Feb 21, 2019

oli-obk commented Feb 21, 2019

eddyb commented Feb 21, 2019

gabi-250 commented Feb 21, 2019

oli-obk commented Feb 21, 2019

bors commented Feb 21, 2019

Allow Self::Module to be mutated. #58609

Allow Self::Module to be mutated. #58609

Conversation

gabi-250 commented Feb 20, 2019

rust-highfive commented Feb 20, 2019

zackmdavis commented Feb 20, 2019

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk Feb 21, 2019

Choose a reason for hiding this comment

oli-obk Feb 21, 2019

Choose a reason for hiding this comment

oli-obk commented Feb 21, 2019

eddyb commented Feb 21, 2019

gabi-250 commented Feb 21, 2019

oli-obk commented Feb 21, 2019

bors commented Feb 21, 2019