Skip to content
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

Add all definitions from linux/ptp-clock.h #3865

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

folkertdev
Copy link
Contributor

code is the same as #3559, but rebased on main instead of 0.2

r? @tgross35

@folkertdev
Copy link
Contributor Author

hmm this needs some extra data types now, and reading kernel C code will need a better-rested brain. I'll continue with this tomorrow.

Copy link
Contributor Author

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hits a CI error that I don't understand

  --- stderr
  thread 'main' panicked at /cargo/registry/src/index.crates.io-6f17d22bba15001f/garando_syntax-0.1.1/src/ext/expand.rs:337:21:
  internal error: entered unreachable code
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

so that is here https://github.com/JohnTitor/garando/blob/master/garando_syntax/src/ext/expand.rs#L337

but there is no context about what actually failed in this case.

@rustbot review

in the sense that I'm kind of stuck

Comment on lines 6294 to 6296
mod ioctl {
const _IOC_NRBITS: u32 = 8;
const _IOC_TYPEBITS: u32 = 8;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this move somewhere else?

I also get a style error on the const definition

src/unix/linux_like/linux/mod.rs:6295: constant found after module when it belongs before

not sure what that is about

@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2024

Failed to set assignee to ready: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

1 similar comment
@rustbot
Copy link
Collaborator

rustbot commented Aug 22, 2024

Failed to set assignee to ready: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@tgross35
Copy link
Contributor

tgross35 commented Aug 28, 2024

This hits a CI error that I don't understand

  --- stderr
  thread 'main' panicked at /cargo/registry/src/index.crates.io-6f17d22bba15001f/garando_syntax-0.1.1/src/ext/expand.rs:337:21:
  internal error: entered unreachable code
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

so that is here https://github.com/JohnTitor/garando/blob/master/garando_syntax/src/ext/expand.rs#L337

but there is no context about what actually failed in this case.

@rustbot review

in the sense that I'm kind of stuck

Ah, I don't know anything about this crate but I suspect it will need to be patched somehow. I don't know what that would be (I have never worked with that crate), I know it has problems parsing some newer syntax.

Since this is seemingly blocked on that:

@rustbot blocked

@tgross35
Copy link
Contributor

If you aren't up for figuring out how to fix that, you could also just try disabling chunks of these changes until things work. Not ideal, but maybe better than waiting.

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Aug 28, 2024
@folkertdev
Copy link
Contributor Author

well I did just reproduce this locally and that library just performs an out-of-bounds lookup

  thread 'main' panicked at library/core/src/panicking.rs:219:5:
  unsafe precondition(s) violated: slice::get_unchecked requires that the index is within the slice
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/std/src/panicking.rs:652:5
     1: core::panicking::panic_nounwind_fmt::runtime
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:110:18
     2: core::panicking::panic_nounwind_fmt
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:120:5
     3: core::panicking::panic_nounwind
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/panicking.rs:219:5
     4: <usize as core::slice::index::SliceIndex<[T]>>::get_unchecked::precondition_check
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ub_checks.rs:68:21
     5: <usize as core::slice::index::SliceIndex<[T]>>::get_unchecked
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/ub_checks.rs:75:17
     6: core::slice::<impl [T]>::get_unchecked
               at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library/core/src/slice/mod.rs:686:20
     7: <alloc::vec::Vec<T> as garando_syntax::util::move_map::MoveMap<T>>::move_flat_map
               at /home/folkertdev/.cargo/registry/src/index.crates.io-6f17d22bba15001f/garando_syntax-0.1.0/src/util/move_map.rs:32:35
     8: garando_syntax::fold::fold_attrs
               at /home/folkertdev/.cargo/registry/src/index.crates.io-6f17d22bba15001f/garando_syntax-0.1.0/src/fold.rs:329:5
     9: garando_syntax::fold::noop_fold_item_simple
               at /home/folkertdev/.cargo/registry/src/index.crates.io-6f17d22bba15001f/garando_syntax-0.1.0/src/fold.rs:1111:16

I'll see what we can do here. My guess is const fn being the most recent/weird bit of syntax here?

@tgross35
Copy link
Contributor

Well that's concerning.

I'll see what we can do here. My guess is const fn being the most recent/weird bit of syntax here?

Ah, that's a good guess. We have a macro that handles this, search for things with {const} syntax. This needs to change at some point before 1.0, but should work here if that is the problem.

@folkertdev
Copy link
Contributor Author

btw that out-of-bounds access also happens on the current main branch locally for me. I tried upgrading ctest2 to the latest version, and it appears to just be broken (some function inside ctest2 expects another argument). So yeah, not great.

That also means that I can't actually locally reproduce the CI error unless I start messing with docker (which, I really don't want to do)

@folkertdev folkertdev force-pushed the add-ptp-clock-main branch 3 times, most recently from 81cd5c7 to d558fe2 Compare August 28, 2024 22:22
@folkertdev
Copy link
Contributor Author

