Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

globally disabled logs still require symbols on x86 = linker error #411

Closed
japaric opened this issue Feb 24, 2021 · 12 comments
Closed

globally disabled logs still require symbols on x86 = linker error #411

japaric opened this issue Feb 24, 2021 · 12 comments
Labels
difficulty: medium Somewhat difficult to solve priority: high High priority for the Knurling team status: needs design This feature needs design work to move forward type: bug Something isn't working

Comments

@japaric
Copy link
Member

japaric commented Feb 24, 2021

This program:

fn main() {
    defmt::info!("hello");
    println!("hello");
}

fails to link if you use defmt v0.2.0

$ cargo r
(..)
  = note: /usr/bin/ld: (..)/target/debug/deps/libdefmt-00ba9b2b47686b1f.rlib(defmt-00ba9b2b47686b1f.defmt.bhjss6u8-cgu.12.rcgu.o): in function `defmt::export::acquire':
          (..)/defmt-0.2.0/src/export.rs:36: undefined reference to `_defmt_acquire'
          /usr/bin/ld: (..)/target/debug/deps/libdefmt-00ba9b2b47686b1f.rlib(defmt-00ba9b2b47686b1f.defmt.bhjss6u8-cgu.12.rcgu.o): in function `defmt::export::release':
          (..)/defmt-0.2.0/src/export.rs:48: undefined reference to `_defmt_release'
          /usr/bin/ld: (..)/target/debug/deps/libdefmt-00ba9b2b47686b1f.rlib(defmt-00ba9b2b47686b1f.defmt.bhjss6u8-cgu.12.rcgu.o): in function `defmt::export::timestamp':
          (..)/defmt-0.2.0/src/export.rs:60: undefined reference to `_defmt_timestamp'
          collect2: error: ld returned 1 exit status

but links and runs fine if you use defmt v0.1.3

$ cargo r
(..)
     Running `target/debug/hello`
hello

