Skip to content

libiconv necessary on mac, doesn't link #488

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.

Sign up for GitHub

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

Jump to bottom
Closed
boyland-pf opened this issue Jul 7, 2022 · 6 comments · Fixed by #554
Closed

libiconv necessary on mac, doesn't link #488

boyland-pf opened this issue Jul 7, 2022 · 6 comments · Fixed by #554
Labels
bug Something isn't working building Build/compile errors or build system-related

Comments

@boyland-pf
Copy link
Contributor

boyland-pf commented Jul 7, 2022 •
edited by kkysen
Loading

The mac version of c2rust with dynamic instrumentation enabled only requires libiconv at some point in the build, resulting in the following error.

  = note: Undefined symbols for architecture x86_64:
            "_libiconv", referenced from:
                _git_fs_path_iconv in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
            "_libiconv_open", referenced from:
                _git_fs_path_direach in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
                _git_fs_path_iconv_init_precompose in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
                _git_fs_path_diriter_init in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
            "_libiconv_close", referenced from:
                _git_fs_path_direach in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
                _git_fs_path_iconv_clear in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
                _git_fs_path_diriter_free in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
                _git_fs_path_dirload in liblibgit2_sys-4e72aab137059590.rlib(fs_path.o)
          ld: symbol(s) not found for architecture x86_64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

This happens on my mac (a Catalina), which has iconv, and won't link libiconv (installed via brew) because it already has iconv which does the same thing.

This post on stackoverflow (https://stackoverflow.com/questions/57734434/libiconv-or-iconv-undefined-symbol-on-mac-osx) has the best description of the problem that I've found.

Because the compilation is handled by cargo, I don't know how to find a list of all the compilation steps to add --with-libiconv-prefix=<dir> to, or whether this would work.
@fw-immunant
Copy link
Contributor

This seems to impact us because this crate depends on the cargo crate which (transitively) requires the libgit2-sys crate. There are a number of upstream reports related to this issue, but this recent one seems potentially helpful: rust-lang/git2-rs#830

In any case, it does seem that this is an upstream issue and maybe not one we can easily fix ourselves (short of not depending on the cargo crate), and that there are some viable workarounds for users (wrapping our build with port [de]activate libiconv).

@kkysen
Copy link
Contributor

kkysen commented Jul 7, 2022 •
edited
Loading

We are planning on no longer depending on the cargo crate soon (see #448). We'll just invoke cargo as an external command and override $RUSTC_WRAPPER, so I think this shouldn't be an issue anymore if we fix that. It's possible we may need cargo for some small querying stuff after that, but I'm guessing it's small enough we can implement it ourselves or use something more detached like cargo metadata.

@kkysen kkysen changed the title libiconv necessary on mac, doesn't link libiconv necessary on mac, doesn't link Jul 7, 2022
@kkysen
Copy link
Contributor

kkysen commented Jul 7, 2022

Should fixing #448 be prioritized at all? How much of an issue is this for you, @boyland-pf? Fixing #448 would have a bunch of other small benefits as well, but isn't immediately pressing otherwise.

@boyland-pf
Copy link
Contributor Author

It's a low priority for me.

@kkysen kkysen added bug Something isn't working building Build/compile errors or build system-related labels Jul 9, 2022
kkysen added a commit that referenced this issue Jul 22, 2022
@kkysen
Added a new implementation of c2rust-instrument that invokes `cargo…
f8ad844 
…` as a subcommand, overriding `RUSTC_WRAPPER`.

More specifically, instead of using `cargo` as a dependency,
which is enormous and causes some compilation issues (#488),
we invoke `cargo` as a subcommand.
This also has the advantage of using the correct `rustup` `cargo` wrapper so that it resolves correctly.

Then we set `RUSTC_WRAPPER` to our own binary.
We use `RUSTC_WRAPPER` to detect if we're being invoked as a `cargo` or `rustc` wrapper.

If we're the `cargo` wrapper, we parse args using `clap` derive,
determine the crate target info (that we want to instrument)
using `cargo metadata` instead of the massive `cargo` dependency,
and then run `cargo` as a subcommand,
passing the `cargo` args as is (so all normal `cargo` invocations all work),
our own executable as `$RUSTC_WRAPPER`,
and the crate target info and instrumentation args serialized through `$C2RUST_INSTRUMENT_INFO`.

If we're the `rustc` wrapper, we parse the instrument info,
determine if we should instrument the current `rustc` command
if the crate being compiled matches the crate target info passed,
and based on that, either invoke `rustc`
or run `RunCompiler::new(...).run()` with our `MirTransformCallbacks`,
and then save the metadata to the metadata file.

Note that we have to pass the `--sysroot` to `RunCompiler`
as normally `rustc` looks this up relative to its executable's location,
which works when the `rustup` `rustc` wrapper resolves to the toolchain-specific `rustc`,
but since we're using `RunCompiler` directly and being `rustc`,
we need to explicitly set the sysroot.

Instead of passing `c2rust_analysis_rt` as an `--extern` and running `cargo` separately to compile it,
we just add it as a normal dependency to the crate we're instrumenting.
In this way it's similar to `libc`, which can be `extern`ed as it's in the sysroot,
but the preferred way is to specify it as a normal dependency.
@kkysen kkysen mentioned this issue Jul 22, 2022
Merged
@kkysen
Copy link
Contributor

kkysen commented Aug 4, 2022

@boyland-pf, can you confirm if this has been fixed now after #554 landed?

@boyland-pf
Copy link
Contributor Author

Yes, it's been fixed, instrumentation can run on mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working building Build/compile errors or build system-related
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Instrument using $RUSTC_WRAPPER and cargo as a subcommand immunant/c2rust
3 participants
@kkysen @boyland-pf @fw-immunant