well, I figured it out. There were really two syntax problems that this crate cannot parse.

one issue is the expression for the length of the array

pub struct ptp_sys_offset {
    pub n_samples: ::c_uint,
    pub rsv: [::c_uint; 3],
    pub ts: [ptp_clock_time; 2 * PTP_MAX_SAMPLES as usize + 1],
}

which gave an error like this

  --- stderr
  rust version: 1.82.0-nightly
  thread 'main' panicked at /cargo/registry/src/index.crates.io-6f17d22bba15001f/ctest2-0.4.8/src/lib.rs:2233:26:
  unknown op: Spanned { node: Mul, span: Span { lo: BytePos(1434930), hi: BytePos(1434931), ctxt: #1760 } }

and then also the debug_asserts here cannot be parsed, giving the error we saw before

/// Build an ioctl number, analogous to the C macro of the same name.
const fn _IOC(dir: u32, ty: u32, nr: u32, size: usize) -> u32 {
    debug_assert!(dir <= _IOC_DIRMASK);
    debug_assert!(ty <= _IOC_TYPEMASK);
    debug_assert!(nr <= _IOC_NRMASK);
    debug_assert!(size <= (_IOC_SIZEMASK as usize));

    (dir << _IOC_DIRSHIFT)
        | (ty << _IOC_TYPESHIFT)
        | (nr << _IOC_NRSHIFT)
        | ((size as u32) << _IOC_SIZESHIFT)
}

so, I guess we just use a literal for that length, and I've commented the asserts for now. I am seeing another error that I'm not sure about. These constants are defined like so many of the other constants, so is it more that these constants are not defined in the libc that we're comparing with? They are defined here https://github.com/torvalds/linux/blob/928f79a188aacc057ba36c85b36b6d1e99c8f595/include/uapi/linux/ptp_clock.h#L234 and don't seem special otherwise.

  cargo:warning=/checkout/target/i686-unknown-linux-gnu/debug/build/libc-test-fa10219da99c25be/out/main.c:45289:77: error: 'PTP_MASK_CLEAR_ALL' undeclared here (not in a function)
  cargo:warning=45289 |             static const unsigned int __test_const_PTP_MASK_CLEAR_ALL_val = PTP_MASK_CLEAR_ALL;
  cargo:warning=      |                                                                             ^~~~~~~~~~~~~~~~~~
  cargo:warning=/checkout/target/i686-unknown-linux-gnu/debug/build/libc-test-fa10219da99c25be/out/main.c:45295:77: error: 'PTP_MASK_EN_SINGLE' undeclared here (not in a function)
  cargo:warning=45295 |             static const unsigned int __test_const_PTP_MASK_EN_SINGLE_val = PTP_MASK_EN_SINGLE;
  cargo:warning=      |                                                                             ^~~~~~~~~~~~~~~~~~

also there is some musl stuff that I'll need to figure out. Anyway, progress!

@folkertdev
Copy link
Contributor Author

@tgross35 how do you recommend implementing this?

cfg_if! {
    if #[cfg(any(target_arch = "sparc", target_arch = "sparc64"))] {
        s!{
            pub struct ptp_clock_caps {
                pub max_adj: ::c_int,
                pub n_alarm: ::c_int,
                pub n_ext_ts: ::c_int,
                pub n_per_out: ::c_int,
                pub pps: ::c_int,
                pub n_pins: ::c_int,
                pub cross_timestamping: ::c_int,
                pub adjust_phase: ::c_int,
                pub rsv: [::c_int; 12],
            }
        }
    } else if #[cfg(any(target_env = "musl", target_env = "ohos"))] {
        s!{
            pub struct ptp_clock_caps {
                pub max_adj: ::c_int,
                pub n_alarm: ::c_int,
                pub n_ext_ts: ::c_int,
                pub n_per_out: ::c_int,
                pub pps: ::c_int,
                pub n_pins: ::c_int,
                pub cross_timestamping: ::c_int,
                pub rsv: [::c_int; 13],
            }
        }
    } else {
        s! {
            pub struct ptp_clock_caps {
                pub max_adj: ::c_int,
                pub n_alarm: ::c_int,
                pub n_ext_ts: ::c_int,
                pub n_per_out: ::c_int,
                pub pps: ::c_int,
                pub n_pins: ::c_int,
                pub cross_timestamping: ::c_int,
                pub adjust_phase: ::c_int,
                pub max_phase_adj: ::c_int,
                pub rsv: [::c_int; 11],
            }
        }
    }
}

it runs into a lint of having more than one s! per module. I can sort of inline the cfgs but that looks awful. cfg_if! can't be parsed when it's inside of the s! macro. So, ?

@folkertdev
Copy link
Contributor Author

@rustbot review

@bors
Copy link
Contributor

bors commented Sep 3, 2024

☔ The latest upstream changes (presumably #3480) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review stable-nominated This PR should be considered for cherry-pick to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants