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

defmt crate doesn't compile on macOS #336

Closed
BriocheBerlin opened this issue Jan 12, 2021 · 18 comments
Closed

defmt crate doesn't compile on macOS #336

BriocheBerlin opened this issue Jan 12, 2021 · 18 comments
Assignees
Labels
priority: high High priority for the Knurling team type: bug Something isn't working
Milestone

Comments

@BriocheBerlin
Copy link
Contributor

BriocheBerlin commented Jan 12, 2021

Updated description:
defmt 0.1.3 compiles on macOS but the git version (0.2.0) doesn't
repro:

$ cargo new --bin hello
$ cd hello
$ cargo add defmt --vers 0.1.3
$ cargo run
$ # ^ doesn't work if you use the git version

When trying to get started to write compile fail tests , running
$ cargo test --test ui
didn't work for me using a mac. I'd always get this unhelpful error:

LLVM ERROR: Global variable '{"package":"defmt","tag":"defmt_prim","data":"{=i8}","disambiguator":"2773326613612971761"}' has an invalid section specifier '.defmt.prim.{"package":"defmt","tag":"defmt_prim","data":"{=i8}","disambiguator":"2773326613612971761"}': mach-o section specifier requires a segment whose length is between 1 and 16 characters.
error: could not compile `defmt` 

Passing more flags like this did help:
$ cargo test -p defmt --all-features --test ui

This makes sense when looking at ci.yml

@japaric
Copy link
Member

japaric commented Jan 12, 2021

@BriocheBerlin could you test this on macOS? I'd like to figure out if defmt fails to compile natively or if this problem is due to the single workspace configuration used in this repository.

$ cargo new --bin hello
$ cd hello
$ # or manually add 'defmt = "0.1.3"' to Cargo.toml
$ cargo add defmt 
$ # does this succeed?
$ cargo run

@BriocheBerlin
Copy link
Contributor Author

$ cargo run succeeded.

@japaric
Copy link
Member

japaric commented Jan 12, 2021

@BriocheBerlin could you run cargo test --test ui -j1 in this repo and report which crate fails to build? That command builds one crate at a time rather than in parallel.

@BriocheBerlin
Copy link
Contributor Author

BriocheBerlin commented Jan 13, 2021

So it seems that in the working version on the hello project the command for building the defmt crate is very similar. However it uses a different src/lib.rs file which is local <more_path>/.cargo/registry/src/github.mirror.nvdadr.com-1ecc6299db9ec823/defmt-0.1.3/src/lib.rs. It is different to the recent file src/lib.rs however simply switching it out does not work. Also it seems old(?) because doesn't have the InternalFormatter.

@japaric
Copy link
Member

japaric commented Jan 14, 2021

hmm, maybe the macOS issue was introduced recently so it's in the git version but not on crates.io?
could you try these two:

$ cargo new --bin hello
$ cd hello

$ # or add 'defmt = { git = "https://github.com/knurling-rs/defmt" }' to Cargo.toml dependencies
$ cargo add defmt --git https://github.com/knurling-rs/defmt

$ # does this succeed?
$ cargo run
$ git clone https://github.com/knurling-rs/defmt
$ cd defmt
$ git checkout defmt-v0.1.3

$ # do these produce the llvm error?
$ cargo build
$ cargo test

@BriocheBerlin
Copy link
Contributor Author

BriocheBerlin commented Jan 14, 2021

defmt-v0.1.3 works without problems. However the hello thing produces this interesting error:

Caused by:
  failed to load source for dependency `defmt`
Caused by:
  Unable to update https://github.com/knurling-rs/defmt
Caused by:
  failed to find branch `master`
Caused by:
  cannot locate remote-tracking branch 'origin/master'; class=Reference (4); code=NotFound (-3)

But of course there is no master anymore.

@japaric
Copy link
Member

japaric commented Jan 14, 2021

oh, yes. my bad it should be

defmt = { git = "https://github.com/knurling-rs/defmt", branch = "main" }

in the Cargo.toml

@BriocheBerlin
Copy link
Contributor Author

BriocheBerlin commented Jan 14, 2021

Ah, doesn't work. So (besides different src/lib.rs files) the resulting commands are slightly different.
Working version (using defmt = "0.1.3" in Cargo.toml):

