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

Use of "rustified" enums in ctre-sys #1

Closed
Lytigas opened this issue Jul 15, 2018 · 4 comments
Closed

Use of "rustified" enums in ctre-sys #1

Lytigas opened this issue Jul 15, 2018 · 4 comments

Comments

@Lytigas
Copy link

Lytigas commented Jul 15, 2018

If the called C/C++ returns an enum out of the range of the defined rust enum, UB results.

Rust-bindgen has the constified enum variant, though its not as ergonomic. What is the current plan to deal with API/ABI breaking updates to Phoenix? Will someone have to comb through all the enums every time we want to update? That's why, personally, I much prefer generating bindings at compile time, and working around the results. Looking at the diffs of rust-bindgens output and the current code, I don't know how big of a change that would be.

The approach I took with first-rust-competition was to use constified enums with a macro to make the identifiers more concise/DRY at the *-sys level. In the public API actual rust enums are used that implement Into<u32> (that's the plan anyway).

@auscompgeek
Copy link
Member

auscompgeek commented Jul 16, 2018

So the intent would probably be to link to the static library. That way there wouldn't be any surprises for anyone using this crate.

I'm still trying to figure out why I'm getting this link error though when building the example I have:

  = note: "arm-frc-linux-gnueabi-gcc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-L" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.1y16o1qfye96o7m0.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.2b8rw6w5d47qwicy.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.3hfnnvlph5ozv32c.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.3rngp6bm2u2q5z0y.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.3xh1n10tuc29bssp.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.4oc10dk278mpk1vy.rcgu.o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.oa3rad818d8sgn4.rcgu.o" "-o" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/examples/test_robot-90dfd36b579aa187.crate.allocator.rcgu.o" "-Wl,--gc-sections" "-pie" "-Wl,-z,relro,-z,now" "-nodefaultlibs" "-L" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/deps" "-L" "/home/aucg/frc/ctre-rs/target/debug/deps" "-L" "/home/aucg/frc/linux/athena/shared" "-L" "/home/aucg/frc/linux/athena/static" "-L" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib" "-Wl,-Bstatic" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/deps/libctre-d90c4df0c4bcf2b0.rlib" "/home/aucg/frc/ctre-rs/target/arm-unknown-linux-gnueabi/debug/deps/libctre_sys-f37aee2eb3ceb8eb.rlib" "-Wl,--start-group" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/libstd-b8cefe8bfc8e06fc.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/libpanic_unwind-c21a3f7e9b2ce71d.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/liballoc_jemalloc-a89ec74248f6f925.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/libunwind-9232e45013d81e94.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/liballoc_system-afbe0a28ca4ea99f.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/liblibc-bcf95ff3aa4e3276.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/liballoc-26716181a02171e3.rlib" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/libcore-978afee7e21437ec.rlib" "-Wl,--end-group" "/home/aucg/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/arm-unknown-linux-gnueabi/lib/libcompiler_builtins-3f4ad009a7cf735f.rlib" "-Wl,-Bdynamic" "-l" "FRC_NetworkCommunication" "-l" "NiFpga" "-l" "NiFpgaLv" "-l" "niriodevenum" "-l" "niriosession" "-l" "NiRioSrv" "-l" "RoboRIO_FRC_ChipObject" "-l" "visa" "-l" "stdc++" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "pthread" "-l" "gcc_s" "-l" "c" "-l" "m" "-l" "rt" "-l" "pthread" "-l" "util" "-l" "util"
  = note: /home/aucg/frc/linux/athena/shared/libNiFpgaLv.so: undefined reference to `RTSetCleanupProc'
          /home/aucg/frc/linux/athena/shared/libNiFpgaLv.so: undefined reference to `EDVR_CreateReference'
          /home/aucg/frc/linux/athena/shared/libNiFpgaLv.so: undefined reference to `NumericArrayResize'
          collect2: error: ld returned 1 exit status

(Update: I discovered that WPILib have had a hack for this dating back before 2015. Fun times.)

The intent then would be that the crate versions follow the Phoenix version numbers.

As for the plan for updates: I've modelled this off of how https://github.com/anguslees/rust-jsonnet is developed, in the hopes that CTRE don't drastically change the CCI layer. For the moment that seems pretty unlikely, other than changing the enum members — CTRE seem pretty happy with adding "versioned" APIs for the various functions.

Note that I was unable to use rust-bindgen for the Logger bindings (at least not without a large amount of pain), and the CANifier and PigeonIMU headers needed patching before I could bindgen those. I should file an issue about the latter…

There's probably a much better way of exposing the ErrorCode enum though, and I would appreciate any suggestions (that aren't too painful to implement).

auscompgeek added a commit that referenced this issue Jul 20, 2018
Avoid possible UB from the function possibly returning a value that is
not in our enum variants.

Ref: #1
@auscompgeek
Copy link
Member

Oh fun. Phoenix 5.7 has a thing to get all the configs. Thing is that it calls ConfigGetParameter, so it grabs doubles and casts them to the enums.

So now we need some way of converting the discriminants of config-related enums to their corresponding variants. And I definitely don't want to write it all by hand.

@auscompgeek
Copy link
Member

So I've proposed rust-lang/rust-bindgen#1669 with the intention of using it for ParamEnum at the very least, and maybe some of the other enums. Let's see where this goes.

@auscompgeek
Copy link
Member

I did a thing to preserve the nice Debug on ErrorCode whilst making it a newtype. So now the only potential UB should be the bool pointers (which I am going to assume will never be assigned invalid bit-patterns by the Phoenix libraries).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants