Resolve duplicate crate dependencies (support multiple versions) #430

avanhatt · 2021-08-17T14:38:55Z

Description of changes:

When building with Cargo RMC, you can run into name conflicts with global allocations if two different versions of a crate are pulled into dependencies.

There are two different problems that this PR addresses:

Global allocations.
Struct types.

Global allocations

We should use the full crate name, including a mangled versioning info string, when naming allocations.

Example:

Before:
Name: getrandom::alloc174
Error:

error: conflicting array sizes for variable 'getrandom::alloc174'
old definition in module '' 
struct getrandom::alloc174::struct {
  unsigned char [10] 0;
}
new definition in module '' 
struct getrandom::alloc174::struct {
  unsigned char [15] 0;
}

reason for conflict at #this.0: array types differ

unsigned char [10]
unsigned char [15]

After: 1vemgcjvp3r154gr::getrandom::alloc174

Struct types

If the same struct is defined differently across versions of a crate, the C we generate has conflicts. This PR now adds the unique type ID, which differs across crates, to the type mangled name.

Before:

reason for conflict at #this.ptr.pointer->data: number of members is different (2/3)

struct mmap::GuestRegionMmap {
  struct mmap_unix::MmapRegion mapping;
  struct guest_memory::GuestAddress guest_base;
}
struct mmap::GuestRegionMmap {
  struct vm_memory::MmapRegion mapping;
  struct vm_memory::GuestAddress guest_base;
  struct std::option::Option<bitmap::Bitmap> dirty_bitmap;
}

After: mmap::GuestRegionMmap::3024063347542320770

Resolved issues:

Resolves #429

Testing:

How is this change tested?

Added a new test that creates a diamond crate dependency, where two dependencies use different version of a crate with the same name. Run as its own script since it needs Cargo RMC.

#         dependency1
#        /           \ v0.1.0
#   main             dependency3
#        \           / v0.1.1
#         dependency2

Also manually running on a (private) codebase with this issue.

Is this a refactor change?

No.

Checklist

Each commit message has a non-empty body, explaining why the change was made
Methods or procedures are documented
Regression or unit tests are included, or existing tests cover the modified code
My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

danielsn · 2021-08-19T18:42:17Z

.gitignore

@@ -80,3 +80,5 @@ src/test/rustdoc-gui/src/**.lock
 /.ninja_deps
 /.ninja_log
 /src/test/dashboard
+**Cargo.lock


Do you need **

danielsn · 2021-08-19T18:43:31Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

@@ -54,13 +57,15 @@ pub struct GotocCtx<'tcx> {
 impl<'tcx> GotocCtx<'tcx> {
    pub fn new(tcx: TyCtxt<'tcx>) -> GotocCtx<'tcx> {
        let (thks, fhks) = type_and_fn_hooks();
+        let crate_name = full_crate_name(tcx);


Why not just call this full_crate_name

danielsn · 2021-08-19T18:44:17Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

        self.tcx.crate_name(LOCAL_CRATE).to_string()
    }

+    /// The full crate name including versioning info
+    pub fn full_crate_name(&self) -> String {
+        self.full_crate_name.clone()


Do we want to clone here? Or should we return &str?

danielsn · 2021-08-19T18:49:57Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

@@ -268,6 +279,21 @@ impl MetadataLoader for GotocMetadataLoader {
    }
 }

+/// The full crate name should use the Codegen Unit builder to include full name resolution,


Is this where this function belongs?

I could see an argument for either, where it's used for global names, or compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/utils/names.rs. Maybe the right thing is to move both the global name stuff and this to names.rs, thoughts?

Yeah, I think that makes sense

src/test/rmc-dependency-test/dependency3/src/lib.rs

danielsn · 2021-08-19T18:54:47Z

src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh

@@ -0,0 +1,39 @@
+#!/bin/bash


https://stackoverflow.com/questions/21612980/why-is-usr-bin-env-bash-superior-to-bin-bash

danielsn · 2021-08-19T18:55:09Z

src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh

+# Test for platform
+platform=`uname -sp`
+if [[ $platform != "Linux x86_64" ]]; then
+  echo "Codegen script only works on Linux x86 platform"


What happens on mac?

With the given command, it fails to find std:

error[E0463]: can't find crate for `std` | = note: the `x86_64-unknown-linux-gnu` target may not be installed = help: consider downloading the target with `rustup target add x86_64-unknown-linux-gnu` = help: consider building the standard library from source with `cargo build -Zbuild-std`

If you try to build std (using nightly), you hit ICE's within building std. This block to skip is copied from the Firecrackers regression test.

danielsn · 2021-08-19T18:55:32Z

src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh

+
+# Compile crates with RMC backend
+cd $(dirname $0)
+rm -rf build


does cargo rmc not work

it fails with:

rmc (dependencies) $ ./src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh ERROR: Unexpected number of json outputs: 5

It seems like the script for it assumes a single symbol table output, but here we get one per dependency and main.

Lets file a ticket for that

danielsn

Approved modulo comments.

danielsn · 2021-08-25T13:25:58Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/codegen/typ.rs

+        // of the same crate that could be pulled in by dependencies. However, RMC's
+        // treatment of FFI C calls asssumes that we generate the same name for structs
+        // as the C name, so don't mangle in that case.
+        let is_repr_c_adt = match *t.kind() {


This might be a useful helper function

Good idea, moved.

danielsn · 2021-08-25T13:26:54Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/codegen/typ.rs

+        // Crate resolution: mangled names need to be distinct across different versions
+        // of the same crate that could be pulled in by dependencies. However, RMC's
+        // treatment of FFI C calls asssumes that we generate the same name for structs
+        // as the C name, so don't mangle in that case.


This is a hack. We should note it as such, and have a tracking issue to do the right thing here.

Filed and adding

danielsn · 2021-08-25T13:27:43Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

        self.tcx.crate_name(LOCAL_CRATE).to_string()
    }

+    /// The full crate name including versioning info
+    pub fn full_crate_name(&self) -> &str {
+        &self.full_crate_name


Why do we cache this one and not the other one?

One we pull directly from MIR, the other will build from two parts. Maybe with the to_string in the first case it's the same, though?

danielsn · 2021-08-25T13:28:09Z

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs

@@ -268,6 +279,21 @@ impl MetadataLoader for GotocMetadataLoader {
    }
 }

+/// The full crate name should use the Codegen Unit builder to include full name resolution,


Yeah, I think that makes sense

danielsn · 2021-08-25T13:30:23Z

src/test/rmc-dependency-test/diamond-dependency/dependency3/src/lib.rs

+
+// Export a function that takes a struct type which differs between this crate 
+// and the other vesion.
+pub fn take_foo(foo: &Foo) {


Would there be an advantage to returning foo.x + foo.y so that upstream tests could assert on that?

Good idea, thanks

danielsn · 2021-08-25T13:32:31Z

src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh

+
+# Compile crates with RMC backend
+cd $(dirname $0)
+rm -rf build


Lets file a ticket for that

danielsn · 2021-08-25T13:33:34Z

src/test/rmc-dependency-test/diamond-dependency/run-dependency-test.sh

+# Compile crates with RMC backend
+cd $(dirname $0)
+rm -rf build
+CARGO_TARGET_DIR=build RUST_BACKTRACE=1 RUSTFLAGS="-Z codegen-backend=gotoc --cfg=rmc" RUSTC=rmc-rustc cargo build --target x86_64-unknown-linux-gnu 2> $LOG


What happens if we use the osx target here and build on mac

IIRC it requires rebuilding the std lib on mac, which then fails. We can look at this again together.

…el-checking#430)

danielsn reviewed Aug 17, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_llvm/src/gotoc/mir_to_goto/context/goto_ctx.rs Outdated Show resolved Hide resolved

avanhatt force-pushed the dependencies branch 4 times, most recently from 2f75433 to 5948728 Compare August 18, 2021 14:38

avanhatt changed the base branch from main-154-2021-08-06 to main-154-2021-08-17 August 19, 2021 13:49

avanhatt force-pushed the dependencies branch from fd7e2e5 to 7f8ef92 Compare August 19, 2021 13:49

danielsn reviewed Aug 19, 2021

View reviewed changes

avanhatt requested a review from danielsn August 20, 2021 17:56

avanhatt force-pushed the dependencies branch 2 times, most recently from e36b13f to 7f5d99e Compare August 24, 2021 18:18

avanhatt changed the title ~~Full crate name for allocations (support multiple crate versions)~~ Resolve duplicate crate dependencies (support multiple versions) Aug 24, 2021

danielsn approved these changes Aug 25, 2021

View reviewed changes

Rebase

bca62c2

avanhatt changed the base branch from main-154-2021-08-17 to main-154-2021-08-24 August 25, 2021 18:03

avanhatt force-pushed the dependencies branch from 2bd1040 to bca62c2 Compare August 25, 2021 18:04

avanhatt merged commit a0c3988 into model-checking:main-154-2021-08-24 Aug 26, 2021

avanhatt deleted the dependencies branch September 14, 2021 15:23

adpaco-aws mentioned this pull request Mar 17, 2022

Mimic crate compilation process as in cargo/rustc #143

Closed

tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 22, 2022

Resolve duplicate crate dependencies (allocations, struct names) (mod…

4d293c7

…el-checking#430)

tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 25, 2022

Resolve duplicate crate dependencies (allocations, struct names) (mod…

de5b8f5

…el-checking#430)

tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 26, 2022

Resolve duplicate crate dependencies (allocations, struct names) (mod…

90ccd3b

…el-checking#430)

tedinski pushed a commit that referenced this pull request Apr 27, 2022

Resolve duplicate crate dependencies (allocations, struct names) (#430)

f5ad6f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve duplicate crate dependencies (support multiple versions) #430

Resolve duplicate crate dependencies (support multiple versions) #430

avanhatt commented Aug 17, 2021 •

edited

Loading

danielsn Aug 19, 2021

avanhatt Aug 23, 2021

danielsn Aug 19, 2021

avanhatt Aug 23, 2021

danielsn Aug 19, 2021

avanhatt Aug 23, 2021

danielsn Aug 19, 2021

avanhatt Aug 19, 2021

danielsn Aug 25, 2021

danielsn Aug 19, 2021

avanhatt Aug 23, 2021

danielsn Aug 19, 2021

avanhatt Aug 20, 2021

danielsn Aug 19, 2021

avanhatt Aug 19, 2021

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

danielsn left a comment

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

danielsn Aug 25, 2021

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

danielsn Aug 25, 2021

danielsn Aug 25, 2021

avanhatt Aug 25, 2021

Resolve duplicate crate dependencies (support multiple versions) #430

Resolve duplicate crate dependencies (support multiple versions) #430

Conversation

avanhatt commented Aug 17, 2021 • edited Loading

Description of changes:

Global allocations

Struct types

Resolved issues:

Testing:

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avanhatt commented Aug 17, 2021 •

edited

Loading