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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

Hello Zhao!

On Mon, 24 Jun 2024 13:12, Zhao Liu <zhao1.liu@intel.com> wrote:
>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.


Yes it could, that's true. Basically meson prints this warning when it 
reconfigures the build targets. So to use these newer features in our 
minimum supported meson version the code should be fixed to be backwards 
compatible.

I will add a sentence explaining that this is meson's output.

>
>> +       # > 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.

Do you mean the test for supported OSes?

This is for future-proofing the Rust integration in general. I haven't 
been able to compile under macos yet because bindgen cannot find the 
system clang header. I also don't have a windows pc to test it on. But 
it should work theoretically under all three.

>
>> +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?

This refers to the host cpu not the virtualized ones, you can run 
qemu-system-aarch64 under x86_64


>> +# 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 ;-)

It is, correct. I was hoping someone on the list would have advice on 
how to best do this (see the FIXME comment right above)

>
>> +# 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?

Now that I think about it again I agree it's better to not mess with 
user/system installations without asking.

>> +      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.

optimization here refers to LLVM's opt flags directly, so I think it 
makes sense to follow the project-wide compiler settings?
>
>> +# 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?

Technically I copy pasted this file from the main.c executable so it's a 
derived work :D

>
>> + *
>> + * 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
>