note that all logs are disabled in both cases (no Cargo features were added to the crate's Cargo.toml)

@japaric
Copy link
Member Author

japaric commented Feb 25, 2021

git bisect points to PR #291 as the cause of this issue

@jonas-schievink jonas-schievink added priority: high High priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: bug Something isn't working labels Feb 25, 2021
@Urhengulas Urhengulas added difficulty: medium Somewhat difficult to solve status: needs design This feature needs design work to move forward and removed status: needs PR Issue just needs a Pull Request implementing the changes labels Mar 16, 2021
@japaric
Copy link
Member Author

japaric commented Mar 30, 2021

a user has reported the _defmt_panic symbol is also required when host = x86 windows

bors bot added a commit that referenced this issue May 17, 2021
477: Disable logging calls via conditional compilation when all defmt features are disabled r=jonas-schievink a=WasabiFan

This is my take on fixing #411. It's not clear whether this is the "right" fix, as that issue was marked with a "medium" difficulty tag which would seem to imply there was more work expected here. Either way, it allows the sample in the linked issue to build for me, and fixes the usage in my own project.

The change here is to replace an "if" emitted by the logging macros (albeit with constant boolean conditions evaluation time) with an explicit `#[cfg(...)]` annotation. With the new form, the compiler seems to successfully strip out the unreachable macro body and as such there are no broken references to the nonexistent symbols.

Co-authored-by: Kaelin Laundry <wasabifan@outlook.com>
@Urhengulas
Copy link
Member

I think this issue is solved by #477. @WasabiFan tested it on windows and it also works for me on fedora.

@WasabiFan
Copy link
Contributor

WasabiFan commented Jun 5, 2021

Unfortunately (in addition to having inadvertently introduced breaking changes), this doesn't seem to solve the problem in all cases. To give a sampling:

  • My project's normal release build works fine, but building the tests on my Windows box returns the original linker errors
    • Depending on defmt via a local path dependency avoids any error, but depending on it via git or crates.io fails to compile as described above
    • Deleting a few arbitrary modules in my project fixes the error (probably something to do with Microsoft's linker giving up on optimizations if the project is too big? Not clear.)
  • Other team members on their own Windows machines cannot reproduce this error (it builds and tests run cleanly for them)
  • Linux builds work fine.

So I'm pretty strongly convinced that my fix is not sufficient to be truly dependable.

However, the fact that it can build demonstrates that there isn't much standing in the way of fixing this the "right" way -- I think bounding all four functions which declare external symbols (those containing extern "Rust" ) in src/export.rs with the same feature check as is now used in the macro calls themselves would be sufficient. We wouldn't even have to disable all usages of acquire/release/timestamp/panic, just swap them out with inert versions like the unstable-test feature does.

In my own project I have now (surely inadvisably) enabled the unstable-test feature in my test builds, purely to disable defmt as much as possible. I'll include details on my setup for others' reference below. This is a sufficient stopgap for my purposes.

I think for now this issue should be re-opened and I'd be interested to hear thoughts on adding those additional feature checks to the four functions mentioned above -- i.e., disable any attempt to resolve those symbols when no defmt features are enabled. In that case, building with --no-default-features would ensure that there is no attempt to resolve nonexistent functions. Alternately, a new feature could be added which has this effect when enabled.


I added the following to my dev-dependencies:

[dev-dependencies]
defmt = { version = "0.2.2", features = [ "unstable-test" ] }

And adopted the new Cargo feature resolver, so that the feature is not enabled in production builds:

[package]
resolver = "2"

@Urhengulas
Copy link
Member

Hi @WasabiFan, Could you reproduce the error on any other windows machine, except yours? And am I understanding it correctly that building the binary works for you, but building the tests fails?

@Urhengulas Urhengulas reopened this Jun 6, 2021
@WasabiFan
Copy link
Contributor

Could you reproduce the error on any other windows machine, except yours?

I could not. I tried on mine and that of one team member. I was on Rust 1.51.0 and linker version 14.29.30037.0, while he was on Rust 1.48.0 and an as-yet-unknown linker version. On my machine I have tried deleting the ~/.cargo and target/ directories but it does not resolve the error.

And am I understanding it correctly that building the binary works for you, but building the tests fails?

That's correct. I've included some more investigation below.


For reference, this is what the failure looks like on Windows:

 = note: libdefmt-bc0949ee2f21e9e1.rlib(defmt-bc0949ee2f21e9e1.defmt.8l142qgz-cgu.13.rcgu.o) : error LNK2019: unresolved external symbol _defmt_acquire referenced in function _ZN5defmt6export7acquire17ha087d68bec3c9c52E
          libdefmt-bc0949ee2f21e9e1.rlib(defmt-bc0949ee2f21e9e1.defmt.8l142qgz-cgu.13.rcgu.o) : error LNK2019: unresolved external symbol _defmt_release referenced in function _ZN5defmt6export7release17h93e50a97c1fdb04cE
          libdefmt-bc0949ee2f21e9e1.rlib(defmt-bc0949ee2f21e9e1.defmt.8l142qgz-cgu.13.rcgu.o) : error LNK2019: unresolved external symbol _defmt_timestamp referenced in function _ZN5defmt6export9timestamp17hfcf361573ac65133E
          libdefmt-bc0949ee2f21e9e1.rlib(defmt-bc0949ee2f21e9e1.defmt.8l142qgz-cgu.13.rcgu.o) : error LNK2019: unresolved external symbol _defmt_panic referenced in function _ZN5defmt6export5panic17h6994ca4167c48b4cE
          C:\Users\kaeli\repos\aruw-cap-bank-sw-2\target\debug\deps\aruw_cap_bank-eda677a66494d9cc.exe : fatal error LNK1120: 4 unresolved externals

I think I've found why my particular project fails when building tests: adding a proptest test, even if empty, causes defmt to fail to link.

Here's a modified version of the repro case from the OP:

fn main() {
    defmt::info!("hello");
    println!("Hello, world!");
}

#[cfg(test)]
mod tests {
    use proptest::prelude::*;

    proptest! {
        #[test]
        fn something(i in 0..10) {
            
        }
    }
}

With the following dependencies in an otherwise bare project:

[dependencies]
proptest = "0.10.1"
defmt = "0.2.2"

...gives the same error quoted above. Again, this is probably specific to the software versions on my machine. Commenting out the proptest test builds fine. To me, this reinforces the conclusion that there'll need to be a more consistent fix here, since the only reasonable way I could imagine proptest affecting the defmt case is if it's breaking the linker's ability to optimize away unused defmt internals.

@WasabiFan
Copy link
Contributor

WasabiFan commented Jun 7, 2021

Further update -- I bit the bullet and tried an upgrade to Rust 1.52.1; the error no longer reproduces. I then tried downgrading back to the same version I was originally on, and I still can't repro it. It isn't clear to me what this would indicate (I'm not sure what might have been reset which fixed it), but it might be sufficient to just call it a solved problem unless/until someone else comes across the same issue. I'll post pack here if it pops up again. 🤷

Edit: the latter part was user error, I can consistently repro on 1.51.0 yet pass a test run on 1.52.1.

@WasabiFan
Copy link
Contributor

Annnnnd it's back. This time, I ran a cargo update; I can stash the Cargo.lock changes, and my tests build fine -- pop the stash, and the tests fail to link. Neither defmt nor proptest were touched in the upgrade.

The MS linker is doing something undocumented which I'm running right up against in inconsistent ways.

@Urhengulas
Copy link
Member

Annnnnd it's back. This time, I ran a cargo update; I can stash the Cargo.lock changes, and my tests build fine -- pop the stash, and the tests fail to link. Neither defmt nor proptest were touched in the upgrade.

The MS linker is doing something undocumented which I'm running right up against in inconsistent ways.

Is it now also happening on 1.52?

@WasabiFan
Copy link
Contributor

That's correct. ☹️

@Urhengulas
Copy link
Member

I will try to test it on my windows partition tomorrow. I'd like to reproduce it on at least one other machine, before taking further steps.

@Urhengulas
Copy link
Member

I just failed to reproduce the error. I will close this issue for now, since the original error is resolved. If you encounter this issue again and are able to reproduce it somewhat realiably, please open a new issue!

PS: setting up rust on windows isn't exactly fun, is it? 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium Somewhat difficult to solve priority: high High priority for the Knurling team status: needs design This feature needs design work to move forward type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants