ctest support for conflicting headers

[tgross35]

EDIT: this may not be the best proposal, see discussion below.

---

We have a few cases where we bind multiple symbols from headers that can't be included together:pidfd is one example #5191, #5326 will add another. I think it might be possible for ctest to add this reasonably easy, rough sketch:

struct TestGenerator {
    // Move fields like `headers`, `skip`, etc into a reusable struct
    pub(crate) default_group: TestGroupInner,
    pub(crate) groups: Vec<TestGroup>, // new
    // ... existing
}

#[derive(Clone)]
struct TestGroup(Rc<RefCell<TestGroupInner>>);

struct TestGroupInner {
    pub(crate) name: BoxStr,
    pub(crate) headers: ..., // Same as in TestGenerator
    pub(crate) skips: Vec<Skip>,
}

impl TestGenerator {
    // Name must match regex `\p{xid_start}\p{xid_continue}*` (i.e. valid identifier)
    // and must be unique, else panic
    fn group(&mut self, name: &str) -> TestGroup;
    fn groups(&self) -> impl Iterator<Item = TestGroup>;
}

impl TestGroup {
    fn name(&self) -> &str;

    // Same signatures as `TestGenerator`. 
    fn define(...) -> &mut Self;
    fn header(...) -> &mut Self;
    fn header_with_defines(...) -> &mut Self;
    fn skip_struct(&mut self, f: impl Fn(&Struct) -> bool + 'static) -> &mut Self;
    // ... skip_*

    fn matches_struct(&mut self, f: impl Fn(&Struct) -> bool + 'static) -> &mut Self;
    fn matches_static(&mut self, f: impl Fn(&Static) -> bool + 'static) -> &mut Self;
    // ... other types
}

For users:

1. If you have headers that may conflict or want to split the test up for some reason, create a new group
2. Headers and defines can be set for a specific group. Defines are also inherited from the default group
3. If some API should be tested as part of a group, use a matches_* function. This excludes it from the default group

For ctest:

1. Each group gets a separate .c file, including one for the "default" group
2. If not the default group, put the group name in the function names so they don't conflict. E.g. ctest_group1_const_red

This doesn't help us with nested modules but does give us a start. (I think those can be handled with just a fn path(&self) -> &str for Struct, Union, etc.)

[tgross35]

Cc @mbyx for thoughts and if you might be interested in picking this up

[mbyx]

The default group would act exactly as before and be controlled by the methods we already have right? Otherwise we'd have to modify libc-test alot.

Additionally, at a cursory glance, would using two TestGenerator instances not be enough? It's a little hacky, because we'd have to generate multiple .c and .rs files with different names and have to modify the ctest.rs file to include those as well. (I'd like to check if its possible but I'm on CachyOS now and it seems the kernel it uses doesn't play nicely when I attempt to test libc-test).

[tgross35]

The default group would act exactly as before and be controlled by the methods we already have right? Otherwise we'd have to modify libc-test alot.

That's right if you don't have any groups then it would act the same. So no changes should be needed by default.

Additionally, at a cursory glance, would using two TestGenerator instances not be enough? It's a little hacky, because we'd have to generate multiple .c and .rs files with different names and have to modify the ctest.rs file to include those as well.

That does work and we use it a few places, it's just inconvenient to skip certain API in one instance and skip everything except that API in another. But hm, maybe we actually can do this cleanly in libc-test/build.rs? If you have an idea how to make this work, feel free to try adding tests for the pidfd.h bits.

(I'd like to check if its possible but I'm on CachyOS now and it seems the kernel it uses doesn't play nicely when I attempt to test libc-test).

What error are you running into?

[mbyx]

When i first shifted to linux i had expected errors where my kernel version was higher than the one being tested in CI so the values of certain MAX constants was different, but on CachyOS with a fresh clone of rust-lang/libc, I get warnings on the C side of the tests (ctest_output.c) that promote to errors and fail the test before compilation:

...
out/ctest_output.c:42638:28: error: attempt to take address of bit-field structure member 'tcpi_accecn_fail_mode'
warning: libc-test@0.1.0: 42638 |     return offsetof(struct tcp_info, tcpi_accecn_fail_mode);
warning: libc-test@0.1.0:       |                            ^~~~
warning: libc-test@0.1.0: /home/mbyx/Projects/oss/test/libc/target/debug/build/libc-test-d63e8f4d7440ac55/out/ctest_output.c: In function 'ctest_size_of_tcp_info_tcpi_accecn_fail_mode':
warning: libc-test@0.1.0: /home/mbyx/Projects/oss/test/libc/target/debug/build/libc-test-d63e8f4d7440ac55/out/ctest_output.c:42642:18: error: 'sizeof' applied to a bit-field
warning: libc-test@0.1.0: 42642 |     return sizeof(((struct tcp_info){}).tcpi_accecn_fail_mode);
warning: libc-test@0.1.0:       |                  ^
warning: libc-test@0.1.0: /home/mbyx/Projects/oss/test/libc/target/debug/build/libc-test-d63e8f4d7440ac55/out/ctest_output.c: In function 'ctest_offset_of_tcp_info_tcpi_accecn_opt_seen':
warning: libc-test@0.1.0: /home/mbyx/Projects/oss/test/libc/target/debug/build/libc-test-d63e8f4d7440ac55/out/ctest_output.c:42646:28: error: attempt to take address of bit-field structure member 'tcpi_accecn_opt_seen'
warning: libc-test@0.1.0: 42646 |     return offsetof(struct tcp_info, tcpi_accecn_opt_seen);
warning: libc-test@0.1.0:       |                            ^~~~
...

Seems to do with certain struct fields changing to be bitfields in the latest kernel version. Though CachyOS modifies the kernel themselves too, so it might be due to that.

EDIT: I'm able to get tests running to success if I add the following to test_linux:

cfg.skip_struct(|s| {
    s.ident() == "tcp_info"
});

[tgross35]

Thinking about this some more, we might actually hit errors even with two ctest instances. The generated .rs file relies on things like:

extern "C" {
    fn ctest_size_of__pthread_introspection_hook_t() -> u64;
    fn ctest_align_of__pthread_introspection_hook_t() -> u64;
}

I'm pretty sure all Cargo tests get the same libraries available to link (since there aren't separate build.rs files), so if you have >1 ctest instance trying to test a struct pthread_introspection_hook_t then there will be a duplicate symbol error or a quiet failure in one of them. I'm actually surprised we haven't hit this yet, at least that I'm aware of.

[tgross35]

One reasonably easy way I can think of to resolve this: hash as much of TestGenerator as we can (using a non-seeded hasher so it's repeatable), base62-encode that hash, and put it into the function names like ctest_size_of__8MNDZVdA7q0_pthread_introspection_hook_t. That should avoid conflicts if two generators are testing API with the same name.

Not perfect since we can't hash the skip functions, but that's good enough for our purposes since usually if you have separate tests there's a header or define change.

[tgross35]

@dybucc fyi I just though of the above ^ if github.com//pull/5326 is giving you problems, that may be why.

[tgross35]

Actually even easier: in generate_files, extract the name and put that in the symbol names. E.g.

libc/libc-test/build/main.rs

Line 5371 in 023c0a3

ctest::generate_test(&mut cfg, "../src/lib.rs", "linux_fcntl.rs").unwrap();

would have something like ctest_size_of__linux_fcntl__flock.

[dybucc]

Thinking about this some more, we might actually hit errors even with two
ctest instances. The generated .rs file relies on things like:

extern "C" {
   fn ctest_size_of__pthread_introspection_hook_t() -> u64;
   fn ctest_align_of__pthread_introspection_hook_t() -> u64;
}

I'm pretty sure all Cargo tests get the same libraries available to link (since
there aren't separate build.rs files), so if you have >1 ctest instance trying
to test a struct pthread_introspection_hook_t then there will be a duplicate
symbol error or a quiet failure in one of them. I'm actually surprised we
haven't hit this yet, at least that I'm aware of.

I don't quite follow here. I did just that in #5326 and things turned out well
(barring SemVer tests.) Each C file is compiled separately as part of a
different call to ctest::generate_test().

I don't see something immediately wrong with that. Granted, each generated set
of files is used in separate tests/binaries each running isolated from each
other.

[tgross35]

I'd need to poke around a bit to verify but as I'm picturing it, the below will be the problem recipe.

---

Headers:

// foo1.h
struct foo { int x; };

// foo2.h
struct foo { long long x; };

Bindings:

mod foo1 {
    struct foo { x: c_int }
}
mod foo2 {
    struct foo { x: c_longlong }
}

One ctest::TestGenerator that includes foo1.h calling ctest::generate_test(&mut cfg, "/whatever/lib.rs", "foo1.rs");, and one with foo2.h calling `ctest::generate_test(&mut cfg, "/whatever/lib.rs", "foo2.rs"). Producing:

// foo1.c
#include <foo1.h>
ctest_size_of__foo(void) { return sizeof(struct foo); } // returns 4

// foo2.c
#include <foo2.h>
ctest_size_of__foo(void) { return sizeof(struct foo); } // returns 8

// foo1.rs
extern "C" {
    fn ctest_size_of__foo() -> u64;
}

let rust_size = size_of::<foo1::foo>() as u64;
let c_size = unsafe { ctest_size_of__foo() };
check_same(rust_size, c_size, "`foo1::foo` size");

// foo2.rs
extern "C" {
    fn ctest_size_of__foo() -> u64;
}

let rust_size = size_of::<foo2::foo>() as u64;
let c_size = unsafe { ctest_size_of__foo() };
check_same(rust_size, c_size, "`foo2::foo` size");

We get linker args to link both foo1.a and foo2.a (built from the .c files)

---

The problem is that both tests are looking for a symbol ctest_size_of__foo which is present in both foo1.a and foo2.a but has different values.

Curious what I might be missing here if this hasn't shown up

[dybucc]

Except the linker args are issued separately on different linker invocations.
Assuming foo1.rs is eventually include!d in a separate Rust binary from
foo2.rs, it'll be getting a link directive to foo1.a.

I may be missing something in the way ctest handles this, but from skimming
through the code, two separate ctest instances will compile whichever C files
and headers get issued in separate compiler invocations.

Then once we're back on the Rust side of things, running each of the tests that
use foo1.rs and foo2.rs won't cause issues so long as they're in separate
tests/binaries.

[tgross35]

They're different final binaries but I don't think that's enough. Because generate_test calls cc::Build::compile, I think we're getting a cargo:rustc-link-lib directive for each TestGenerator instances, and I think those directives go to all binaries (main library, integration tests, etc).