Rust: proc_macro::__internal::with_parse_sess() called before set_parse_sess()

[dtolnay]

https://github.com/serde-rs/serde/issues/764 and https://github.com/jwilm/alacritty/issues/413 both have this assertion failing. Is this a bug in rustc or in our use of libproc_macro?

pub fn with_parse_sess<F, R>(f: F) -> R
    where F: FnOnce(&ParseSess) -> R
{
    let p = CURRENT_SESS.with(|p| p.get());
    assert!(!p.is_null());
    f(unsafe { &*p })
}

cc @abonander and @alexcrichton who own most of that file
cc @jwilm who is debugging https://github.com/jwilm/alacritty/issues/413

[abonander]

Something changed since I touched this file. I added a message to that assert!() when I implemented #[proc_macro_attribute].

[dtolnay]

Your message is in 1.16.0-beta. The panic is from 1.15.1. Any idea how this assert could be failing?

[abonander]

I'd have to look at the sources that Homebrew Rust used, apparently.

[alexcrichton]

This doesn't sound like a bug with either serde or the compiler but rather an error in some form of a binary artifact. At first blush this looks like the compiler is running with one libproc_macro.dylib but serde's using a different one. Basically libproc_macro.dylib relies on global state (the thread local) and if that's duplicate (in the case that there's two by accident) then things can go very wrong!

I'm installing Homebrew rust to investigate what's going on.

[alexcrichton]

Ah yes, that is indeed the bug. Here's what's happening.

So when you install Rust you actually get two copies of libraries. For example you'll have both:

• $sysroot/lib/libproc_macro-$hash.dylib
• $sysroot/lib/rustlib/x86_64-apple-darwin/lib/libproc_macro-$hash.dylib

Both of these should be byte-for-byte identical, we actually copy them in the build system! When you link against a dynamic library on OSX the library itself encodes a name of how to find it in the future by default. We can inspect this for rustup via the LC_ID_DYLIB attribute like so:

$ otool -l $sysroot/rustlib/x86_64-apple-darwin/lib/libproc_macro-*.dylib | grep -A2 LC_ID_DYLIB
          cmd LC_ID_DYLIB
      cmdsize 72
         name @rpath/libproc_macro-4a4250f8567d2c62.dylib (offset 24)

What this says is basically "in the future, find my by looking at @rpath (a meta variable) and look for this file name". If we take a look at Homebrew, however, we see a difference:

$ otool -l $sysroot/rustlib/x86_64-apple-darwin/lib/libproc_macro-*.dylib | grep -A2 LC_ID_DYLIB
          cmd LC_ID_DYLIB
      cmdsize 120
         name /usr/local/opt/rust/lib/rustlib/x86_64-apple-darwin/lib/libproc_macro-d8fca079e2c66403.dylib (offset 24)

Aha, a difference! I believe this is something that homebrew does by default, changing the value of LC_ID_DYLIB of installed dylibs (to ensure these versions are loaded, not others).

This change in LC_ID_DYLIB is then reflected in the libserde_derve-*.dylib itself, if we take a look at that we'll see for rustup:

          cmd LC_LOAD_DYLIB
      cmdsize 72
         name @rpath/libproc_macro-4a4250f8567d2c62.dylib (offset 24)

whereas for Homebrew we'll see:

          cmd LC_LOAD_DYLIB
      cmdsize 120
         name /usr/local/opt/rust/lib/rustlib/x86_64-apple-darwin/lib/libproc_macro-d8fca079e2c66403.dylib (offset 24)

So I think there's two bugs here:

1. There's a longstanding bug that we shouldn't ship duplicate sets of libraries, and this is something that we should sort out ourselves.
2. Homebrew may wish to not alter the LC_ID_DYLIB value of target libraries in $sysroot/lib/rustlib/*

[alexcrichton]

cc https://github.com/rust-lang/rust/issues/28640, more info about the internals here

[brson]

Would this setup without rpath would work correctly if the homebrew build maintained the property that the two libs directories are identical? (It doesn't seem proper to _require_ rpath for rust to work correctly).

[brson]

Excellent investigation @alexcrichton. Thanks.

[alexcrichton]

cc @ilovezfs and @alex, I've seen you two updating the Rust formula over at Homebrew/homebrew-core so I was hoping you'd be able to help me with a question.

In my comment above I've noticed that a Homebrew-installed version of Rust (specifically installed from a bottle) has a different LC_ID_DYLIB directive than the compiler is expecting, which is causing this problem unfortunately. I think this definitely a bug on our end that we could fix (to make this not actually cause problems) but that may take us awhile to fix so I was hoping we could in parallel pursue a solution with Homebrew as well.

Do you know if it's correct if Homebrew uses install_name_tool to alter dynamic libraries that are installed? I vaguely remember this being the case awhile back but poking around the codebase I can't seem to find any relevant locations. If not, do you know where such a change might arise?

I'm currently installing from source to verify it's not a bottle problem, but that may take a few minutes!

[alex]

Unfortunately I can't really help. I'm an unsophisticated person who just increments version numbers and computes hashes.

[ilovezfs]

@alexcrichton it's actually ruby-macho not install_name_tool, but yes you have the right idea about what's happening. Some manual interventions are also possible (see https://github.com/Homebrew/homebrew-core/pull/8049 for examples).

The main reason the paths get changed is so that they use the opt path instead of the Cellar path, which prevents revision bumps from causing the paths to change, and thus all reverse dependencies to require pointless rebuilds even when there's been no change to the ABI.

It might be possible to fix the breakage in post_install, which runs after the bottle is poured but before brew install exits.

CC @woodruffw who knows the most about our use of ruby-macho for thoughts here.

[alexcrichton]

Ok cool, thanks for the info @ilovezfs! I think here what should happen is that libraries the rustc binary itself runs with should be changed as usual (e.g. what's happening today). The libraries installed that programs link against, however, should probably preserve the same LC_ID_DYLIB that Rust ships with, and I believe that'd fix this bug.

If I can finagle that locally does that sounds like a reasonable patch?

[ilovezfs]

If I can finagle that locally does that sounds like a reasonable patch?

Whatever actually works should be fine at least temporarily, but it would be good to get the fixes into rust or https://github.com/Homebrew/brew or some combination both if possible since manual uses of ruby-macho in homebrew/core are pretty yuck.

[ilovezfs]

BTW the magic mostly happens here: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/extend/os/mac/keg_relocate.rb

[jwilm]

Fantastic work figuring this out @alexcrichton, and thanks @dtolnay for posting the issue.

[woodruffw]

Thanks for pinging me in @ilovezfs.

I think the bug is in our (i.e., Homebrew's) handing of dylib IDs - we're careful not to clobber metavariables like @rpath in install names, but we seem to have neglected the same case for the IDs themselves. I'm building rust locally with a patched Homebrew to confirm my suspicions here, and I'll report the results when the build completes.

[woodruffw]

Patching Homebrew with this fixed the rewriting of @rpath in LC_ID_DYLIBs for me, but I don't know enough about the rust toolchain to test for functionality. Could someone with the original problem use --build-from-source and confirm that this works?

diff --git a/Library/Homebrew/extend/os/mac/keg_relocate.rb b/Library/Homebrew/extend/os/mac/keg_relocate.rb
index f44a97b31..476e5da4a 100644
--- a/Library/Homebrew/extend/os/mac/keg_relocate.rb
+++ b/Library/Homebrew/extend/os/mac/keg_relocate.rb
@@ -78,13 +78,19 @@ class Keg
     end
   end

+  def filename_contains_metavariable?(fn)
+    fn =~ /^@(loader_|executable_|r)path/
+  end
+
   def each_install_name_for(file, &block)
     dylibs = file.dynamically_linked_libraries
-    dylibs.reject! { |fn| fn =~ /^@(loader_|executable_|r)path/ }
+    dylibs.reject! { |fn| filename_contains_metavariable?(fn) }
     dylibs.each(&block)
   end

   def dylib_id_for(file)
+    return file.dylib_id if filename_contains_metavariable?(file.dylib_id)
+
     # The new dylib ID should have the same basename as the old dylib ID, not
     # the basename of the file itself.
     basename = File.basename(file.dylib_id)

[woodruffw]

The patch above looks correct to me, but I'm not sure why we (again, Homebrew) weren't skipping dylib ID rewriting under those cases before.

cc @UniqMartin for an opinion (sorry for bringing you out of retirement)

[alexcrichton]

Oh wow thanks for the quick patch @woodruffw! I'm testing that out locally. If you've got a compiler on hand though an easy way to test is to clone https://github.com/alexcrichton/toml-rs and run cargo test inside of that directory. If it works then the patch fixed the problem, and if it fails there's something else lurking :)

[woodruffw]

Thanks @alexcrichton, I'll test that out!

[woodruffw]

Seems to have worked!

warning: unused manifest key: badges.travis-ci.repository
warning: unused manifest key: package.categories
warning: unused manifest key: package.categories
warning: unused manifest key: package.categories
    Updating registry `https://github.com/rust-lang/crates.io-index`
 Downloading serde_json v0.9.6
 Downloading serde_derive v0.9.7
 Downloading serde v0.9.7
 Downloading num-traits v0.1.36
 Downloading dtoa v0.4.1
 Downloading itoa v0.3.1
 Downloading syn v0.11.4
 Downloading serde_codegen_internals v0.13.0
 Downloading quote v0.3.12
 Downloading unicode-xid v0.0.4
   Compiling num-traits v0.1.36
   Compiling quote v0.3.12
   Compiling unicode-xid v0.0.4
   Compiling dtoa v0.4.1
   Compiling serde v0.9.7
   Compiling itoa v0.3.1
   Compiling syn v0.11.4
   Compiling serde_codegen_internals v0.13.0
   Compiling serde_derive v0.9.7
   Compiling serde_json v0.9.6
   Compiling toml v0.3.0 (file:///Users/william/toml-rs)
    Finished debug [unoptimized + debuginfo] target(s) in 33.23 secs
     Running target/debug/deps/backcompat-40a51311afd74555

[...snip...]

   Doc-tests toml

running 6 tests
test _0 ... ignored
test ser::tables_last_0 ... ok
test _1 ... ok
test _3 ... ok
test ser_0 ... ok
test _2 ... ok

test result: ok. 5 passed; 0 failed; 1 ignored; 0 measured

[alexcrichton]

Excellent! Sorry I've been slow at verifying this morning, had a few false starts for various reasons building rustc.

Would the next steps here be a PR to Homebrew? @woodruffw would you be willing to do so?

[woodruffw]

Would the next steps here be a PR to Homebrew? @woodruffw would you be willing to do so?

Yep, and yep. I'm going to do some more local testing to ensure that this doesn't cause breakage in other packages, but I'll create a brew PR soon.

As you mentioned in https://github.com/rust-lang/rust/issues/39870#issuecomment-280221581, it'd be good to have this fixed upstream as well :smile:

[woodruffw]

Opened as https://github.com/Homebrew/brew/issues/2036.

[alexcrichton]

Awesome, thanks @woodruffw!

[woodruffw]

Homebrew/brew now has the fix, and https://github.com/Homebrew/homebrew-core/issues/10198 will bump rust to apply it to the bottles.

[woodruffw]

As a heads up: we think that Homebrew/homebrew-core#10198 is likely to cause problems for users who have SIP enabled, so we may be reverting it soon. If that happens, rust is going to need to expedite the fix for its libraries or rewrite the IDs/install names in the formula as originally discussed.

[alexcrichton]

@woodruffw would we be able to apply such a fix to just the Rust recipe itself? AFAIK that's the "correct fix" for us (and easiest)

[woodruffw]

would we be able to apply such a fix to just the Rust recipe itself? AFAIK that's the "correct fix" for us (and easiest)

That's possible, although it'd be much nicer from Homebrew's perspective to get https://github.com/rust-lang/rust/issues/39875 rolled through. We strongly prefer not to fixup Mach-Os inside of individual formulae, since it quickly becomes a maintenance burden.

That being said, no change is immediately necessary. The SIP issue will probably be limited to linkages with system-provided libraries, and it hasn't reared its head yet. Apologies if I made things sound more urgent than they actually are in my previous comment!

[ilovezfs]

@alexcrichton FYI https://github.com/Homebrew/homebrew-science/issues/5173 is the sort of issue we're concerned about. Presumably rust is subject to that problem if linked from system Python, Perl, etc. just like anything else would be …

[alexcrichton]

Unfortunately I don't think I'll have any time soon to tackle #39875, so it may take quite awhile to get fixed

[ilovezfs]

@alexcrichton it turns out the change broke llvm:

$ brew test llvm
Testing llvm
==> Using the sandbox
==> /usr/local/Cellar/llvm/3.9.1/bin/llvm-config --prefix
==> /usr/local/Cellar/llvm/3.9.1/bin/clang -L/usr/local/Cellar/llvm/3.9.1/lib -fopenmp -nobuiltininc -I/usr/local/Cellar/llvm/3.9.1/lib/clang/3.9.1/include omptest.c -o omptest
==> ./omptest
dyld: Library not loaded: @rpath/libomp.dylib
  Referenced from: /private/tmp/llvm-test-20170228-37913-v6douy/./omptest
  Reason: image not found
Error: llvm: failed
<0> expected but was
<nil>.

So a revert is probably imminent.

[ilovezfs]

One option is to work around it with

iMac-TMP:homebrew-core joe$  git diff -- Formula/llvm.rb 
diff --git a/Formula/llvm.rb b/Formula/llvm.rb
index 103aa3b..cc5ee3c 100644
--- a/Formula/llvm.rb
+++ b/Formula/llvm.rb
@@ -301,8 +301,8 @@ class Llvm < Formula
       }
     EOS

-    system "#{bin}/clang", "-L#{lib}", "-fopenmp", "-nobuiltininc",
-                           "-I#{lib}/clang/#{version}/include",
+    system "#{bin}/clang", "-L#{lib}", "-Wl,-rpath,#{lib}", "-fopenmp",
+                           "-nobuiltininc", "-I#{lib}/clang/#{version}/include",
                            "omptest.c", "-o", "omptest"
     testresult = shell_output("./omptest")

@@ -381,7 +381,7 @@ class Llvm < Formula
               "-I#{MacOS.sdk_path}/usr/include",
               "-L#{lib}",
               "-Wl,-rpath,#{lib}", "test.cpp", "-o", "test"
-      assert_includes MachO::Tools.dylibs("test"), "#{opt_lib}/libc++.1.dylib"
+      assert_includes MachO::Tools.dylibs("test"), "@rpath/libc++.1.dylib"
       assert_equal "Hello World!", shell_output("./test").chomp
     end
   end

though I'm wondering what else may have broken besides our own tests.

[ilovezfs]

@alexcrichton heads up that we have reverted https://github.com/Homebrew/brew/pull/2036 in https://github.com/Homebrew/brew/pull/2450 so rust will now be broken in the same way as before.

[alexcrichton]

Thanks for the heads up @ilovezfs

[alexcrichton]

er didn't mean to close

[UniqMartin]

cc @UniqMartin for an opinion (sorry for bringing you out of retirement)

No worries and sorry for the incredibly late response. 🙈 In my understanding, the reason why Homebrew handles @rpath et al. the way it does is twofold:

1. Unconditionally rewriting LC_ID_DYLIB is safe as we (Homebrew) know with certainty where the referenced file resides and have an opinion on what the canonical reference for other binaries using this library should be. The same cannot be said for LC_LOAD_DYLIB (and friends) as we don't have enough knowledge about how a library is going to be used and the run-time resolution of those placeholders can become arbitrarily complex (and unpredictable) – except for the relatively straightforward case of @loader_path.

2. We (again Homebrew) tend to have the opinion that we build tools and libraries for use with Homebrew, referencing Homebrew (and system components), and don't aim to provide packages that work in isolation from Homebrew either directly or indirectly (i.e. binaries created by tools installed via Homebrew). Thus a rewrite of paths that causes them to become absolute and to point into the Homebrew installation is acceptable, even if that undermines the relocatability of a package that has managed to get all the loader semantics right on macOS – something that is probably not true for the majority of software packaged by Homebrew.

[woodruffw]

Those reasons make sense; I'm surprised we didn't have more troubles when we merged https://github.com/Homebrew/brew/pull/2036 :smile:

[ilovezfs]

@woodruffw hot off the press https://github.com/Homebrew/homebrew-science/issues/5434

[iKevinY]

So @alexreg and I are looking to add Homebrew support to Tectonic, but we've run into this issue. Does anyone know if there are ways to work around this (or plans to re-patch "Homebrew Rust")?

[lvillani]

@ilovezfs Since the rust formula is essentially broken, wouldn't be better to either temporarily remove it or add a huge warning to the caveats section?

[alexreg]

@lvillani @iKevinY I think, rather, the solution is to fix Homebrew's Rust ASAP!

[ilovezfs]

@lvillani @iKevinY @alexreg you'll need to encourage upstream to work on https://github.com/rust-lang/rust/issues/39875. I don't expect any further changes on the Homebrew side, nor will the rust formula be removed.

[lvillani]

As an outsider who doesn't know why rust ships two identical dylibs or why brew has to fiddle with dylib references it seems to me that both upstream (rust) and downstream (brew) are doing something wrong, to varying degrees.

But...

I don't understand Homebrew's insistence in shipping a clearly broken formula/bottle without printing a huge warning that it's broken. This led me to waste a couple of hours trying to figure out what was wrong with the compiler. Judging from comments elsewhere this seems to be impacting other people.

My humble suggestion is to either:

• Temporarily remove/disable the rust formula until #39875 is sorted out so that people looking to install the compiler end up using rustup, which works just fine.
• Add a big scary warning to the formula so that people who end up installing it know what they are getting into. I can work on that if that's a PR that's going to be accepted (I'll need a bit of hand-holding to figure out the best way to do it, though).

Let me know how you wish to proceed, I'd like to spare other people the frustration caused by this particular issue.

[ilovezfs]

Neither of those is going to happen.

[woodruffw]

brew has to fiddle with dylib references

Just to explain this: Since brew installs software in its own prefix, we need to rewrite binaries to find their libraries, dependencies, and resources correctly. The details aren't all that important, but it's an essential part of the installation process.

[woodruffw]

Hey @alexcrichton, https://github.com/Homebrew/brew/issues/2764 has the changes to temporarily work around this problem in Homebrew. It's based on your observation in https://github.com/rust-lang/rust/issues/39870#issuecomment-280216673 that only the binaries /lib/rustlib/ need to be left untouched.

We'd still like to get this fixed more permanently by de-duplicating these libraries, but could you give this a look over/some testing? @ilovezfs and I have tested 1.17.0 and 1.18.0 (I ran cargo test on toml-rust successfully), but we'd appreciate your feedback/more informed testing as well.

[lvillani]

Thanks for working on this @woodruffw! I was about to submit a simple test case and a patch to add a "caveats" section to the formula but I'm glad they won't be necessary anymore. 🎉

[alexreg]

Sounds good! If we can get this accepted to Homebrew, it will really make things a lot easier for creators of Rust-based formulas like @iKevinY and me.

[ilovezfs]

@alexcrichton A new workaround for this issue and https://github.com/rust-lang/rust/issues/39875 is now in Homebrew: https://github.com/Homebrew/homebrew-core/pull/14490 thanks to @woodruffw.

bash-3.2$ export sysroot=$(brew --prefix rust)
bash-3.2$ otool -l $sysroot/lib/rustlib/x86_64-apple-darwin/lib/libproc_macro-*.dylib | grep -A2 LC_ID_DYLIB
          cmd LC_ID_DYLIB
      cmdsize 72
         name @rpath/libproc_macro-b5231b1bb281b0e9.dylib (offset 24)
bash-3.2$ otool -l $sysroot/lib/libproc_macro-*.dylib | grep -A2 LC_ID_DYLIB
          cmd LC_ID_DYLIB
      cmdsize 88
         name /usr/local/opt/rust/lib/libproc_macro-b5231b1bb281b0e9.dylib (offset 24)
bash-3.2$

@lvillani

bash-3.2$ git clone https://github.com/lvillani/homebrew-rustc-proc-macro-ice
Cloning into 'homebrew-rustc-proc-macro-ice'...
remote: Counting objects: 14, done.
remote: Compressing objects: 100% (10/10), done.
remote: Total 14 (delta 4), reused 12 (delta 2), pack-reused 0
Unpacking objects: 100% (14/14), done.
bash-3.2$ cd homebrew-rustc-proc-macro-ice
bash-3.2$ mdutil -t `which rustc`
/usr/local/Cellar/rust/1.18.0_1/bin/rustc
bash-3.2$ mdutil -t `which cargo`
/usr/local/Cellar/rust/1.18.0_1/bin/cargo
bash-3.2$ cargo clean
bash-3.2$ cargo build
   Compiling dtoa v0.4.1
   Compiling quote v0.3.15
   Compiling itoa v0.3.1
   Compiling unicode-xid v0.0.4
   Compiling serde v1.0.8
   Compiling num-traits v0.1.38
   Compiling synom v0.11.3
   Compiling syn v0.11.11
   Compiling serde_derive_internals v0.15.1
   Compiling serde_json v1.0.2
   Compiling serde_derive v1.0.8
   Compiling homebrew-rustc-proc-macro-ice v0.1.0 (file:///usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/homebrew-rustc-proc-macro-ice)
    Finished dev [unoptimized + debuginfo] target(s) in 9.17 secs

[alexcrichton]

Thanks for the updates here @ilovezfs and @woodruffw! Sorry I was on vacation this past weekend but both those fixes look good to me! Thanks so much for getting Rust on Homebrew working!

[MikeMcQuaid]

@alexcrichton Thanks for the kind words! Our fix is a temporary measure though and not one we want to maintain long-term. Would an upstream fix on your end be possible? If so, what might the (extremely, extremely) vague timescale be? Thanks!

[alexcrichton]

Unfortunately I wouldn't have a great estimate of that. I've opened https://github.com/rust-lang/rust/issues/42645 to track work on that specifically but it's likely to remain open until someone motivated comes along to fix it. To that end I've tagged it as a help wanted issue!

It looks though like this issue is itself fixed due to the hack now present in Homebrew. In light of that the more relevant issue now is https://github.com/rust-lang/rust/issues/42645 so I'm going to close this in favor of that one. Fixing https://github.com/rust-lang/rust/issues/42645 would solve this issue as well and also allow the Homebrew hack to be removed.