rustc --crate-name defmt 
--edition=2018 
<more_path>/.cargo/registry/src/github.mirror.nvdadr.com-1ecc6299db9ec823/defmt-0.1.3/src/lib.rs 
--error-format=json 
--json=diagnostic-rendered-ansi 
--crate-type lib 
--emit=dep-info,metadata,link 
-C embed-bitcode=no 
-C debuginfo=2 
-C metadata=4f3b6bbbb29948b6 
-C extra-filename=-4f3b6bbbb29948b6 
--out-dir <more_path>/hello/target/debug/deps 
-L dependency=<more_path>/hello/target/debug/deps 
--extern defmt_macros=<more_path>/hello/target/debug/deps/libdefmt_macros-f658b9f3b197fd12.dylib 
--extern heapless=<more_path>/hello/target/debug/deps/libheapless-9846ae8300a43c3e.rmeta 
--cap-lints allow 
-L <more_path>/hello/target/debug/build/defmt-d1e4ca37c018f197/out

With defmt = { git = "https://github.com/knurling-rs/defmt", branch = "main" } it doesn't have
--extern heapless=<path>
When running on defmt as is, this line is different:
-L dependency=<more_path>/hello/target/debug/deps
Instead I got:
-C incremental=<more_path>/defmt/target/debug/incremental

@japaric
Copy link
Member

japaric commented Jan 14, 2021

Then this more serious: as things are defmt 0.2 won't compile on macOS which is going to hinder adoption: using defmt in a no-std, non-embedded crate would make the crate unusable on macOS.

I'll put this on the list of blockers for 0.2.0. cc @jonas-schievink

@japaric japaric added this to the v0.2.0 milestone Jan 14, 2021
@japaric japaric added type: bug Something isn't working priority: high High priority for the Knurling team labels Jan 14, 2021
@japaric japaric changed the title Running ui tests on mac defmt crate doesn't compile on macOS Jan 14, 2021
@jonas-schievink
Copy link
Contributor

The invalid section names were already used in defmt 0.1 though? Or is there another error?

@japaric
Copy link
Member

japaric commented Jan 14, 2021

I think this may have been caused by PR #291 . After the changes in the macros we emit link_section on x86_64 macOS -- before we emitted fetch_add_string_index on x86_64 macOS.

@BriocheBerlin
Copy link
Contributor Author

That would explain why in the working src/lib.rs file it was always target_arch = "x86_64" instead of feature = "unstable-test".

@BriocheBerlin
Copy link
Contributor Author

BriocheBerlin commented Jan 14, 2021

The last commit where $ cargo build works for me is 04cf573.
(But $ cargo test fails, again complaining about missing InternalFormatter).

@japaric
Copy link
Member

japaric commented Jan 15, 2021

we looked into this. the llvm error can be fixed by changing the linker section names we use.
currently we use .defmt.print.<LONG JSON STRING HERE>
on macOS the longest linker section name can be .defmt,0123456789ABCDEF (note comma after .defmt not period)
we should adjust the macros to generate a linker section name like that
instead of we should use digest / hash of that and truncate it to not exceed the 16 character limits

@japaric
Copy link
Member

japaric commented Jan 15, 2021

for starters this proc-macro needs to be changed (but probably others also need to be changed to work on macOS):

let section = format!(".defmt.prim.{}", sym);

@japaric
Copy link
Member

japaric commented Jan 15, 2021

we should also build and run the following std program on linux, macos and windows to shake out any other linker section name related bugs:

// Cargo.toml uses defmt via a path dependency

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

#[defmt::global_logger]
struct Logger;

unsafe impl defmt::Logger for Logger {
    fn acquire() -> Option<core::ptr::NonNull<dyn defmt::Write>> {
        None
    }
    unsafe fn release(writer: core::ptr::NonNull<dyn defmt::Write>) {}
}

defmt::timestamp!("{=u32}", 0);

@spookyvision spookyvision self-assigned this Jan 25, 2021
spookyvision pushed a commit that referenced this issue Jan 25, 2021
bors bot added a commit that referenced this issue Jan 28, 2021
357: fix issue #336 r=jonas-schievink a=spookyvision

Introduce special cased symbol names to satisfy the macos linker

Co-authored-by: Anatol Ulrich <anatol.ulrich@ferrous-systems.com>
bors bot added a commit that referenced this issue Feb 1, 2021
363: add a test for #336 r=jonas-schievink a=spookyvision

make sure basic usage builds & links, as suggested [here](#336 (comment))

Co-authored-by: Anatol Ulrich <anatol.ulrich@ferrous-systems.com>
@Urhengulas
Copy link
Member

This issue seems resolved, right?

@jonas-schievink
Copy link
Contributor

Yup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high High priority for the Knurling team type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants