Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency

[Zhao Liu <zhao1.liu@intel.com>]

Hi Manos,

Thanks for your patch. I have a few comments below:

> diff --git a/meson.build b/meson.build
> index 3533889852..2b305e745a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3876,6 +3876,62 @@ foreach target : target_dirs
>      lib_deps += dep.partial_dependency(compile_args: true, includes: true)
>    endforeach
>  
> +  if with_rust and target_type == 'system'
> +       # FIXME:

Just nit, FIXME looks a bit ambiguous, is it intended to fix the
following warning? It could be a bit more specific.

> +       # > WARNING: Project specifies a minimum meson_version '>=0.63.0' but
> +       # > uses features which were added in newer versions:
> +       # > * 0.64.0: {'fs.copyfile'}
> +       # > * 1.0.0: {'dependencies arg in rust.bindgen', 'module rust as stable module'}
> +      rust_bindgen = import('rust')
> +
> +      # We need one generated_rs target per target, so give them
> +      # target-specific names.
> +      copy = fs.copyfile('rust/wrapper.h',
> +                         target + '_wrapper.h')
> +      generated_rs = rust_bindgen.bindgen(
> +        input: copy,
> +        dependencies: arch_deps + lib_deps,
> +        output: target + '-generated.rs',
> +        include_directories: include_directories('.', 'include'),
> +        args: [
> +          '--ctypes-prefix', 'core::ffi',
> +          '--formatter', 'rustfmt',
> +          '--generate-block',
> +          '--generate-cstr',
> +          '--impl-debug',
> +          '--merge-extern-blocks',
> +          '--no-doc-comments',
> +          '--no-include-path-detection',
> +          '--use-core',
> +          '--with-derive-default',
> +          '--allowlist-file', meson.project_source_root() + '/include/.*',
> +          '--allowlist-file', meson.project_source_root() + '/.*',
> +          '--allowlist-file', meson.project_build_root() + '/.*'
> +        ],
> +      )
> +
> +      if target in rust_targets
> +        rust_hw = ss.source_set()
> +        foreach t: rust_targets[target]
> +          rust_device_cargo = custom_target(t['name'],
> +                                       output: t['output'],
> +                                       depends: [generated_rs],
> +                                       build_always_stale: true,
> +                                       command: t['command'])
> +          rust_dep = declare_dependency(link_args: [
> +                                          '-Wl,--whole-archive',
> +                                          t['output-path'],
> +                                          '-Wl,--no-whole-archive'
> +                                          ],
> +                                          sources: [rust_device_cargo])
> +          rust_hw.add(rust_dep)
> +        endforeach
> +        rust_hw_config = rust_hw.apply(config_target, strict: false)
> +        arch_srcs += rust_hw_config.sources()
> +        arch_deps += rust_hw_config.dependencies()
> +      endif
> +  endif
> +
>    lib = static_library('qemu-' + target,
>                   sources: arch_srcs + genh,
>                   dependencies: lib_deps,
> diff --git a/rust/.gitignore b/rust/.gitignore
> new file mode 100644
> index 0000000000..1bf71b1f68
> --- /dev/null
> +++ b/rust/.gitignore
> @@ -0,0 +1,3 @@
> +# Ignore any cargo development build artifacts; for qemu-wide builds, all build
> +# artifacts will go to the meson build directory.
> +target
> diff --git a/rust/meson.build b/rust/meson.build
> new file mode 100644
> index 0000000000..435abd3e1c
> --- /dev/null
> +++ b/rust/meson.build
> @@ -0,0 +1,129 @@
> +# Supported hosts
> +rust_supported_oses = {
> +  'linux': '-unknown-linux-gnu',
> +  'darwin': '-apple-darwin',
> +  'windows': '-pc-windows-gnu'
> +}

Does the current test cover windows and apple? If not, I think we can
add these two platforms later on.

> +rust_supported_cpus = ['x86_64', 'aarch64']

Similarly, here I have another question, x-pl011-rust in arm virt machine
I understand should only run on ARM platform, right? So compiling to
x86_64 doesn't seem necessary at the moment?

> +# Future-proof the above definitions against any change in the root meson.build file:
> +foreach rust_os: rust_supported_oses.keys()
> +  if not supported_oses.contains(rust_os)
> +    message()
> +    warning('UNSUPPORTED OS VALUES IN ' + meson.current_source_dir() + '/meson.build')
> +    message()
> +    message('This meson.build file claims OS `+' + rust_os + '` is supported but')
> +    message('it is not included in the global supported OSes list in')
> +    message(meson.global_source_root() + '/meson.build.')
> +  endif
> +endforeach
> +foreach rust_cpu: rust_supported_cpus
> +  if not supported_cpus.contains(rust_cpu)
> +    message()
> +    warning('UNSUPPORTED CPU VALUES IN ' + meson.current_source_dir() + '/meson.build')
> +    message()
> +    message('This meson.build file claims CPU `+' + rust_cpu + '` is supported but')
> +    message('it is not included in the global supported CPUs list in')
> +    message(meson.global_source_root() + '/meson.build.')
> +  endif
> +endforeach
> +
> +# FIXME: retrieve target triple from meson and construct rust_target_triple
> +if meson.is_cross_build()
> +  message()
> +  error('QEMU does not support cross compiling with Rust enabled.')
> +endif

Is the check here to avoid inconsistencies between cross-build's target
and rust_target_triple?

If target consistency is not guaranteed, then it seems custom rust_target_triple
is useless, right? please educate me if I am wrong ;-)

> +# FIXME: These are the latest stable versions, refine to actual minimum ones.
> +msrv = {
> +  'rustc': '1.79.0',
> +  'cargo': '1.79.0',
> +  'bindgen': '0.69.4',
> +}
> +
> +# rustup = find_program('rustup', required: false)
> +foreach bin_dep: msrv.keys()
> +  bin = find_program(bin_dep, required: true)
> +  if bin.version() < msrv[bin_dep]
> +    if bin_dep == 'bindgen' and get_option('wrap_mode') != 'nodownload'

What about adding a download info here?

It took a long time for my bindgen-cli to update quietly, and for a
moment I thought it was stuck here...

e.g., message('Installing bindgen-cli...')

Or can we just report the error and let users install bindgen-cli
themselves?

> +      run_command(cargo, 'install', 'bindgen-cli', capture: true, check: true)
> +      bin = find_program(bin_dep, required: true)
> +      if bin.version() >= msrv[bin_dep]
> +        continue
> +      endif
> +    endif
> +    message()
> +    error(bin_dep + ' version ' + bin.version() + ' is unsupported: Please upgrade to at least ' + msrv[bin_dep])
> +  endif
> +endforeach
> +
> +rust_target_triple = get_option('with_rust_target_triple')
> +
> +if rust_target_triple == ''
> +  if not supported_oses.contains(host_os)
> +    message()
> +    error('QEMU does not support `' + host_os +'` as a Rust platform.')
> +  elif not supported_cpus.contains(host_arch)
> +    message()
> +    error('QEMU does not support `' + host_arch +'` as a Rust architecture.')
> +  endif
> +  rust_target_triple = host_arch + rust_supported_oses[host_os]
> +  if host_os == 'windows' and host_arch == 'aarch64'
> +    rust_target_triple += 'llvm'
> +  endif
> +else
> +  # verify rust_target_triple if given as an option
> +  rustc = find_program('rustc', required: true)
> +  rustc_targets = run_command(rustc, '--print', 'target-list', capture: true, check: true).stdout().strip().split()
> +  if not rustc_targets.contains(rust_target_triple)
> +    message()
> +    error('Given rust_target_triple ' + rust_target_triple + ' is not listed in rustc --print target-list output')
> +  endif

It seems rust_target_triple missed support check, we should use rust_supported_cpus
+ rust_supported_oses to check rust_target_triple, right? ;-)

> +endif
> +
> +

An extra blank line :-)

> +rust_targets = {}
> +
> +cargo_wrapper = [
> +  find_program(meson.global_source_root() / 'scripts/cargo_wrapper.py'),
> +  '--config-headers', meson.project_build_root() / 'config-host.h',
> +  '--meson-build-root', meson.project_build_root(),
> +  '--meson-build-dir', meson.current_build_dir(),
> +  '--meson-source-dir', meson.current_source_dir(),
> +]
> +
> +if get_option('b_colorout') != 'never'
> +  cargo_wrapper += ['--color', 'always']
> +endif
> +
> +if get_option('optimization') in ['0', '1', 'g']
> +  rs_build_type = 'debug'
> +else
> +  rs_build_type = 'release'
> +endif

Could we decouple 'optimization' with cargo's opt-level (maybe in the future)?

Or we could add new option to specify custom rust opt-level, which will
set the environment varialbe CARGO_PROFILE_<profile_name>_OPT_LEVEL and
override the default opt-level in Cargo.toml.

> +# Collect metadata for each (crate,qemu-target,compiler-target) combination.
> +# Rust meson targets cannot be defined a priori because they depend on bindgen
> +# generation that is created for each emulation target separately. Thus Rust
> +# meson targets will be defined for each target after the target-specific
> +# bindgen dependency is declared.
> +rust_hw_target_list = {}
> +
> +foreach rust_hw_target, rust_hws: rust_hw_target_list
> +  foreach rust_hw_dev: rust_hws
> +    output = meson.current_build_dir() / rust_target_triple / rs_build_type / rust_hw_dev['output']
> +    crate_metadata = {
> +      'name': rust_hw_dev['name'],
> +      'output': [rust_hw_dev['output']],
> +      'output-path': output,
> +      'command': [cargo_wrapper,
> +        '--crate-dir', meson.current_source_dir() / rust_hw_dev['dirname'],
> +        '--profile', rs_build_type,
> +        '--target-triple', rust_target_triple,
> +        '--outdir', '@OUTDIR@',
> +        'build-lib'
> +        ]
> +      }
> +    rust_targets += { rust_hw_target: [crate_metadata] }
> +  endforeach
> +endforeach
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> new file mode 100644
> index 0000000000..bcf808c8d7
> --- /dev/null
> +++ b/rust/wrapper.h
> @@ -0,0 +1,39 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2020 Fabrice Bellard

Here should the author be yourself?

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qemu-io.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/sysbus.h"
> +#include "exec/memory.h"
> +#include "chardev/char-fe.h"
> +#include "hw/clock.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/irq.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "chardev/char-serial.h"

Thanks,
Zhao