autocxx icon indicating copy to clipboard operation
autocxx copied to clipboard

Incorrect binding code for va_list generated when targeting aarch64

Open pcc opened this issue 1 year ago • 5 comments

Expected Behavior

Declarations using va_list are ignored when targeting aarch64, same as on x86.

Actual Behavior

Code is generated that causes a build error.

When targeting aarch64 Linux:

  cargo:warning=/home/pcc/autocxx-va-list-test/target/debug/build/autocxx-va-list-test-b0bfa856bea115b4/out/autocxx-build-dir/cxx/gen0.cxx: In function ‘void cxxbridge1$Test$foo(Test&, std::array<long unsigned int, 4>*)’:
  cargo:warning=/home/pcc/autocxx-va-list-test/target/debug/build/autocxx-va-list-test-b0bfa856bea115b4/out/autocxx-build-dir/cxx/gen0.cxx:137:60: error: cannot convert ‘void (Test::*)(va_list)’ to ‘void (Test::*)(std::array<long unsigned int, 4>)’ in initialization
  cargo:warning=  137 |   void (::Test::*foo$)(::std::array<::std::uint64_t, 4>) = &::Test::foo;
  cargo:warning=      |                                                            ^~~~~~~~~~~~

When targeting aarch64 Android:

warning: /usr/local/google/home/pcc/src/autocxx-va-list-test/target/aarch64-linux-android/debug/build/autocxx-va-list-test-6c15445a434365c7/out/autocxx-build-dir/cxx/gen0.cxx:137:18: error: cannot initialize a variable of type 'void (Test::*)(::std::array< ::std::uint64_t, 4>)' with an rvalue of type 'void (Test::*)(va_list)': type mismatch at 1st parameter ('::std::array< ::std::uint64_t, 4>' (aka 'array<unsigned long, 4>') vs 'va_list' (aka '__builtin_va_list'))
warning:   void (::Test::*foo$)(::std::array<::std::uint64_t, 4>) = &::Test::foo;
warning:                  ^                                         ~~~~~~~~~~~~
warning: 1 error generated.

Steps to Reproduce the Problem

  1. Download the attached project autocxx-va-list-test.tar.gz and extract it somewhere

Either:

  1. On a native aarch64 Linux machine, run cargo build

or:

  1. Download Android NDK r25c from https://developer.android.com/ndk/downloads and extract it somewhere
  2. Build targeting Android by passing the path to the NDK to the included script: ./cargo-build-android.sh /path/to/android-ndk-r25c

Specifications

  • Version: 0.25.0
  • Platform: x86 Linux host/aarch64 Android target, native aarch64 Linux

pcc avatar Mar 21 '23 05:03 pcc

Thanks - do you think you could raise a PR containing a minimal test case?

adetaylor avatar Mar 21 '23 08:03 adetaylor

I've added a naïve test for va_list in the hopes that it can reveal the problem on CI, but even once I've fixed it, it still doesn't help me very much. I need to be able to reproduce this locally. That will require running the creduce pipeline to get a minimal test case using the particular va_list headers present on these platforms. Please check out the link I posted above regarding generating a JSON failure case which hopefully I can then send through the reduction pipeline.

adetaylor avatar Mar 29 '23 13:03 adetaylor

repro.json.gz Did you try reproducing using the Android NDK and the sample project I provided? That should be easier than getting access to an aarch64 machine.

I generated a repro.json file on my aarch64 machine using the sample project, which I've attached. It seems to be possible to reduce it on an x86_64 machine with a command such as

target/debug/autocxx-reduce --problem 'cannot initialize' -k --clang-arg=-std=c++17 --creduce-arg=--n --creduce-arg=192 --clang-arg=--target=aarch64-linux-gnu repro -r repro.json

but this was running very slowly for me and did not complete after several hours.

Your test case was failing on both aarch64 and x86_64 unless I replaced #include <array> with include <stdarg.h>. After making that change, the test passes on x86_64 and fails on aarch64, like so:

cargo:rustc-env=AUTOCXX_RS=/tmp/.tmpLMLCrf/target/rs
thread 'integration_test::test_ignore_va_list' panicked at 'called `Result::unwrap()` on an `Err` value: AutoCxx(ParseError(AutocxxCodegenError(Conversion(Cpp(UnsupportedType("[u64 ; 4usize]"))))))', integration-tests/tests/integration_test.rs:12079:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

By the way, running cargo test --all revealed a pre-existing test failure on aarch64. I was seeing this:

   Compiling autocxx-integration-tests-tests v0.0.0 (/home/pcc/autocxx/target/tests/autocxx-integration-tests)
    Finished dev [unoptimized + debuginfo] target(s) in 0.19s


test /tmp/.tmpq24wIe/input.rs ... error
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0308]: mismatched types
 --> /tmp/.tmpq24wIe/input.rs:1:350
  |
1 | ... () . make_char (c1) ; assert_eq ! (unsafe { ch . as_ref () } . unwrap () , & 104i8) ; assert_eq ! (unsafe { a . as_ref () . unwrap ()...
  |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
  |
  = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: can't compare `u8` with `i8`
 --> /tmp/.tmpq24wIe/input.rs:1:350
  |
1 | ... . make_char (c1) ; assert_eq ! (unsafe { ch . as_ref () } . unwrap () , & 104i8) ; assert_eq ! (unsafe { a . as_ref () . unwrap () . ...
  |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no implementation for `u8 == i8`
  |
  = help: the trait `PartialEq<i8>` is not implemented for `u8`
  = help: the trait `PartialEq` is implemented for `u8`
  = note: required for `&u8` to implement `PartialEq<&i8>`
  = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈



test integration_test::test_take_char_by_ptr_in_wrapped_method ... FAILED

I think this could be related to aarch64 defaulting to -funsigned-char.

pcc avatar Mar 30 '23 02:03 pcc

Reduction gave

struct Test;
namespace std {
template <typename, typename> class a;
template <typename b, typename ac = b> class unique_ptr {
  using ad = typename a<b, ac>::c;
  ad release();
};
typedef char string;
} // namespace std
namespace rust {
namespace {
class Str {};
} // namespace
} // namespace rust

Sometimes the results of the reduction process make no sense so I'll give that a try later and find out. Posting it here so I don't lose it.

adetaylor avatar Mar 30 '23 13:03 adetaylor

I got a chance to look at the reduction output and it isn't valuable. I also added a couple of tests which failed to reproduce either problem, and I checked, and there are no github ARM64 executors. So I've tried all the easy things.

The next step would be to raise a PR against the github actions to run the test suite against the NDK. @pcc are you interested in doing that? There are two reasons:

  1. even carefully-reported bugs like this often turn out not to be reproducible for me after hours of trying, and having a failing github action is proof that I'll be able to repro the bug myself.
  2. once I fix it, I don't want it to regress.

If you don't think you'll have time for such a PR, I will aim to get around to it, but my spare time for maintaining autocxx is limited so it might not be soon. Let me know!

adetaylor avatar Apr 02 '23 02:04 adetaylor