Skip to content

Update clang flag in cpp.md #1852

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

Closed
wants to merge 1 commit into from
Closed

Conversation

virtualritz
Copy link

Removed excess space arg that will cause a panic when used with .clang_arg().
Fixed style.

Removed excess space arg that will cause a panic when used with `.clang_arg()`.
Fixed style.
@kulp
Copy link
Member

kulp commented Jul 30, 2020

I imagine this is the kind of error you are seeing if you do cargo build -vv :

[2020-07-30T04:18:28Z DEBUG bindgen] Generating bindings, libclang at /usr/local/opt/llvm@9/lib/libclang.dylib
[2020-07-30T04:18:28Z DEBUG bindgen] Trying to find clang with flags: ["-x c++"]
[2020-07-30T04:18:28Z DEBUG bindgen] Found clang: Clang { path: "/usr/bin/clang", version: Some(CXVersion { Major: 10, Minor: 0, Subminor: 1 }), c_search_paths: None, cpp_search_paths: None }
[2020-07-30T04:18:28Z DEBUG bindgen] Fixed-up options: BindgenOptions { blacklisted_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, blacklisted_functions: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, blacklisted_items: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, opaque_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, rustfmt_path: None, whitelisted_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, whitelisted_functions: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, whitelisted_vars: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, default_enum_style: Consts, bitfield_enums: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, newtype_enums: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, rustified_enums: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, rustified_non_exhaustive_enums: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, constified_enum_modules: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, constified_enums: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, default_alias_style: TypeAlias, type_alias: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, new_type_alias: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, new_type_alias_deref: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, builtins: false, emit_ast: false, emit_ir: false, emit_ir_graphviz: None, enable_cxx_namespaces: false, enable_function_attribute_detection: false, disable_name_namespacing: false, disable_nested_struct_naming: false, disable_header_comment: false, layout_tests: true, impl_debug: false, impl_partialeq: false, derive_copy: true, derive_debug: true, derive_default: false, derive_hash: false, derive_partialord: false, derive_ord: false, derive_partialeq: false, derive_eq: false, use_core: false, ctypes_prefix: None, time_phases: false, namespaced_constants: true, msvc_mangling: false, convert_floats: true, raw_lines: [], module_lines: {}, clang_args: ["-x c++", "wrapper.hpp"], input_header: Some("wrapper.hpp"), input_unsaved_files: [], parse_callbacks: None, codegen_config: FUNCTIONS | TYPES | VARS | METHODS | CONSTRUCTORS | DESTRUCTORS, conservative_inline_namespaces: false, generate_comments: true, generate_inline_functions: false, whitelist_recursively: true, objc_extern_crate: false, generate_block: false, block_extern_crate: false, enable_mangling: true, detect_include_paths: true, prepend_enum_name: true, rust_target: Stable_1_40, rust_features: RustFeatures { untagged_union: true, associated_const: true, builtin_clone_impls: true, repr_align: true, i128_and_u128: true, must_use_function: true, repr_transparent: true, min_const_fn: true, core_ffi_c_void: true, repr_packed_n: true, maybe_uninit: true, non_exhaustive: true, thiscall_abi: false }, record_matches: true, size_t_is_usize: false, rustfmt_bindings: true, rustfmt_configuration_file: None, no_partialeq_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, no_copy_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, no_hash_types: RegexSet { items: [], matched: [], set: Some(RegexSet([])), record_matches: true }, array_pointers_in_arguments: false, wasm_import_module_name: None }
thread 'main' panicked at 'libclang error; possible causes include:
- Invalid flag syntax
- Unrecognized flags
- Invalid flag arguments
- File I/O errors
- Host vs. target architecture mismatch
If you encounter an error missing from this list, please file an issue or a PR!', /Users/kulp/.cargo/registry/src/git.colasdn.top-1ecc6299db9ec823/bindgen-0.54.1/src/ir/context.rs:567:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I think -x c++ is meant to work, though, since it is supported by clang on the command-line. I am not sure that the right thing is to change the docs yet.

However, if we should change the docs in the short term, then we should update book/src/objc.md too.

@virtualritz
Copy link
Author

virtualritz commented Jul 30, 2020

Some developer on the Rust Discord suggested that the space after -c is the issue. Aka: -xc++ will work as advertised. That was the main edit I did.
While at it I also corrected a pleonasm and a grammar mistake. :]

@virtualritz
Copy link
Author

virtualritz commented Jul 30, 2020

I can confirm this works. Aka, sending a C++ header ending with .h to bindgen without -xc++:

  thread 'main' panicked at 'Unable to generate bindings: ()', build.rs:82:10
  stack backtrace:
     0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
     1: core::fmt::write
     2: std::io::Write::write_fmt
     3: std::panicking::default_hook::{{closure}}
     4: std::panicking::default_hook
     5: std::panicking::rust_panic_with_hook
     6: rust_begin_unwind
     7: core::panicking::panic_fmt
     8: core::result::unwrap_failed
     9: core::result::Result<T,E>::expect
    10: build_script_build::main
    11: std::rt::lang_start::{{closure}}
    12: std::rt::lang_start_internal
    13: std::rt::lang_start
    14: main
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Adding .clang_arg("-xc++") to the Builder makes the build succeed. So the corrections I submitted in this PR are right as they are and make this work as advertised in the docs.

@virtualritz
Copy link
Author

I added another PR for book/src/objc.md, as you suggested.

@kulp
Copy link
Member

kulp commented Jul 31, 2020

I do not think it is necessary or helpful to have a separate PR per Markdown file. If we update the documentation, it would be better for the related updates to occur in one PR.

However, I think we still need to agree on a few things.

The space between -x and c++ is not "wrong" as you say in #1854. The clang compiler, and thus libclang, inherits the -x option from GCC, and GCC's docs show that -x language, with a space, is the expected syntax for GCC. It is true that clang's documentation shows -x without a space, but this is a mere stylistic difference, since clang on the command-line accepts -x c++ with no problems.

The problem is shown when you look carefully at the backtrace. There is a space before c++. The problem is that libclang is (due to quoting rules being different when a shell is not involved) including the space in the name of the language. This is a shell quoting problem that bindgen can fix.

error: language not recognized: ' c++', err: true
warning: '-x c++': 'linker' input unused [-Wunused-command-line-argument], err: false
thread 'main' panicked at 'Couldn't make bindings!: ()', build.rs:4:20
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: std::panicking::rust_panic_with_hook
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::result::unwrap_failed
   9: core::result::Result<T,E>::expect
  10: build_script_build::main
  11: std::rt::lang_start::{{closure}}
  12: std::rt::lang_start_internal
  13: std::rt::lang_start
  14: main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I believe compiler documentation online is going to show the space more often than not, and that changing bindgen's documentation is not the answer.

We can already see multiple examples in bindgen's source code where -x c++ is passed to libclang as separate arguments :

.clang_arg("-x")
.clang_arg("c++")

.clang_args(&["-x", "c++", "-std=c++11"])

and there are also many files like this :

// bindgen-flags: -- -x c++ --std=c++11

Here is an build.rs that shows some possibilities :

use std::path::PathBuf;

fn main() {
    let bindings = bindgen::Builder::default()
        .header("wrapper.hpp")
        //.clang_arg("-x c++")            // does not work
        .clang_arg("'-x c++'")            // works, probably should not be used
        .clang_args(&["-x", "c++"])       // works
        .clang_arg("-x").clang_arg("c++") // works (see tests/stylo_sanity.rs)
        .generate()
        .expect("Couldn't make bindings!");

    let out_path = PathBuf::from(std::env::var("OUT_DIR").unwrap());
    bindings.write_to_file(out_path.join("bindings.rs"))
        .expect("Couldn't write bindings!");
}

My points are that

  1. @virtualritz found a real issue in bindgen -- thank you.
  2. There are many good reasons that we should expect -x c++ to work everywhere.
  3. Updating the documents is not the right solution.

Thank you for finding this, @virtualritz. I have created #1855 to track this issue. We can discuss the right solution further there.

@kulp kulp closed this Jul 31, 2020
@kulp kulp mentioned this pull request Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants