#90 Seems to have come back again

[ckaran]

Issue #90 seems to have come back again...

The issue is really, really weird though; when I build in debug mode, I don't have this bug, but when I rebuild (completely from scratch) in release mode, it comes back to bite me. Here's the output from the lldb. Compiler info, etc., are in the Meta section below.

$ lldb target/release/disc_model_simulator_driver -- ../scenarios.toml 
(lldb) target create "target/release/disc_model_simulator_driver"
Current executable set to 'target/release/disc_model_simulator_driver' (x86_64).
(lldb) settings set -- target.run-args  "../scenarios.toml"
(lldb) run
Process 32688 launched: '/home/cfkaran2/Documents/Dissertation/disc_model_simulator_driver/target/release/disc_model_simulator_driver' (x86_64)
ERROR: "/home/cfkaran2/Documents/Dissertation/disc_model_simulator/src/debug_memory_use.rs": 212: Debug_memory_logging_thread has started.
Note: Release mode will improve performance greatly.
    e.g. use `cargo run --release`
Process 32688 stopped
* thread #1, name = 'disc_model_simu', stop reason = signal SIGILL: illegal instruction operand
    frame #0: 0x0000555555947d5d disc_model_simulator_driver`x11_dl::xlib_xcb::Xlib_xcb::open::h23f529574679c79f at link.rs:0:7
   1    // x11-rs: Rust bindings for X11 libraries
   2    // The X11 libraries are available under the MIT license.
   3    // These bindings are public domain.
   4    
   5    use std::ffi::{ CStr, CString };
   6    use std::path::Path;
   7    use std::os::raw::{ c_char, c_void };
(lldb) bt
error: need to add support for DW_TAG_base_type 'char' encoded with DW_ATE = 0x8, bit_size = 32
* thread #1, name = 'disc_model_simu', stop reason = signal SIGILL: illegal instruction operand
  * frame #0: 0x0000555555947d5d disc_model_simulator_driver`x11_dl::xlib_xcb::Xlib_xcb::open::h23f529574679c79f at link.rs:0:7
    frame #1: 0x0000555555865a0a disc_model_simulator_driver`winit::platform::platform::x11::xdisplay::XConnection::new::h6b68c0a556c991c9(error_handler=Option<unsafe extern "C" fn(*mut x11_dl::xlib::_XDisplay, *mut x11_dl::xlib::XErrorEvent) -> i32> @ r14) at xdisplay.rs:39:23
    frame #2: 0x00005555558759f8 disc_model_simulator_driver`spin::once::Once$LT$T$GT$::call_once::hc944d05a52b546ee [inlined] _$LT$winit..platform..platform..X11_BACKEND$u20$as$u20$core..ops..deref..Deref$GT$::deref::__static_ref_initialize::h33afb142c7d0d9df at mod.rs:54:19
    frame #3: 0x00005555558759e6 disc_model_simulator_driver`spin::once::Once$LT$T$GT$::call_once::hc944d05a52b546ee [inlined] core::ops::function::FnOnce::call_once::h03d3fd96eef4dec8 at function.rs:231
    frame #4: 0x00005555558759e6 disc_model_simulator_driver`spin::once::Once$LT$T$GT$::call_once::hc944d05a52b546ee(self=0x0000555555c311a0, builder=<unavailable>) at once.rs:110
    frame #5: 0x00005555558839e0 disc_model_simulator_driver`winit::platform::platform::EventsLoop::new_x11::hdb92276b71d7487a [inlined] lazy_static::lazy::Lazy$LT$T$GT$::get::ha7a0c63009b0f450 at core_lazy.rs:21:8
    frame #6: 0x00005555558839d4 disc_model_simulator_driver`winit::platform::platform::EventsLoop::new_x11::hdb92276b71d7487a [inlined] _$LT$winit..platform..platform..X11_BACKEND$u20$as$u20$core..ops..deref..Deref$GT$::deref::__stability::h5f73dcc3ea994ef9 at <::lazy_static::__lazy_static_internal macros>:12
    frame #7: 0x00005555558839d4 disc_model_simulator_driver`winit::platform::platform::EventsLoop::new_x11::hdb92276b71d7487a [inlined] _$LT$winit..platform..platform..X11_BACKEND$u20$as$u20$core..ops..deref..Deref$GT$::deref::h7fc2a0f514c058e5(self=<unavailable>) at <::lazy_static::__lazy_static_internal macros>:13
    frame #8: 0x00005555558839d4 disc_model_simulator_driver`winit::platform::platform::EventsLoop::new_x11::hdb92276b71d7487a at mod.rs:459
    frame #9: 0x000055555588361f disc_model_simulator_driver`winit::platform::platform::EventsLoop::new::he6b4ce1a408563a5 at mod.rs:440:28
    frame #10: 0x00005555558a003a disc_model_simulator_driver`winit::EventsLoop::new::h6f539370fc8eaf4c at lib.rs:250:25
    frame #11: 0x000055555571b9f4 disc_model_simulator_driver`ggez::context::ContextBuilder::build::hddca8b3bb3196779 at context.rs:69:26
    frame #12: 0x000055555571b89e disc_model_simulator_driver`ggez::context::ContextBuilder::build::hddca8b3bb3196779(self=ContextBuilder @ 0x00007fffffffc500) at context.rs:273
    frame #13: 0x000055555564df6d disc_model_simulator_driver`disc_model_simulator_driver::main::_$u7b$$u7b$closure$u7d$$u7d$::h3d4016fcf37dcd4f((null)=<unavailable>, scenario=Scenario @ 0x00007fffffffd8b0) at main.rs:291:37
    frame #14: 0x000055555564d442 disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 [inlined] core::iter::traits::iterator::Iterator::for_each::_$u7b$$u7b$closure$u7d$$u7d$::h153c46f0df67cdea at iterator.rs:604:38
    frame #15: 0x000055555564d3ca disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 [inlined] core::iter::traits::iterator::Iterator::fold::_$u7b$$u7b$closure$u7d$$u7d$::h739f9c6e8319c5f4 at iterator.rs:1685
    frame #16: 0x000055555564d3ca disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 [inlined] core::iter::traits::iterator::Iterator::try_fold::h05f7e3384a01aa3c(self=<unavailable>) at iterator.rs:1573
    frame #17: 0x000055555564d356 disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 [inlined] core::iter::traits::iterator::Iterator::fold::hd177379ce9516e2d(self=<unavailable>) at iterator.rs:1685
    frame #18: 0x000055555564d356 disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 [inlined] core::iter::traits::iterator::Iterator::for_each::h10954f1698a2ed5f(self=IntoIter<disc_model_simulator_driver::scenario_parser::Scenario> @ 0x00000000027282b0) at iterator.rs:604
    frame #19: 0x000055555564d32c disc_model_simulator_driver`disc_model_simulator_driver::main::h339ab0a862cd6123 at main.rs:273
    frame #20: 0x00005555555dbe72 disc_model_simulator_driver`std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h4f4551107d3dd7b2 at rt.rs:64:33
    frame #21: 0x0000555555a0b9b3 disc_model_simulator_driver`std::panicking::try::do_call::hcfcd808f74618614 [inlined] std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::ha6cdc0674c227baf at rt.rs:49:12
    frame #22: 0x0000555555a0b9a7 disc_model_simulator_driver`std::panicking::try::do_call::hcfcd808f74618614 at panicking.rs:296
    frame #23: 0x0000555555a105da disc_model_simulator_driver`__rust_maybe_catch_panic at lib.rs:82:7
    frame #24: 0x0000555555a0c4bd disc_model_simulator_driver`std::rt::lang_start_internal::ha819bd148e81f2bd [inlined] std::panicking::try::h28299215a9cbdc2e at panicking.rs:275:12
    frame #25: 0x0000555555a0c47f disc_model_simulator_driver`std::rt::lang_start_internal::ha819bd148e81f2bd [inlined] std::panic::catch_unwind::h6d2adbec69dbd2ec at panic.rs:394
    frame #26: 0x0000555555a0c47f disc_model_simulator_driver`std::rt::lang_start_internal::ha819bd148e81f2bd at rt.rs:48
    frame #27: 0x0000555555651b08 disc_model_simulator_driver`main + 40
    frame #28: 0x00007ffff7c82b6b libc.so.6`__libc_start_main + 235
    frame #29: 0x00005555555a023a disc_model_simulator_driver`_start + 42

Meta

Linux Dissertation 5.0.0-20-generic #21-Ubuntu SMP Mon Jun 24 09:32:09 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Distributor ID: Ubuntu
Description:    Ubuntu 19.04
Release:    19.04
Codename:   disco

rustc 1.36.0 (a53f9df32 2019-07-03)
binary: rustc
commit-hash: a53f9df32fbb0b5f4382caaad8f1a46f36ea887c
commit-date: 2019-07-03
host: x86_64-unknown-linux-gnu
release: 1.36.0
LLVM version: 8.0

rustc 1.37.0-beta.2 (74e5a0d47 2019-07-08)
binary: rustc
commit-hash: 74e5a0d47ef4614524675f917e42b3fc45e8f759
commit-date: 2019-07-08
host: x86_64-unknown-linux-gnu
release: 1.37.0-beta.2
LLVM version: 8.0

rustc 1.38.0-nightly (0b680cfce 2019-07-09)
binary: rustc
commit-hash: 0b680cfce544ff9a59d720020e397c4bf3346650
commit-date: 2019-07-09
host: x86_64-unknown-linux-gnu
release: 1.38.0-nightly
LLVM version: 8.0

cargo 1.36.0 (c4fcfb725 2019-05-15)
release: 1.36.0
commit-hash: c4fcfb725b4be00c72eb9cf30c7d8b095577c280
commit-date: 2019-05-15

cargo 1.37.0-beta (4c1fa54d1 2019-06-24)
release: 1.37.0
commit-hash: 4c1fa54d10f58d69ac9ff55be68e1b1c25ecb816
commit-date: 2019-06-24

cargo 1.37.0-nightly (4c1fa54d1 2019-06-24)
release: 1.37.0
commit-hash: 4c1fa54d10f58d69ac9ff55be68e1b1c25ecb816
commit-date: 2019-06-24

[o01eg]

I get same issue on nightly:

Thread 1 "veloren-voxygen" received signal SIGILL, Illegal instruction.
0x00005555559e9f05 in x11_dl::xlib_xcb::Xlib_xcb::open () at /home/o01eg/.cargo/registry/src/github.com-1ecc6299db9ec823/x11-dl-2.18.3/src/link.rs:65
65        }
(gdb) disassemble 
Dump of assembler code for function x11_dl::xlib_xcb::Xlib_xcb::open:
   0x00005555559e9eb0 <+0>: push   %rbx
   0x00005555559e9eb1 <+1>: sub    $0x30,%rsp
   0x00005555559e9eb5 <+5>: mov    %rdi,%rbx
   0x00005555559e9eb8 <+8>: lea    0x307429(%rip),%rsi        # 0x555555cf12e8
   0x00005555559e9ebf <+15>:    lea    0x4d5c72(%rip),%rcx        # 0x555555ebfb38
   0x00005555559e9ec6 <+22>:    lea    0x8(%rsp),%rdi
   0x00005555559e9ecb <+27>:    mov    $0xa,%edx
   0x00005555559e9ed0 <+32>:    mov    $0x2,%r8d
   0x00005555559e9ed6 <+38>:    callq  0x5555559e89a0 <x11_dl::link::DynamicLibrary::open_multi>
   0x00005555559e9edb <+43>:    cmpq   $0x1,0x8(%rsp)
   0x00005555559e9ee1 <+49>:    jne    0x5555559e9f05 <x11_dl::xlib_xcb::Xlib_xcb::open+85>
   0x00005555559e9ee3 <+51>:    movups 0x10(%rsp),%xmm0
   0x00005555559e9ee8 <+56>:    movups 0x20(%rsp),%xmm1
   0x00005555559e9eed <+61>:    movups %xmm1,0x18(%rbx)
   0x00005555559e9ef1 <+65>:    movups %xmm0,0x8(%rbx)
   0x00005555559e9ef5 <+69>:    movq   $0x1,(%rbx)
   0x00005555559e9efc <+76>:    mov    %rbx,%rax
   0x00005555559e9eff <+79>:    add    $0x30,%rsp
   0x00005555559e9f03 <+83>:    pop    %rbx
   0x00005555559e9f04 <+84>:    retq   
=> 0x00005555559e9f05 <+85>:    ud2

[o01eg]

Minimal example:

extern crate x11_dl;

use x11_dl::xlib_xcb;

fn main () {
    unsafe {
        let xlib_xcb = xlib_xcb::Xlib_xcb::open().unwrap();
    }
}

[o01eg]

It was broken between https://github.com/rust-lang/rust/commit/24a9bcbb7cb0d8bdc11b8252a9c13f7562c7e4ca (nightly-2019-07-05) and https://github.com/rust-lang/rust/commit/481068a707679257e2a738b40987246e0420e787 (nightly-2019-07-06)

[o01eg]

Seemed to be caused by https://github.com/rust-lang/rust/pull/62150, and doesn't affect other than xlib_xcb libraries.

[o01eg]

I suppose minimum supported rust version is from Debian Jessie. What if left 1.8.x with Debian Jessie minimum, and set new minimum supported rust version with MaybeUninit in 1.9.x?

[RalfJung]

Do you know which code on your end is creating the zeroed reference?

A quick search brought up

https://github.com/erlepereira/x11-rs/blob/40f3cc218e53e89f4b5093b16348de3b7103a8bd/src/internal.rs#L30

which looks... very bad.^^ There's not even a comment explaining what this is doing, or under which conditions it is safe to call---something that should be done for every unsafe fn.

How is it different from transmute_copy?

[bschwind]

@RalfJung I'm still digging into this, but from the minimal example from @o01eg , it seems to be crashing at this line in release mode:

https://github.com/erlepereira/x11-rs/blob/f5134f97271dcfe901c544e4f4dfa9229a194e1d/x11-dl/src/link.rs#L58-L59

[RalfJung]

Ah yes, that's incorrect. ManuallyDrop has no effect on the rule that data must be "valid", such as references not being NULL. You have to use MaybeUninit for that.

[bschwind]

I'm new to MaybeUninit, just heard about it when it was stabilized. Can you initialize parts of the struct at a time or does it have to all be in one go with as_mut_ptr().write() ?

My other thought to work through this would be to generate a struct with all fields as Options, write to the fields as you go, then convert to a struct with the same fields which aren't contained in Options.

[RalfJung]

Can you initialize parts of the struct at a time or does it have to all be in one go with as_mut_ptr().write() ?

Unfortunately that's not possible yet in Rust. :/ This requires a solution for https://github.com/rust-lang/rfcs/pull/2582. But you can get close, by getting a raw pointer to a field: &mut (*uninit.as_mut_ptr()).field as *mut _.

[bschwind]

Ahhh okay, that's enough to get me started. I'll take a crack at it tomorrow when I have access to a Linux machine, thanks!

[ghost]

Do you know which code on your end is creating the zeroed reference?

A quick search brought up

https://github.com/erlepereira/x11-rs/blob/40f3cc218e53e89f4b5093b16348de3b7103a8bd/src/internal.rs#L30

which looks... very bad.^^ There's not even a comment explaining what this is doing, or under which conditions it is safe to call---something that should be done for every unsafe fn.

How is it different from transmute_copy?

It's been a long time since I wrote that code, so I don't remember what my reasoning was at the time, but it may be that I only know of transmute and not transmute_copy but needed a function that can operate on types of different sized. I'd be afraid to meddle with some of this code myself as I've been away from this project for 2 years, but I believe it would be safe to remove that ugly function and use transmute_copy instead.

As for this bug coming up again, using mem::uninitialized instead of mem::zeroed once fixed it, but some of the Rust devs seem to be doubling down on breaking backwards compatibility and closing issues when it's brought up. I'm not really sure what can be done without breaking library compatibility. If, from the start, the macro had generated an inner struct made up of only function pointers and without a destructor, this bug probably never would have happened, but the library would have been even more awkward to use.

[bschwind]

Interesting further discussion here: https://github.com/rust-lang/rust/issues/52898#issuecomment-513052832

[RalfJung]

I believe it would be safe to remove that ugly function and use transmute_copy instead.

It would be great if someone who's uo-to-date on the project could look into this. transmute_copy is a very sharp knife only to be used with great care, but at least it is a fairly widely known knife, which makes it much better than an entirely undocumented unsafe function with a mysterious name. (What does the "union" part of it mean?)

some of the Rust devs seem to be doubling down on breaking backwards compatibility and closing issues when it's brought up

I think that is a very unfair characterization of what is going on. But this is being discussed in the other thread mentioned by @bschwind.

[RalfJung]

Oh and for completeness' sake, the likely cause of this SIGILL is https://github.com/rust-lang/rust/pull/62150.

[RalfJung]

Btw, my best guess for where the bad function pointers are declared is

https://github.com/erlepereira/x11-rs/blob/7fe454564799c22e7bc1069758bd99f372aaf095/x11-dl/src/link.rs#L30-L31

If you want to zero/"uninit"-initialize that struct, these should definitely be Option<fn>. A fn may never be 0.

[mickvangelderen]

@Daggerbot How are the compile times if you have two versions of the struct, one with Option/usize and one with the actual fn pointers. You use the first version to set all fields to 0 and initialize the values in a loop like happens now. Then, before passing it back to the user and after having asserted all fields != 0, you transmute the whole struct to the second version.

I realize there is a good chance this isn't helping because you now have two versions of this giant struct. Perhaps you can work on an array of usizes directly instead and transmute that.

[est31]

The revert PR has just been merged in Rust upstream so fixing this issue isn't as urgent any more: https://github.com/rust-lang/rust/pull/63343 . They will do the change sooner or later though so it should not be ignored.

[RalfJung]

fixing this issue isn't as urgent any more

No, it's just as urgent. We only reverted this in Rust so that users of this crate don't have to suffer, and we want to un-do the revert as soon as we can. Basically, we granted you a reprieve. We would much appreciate if y'all could help us in this (help us keeping the compiler clean and not accumulate cruft) by fixing this bug in this crate. :)

We will also land a lint on nightly soon (hopefully in time for the beta) that will help find code that uses mem::zeroed/mem::uninitialized the wrong way. It won't find all the bugs, but I think it should be able to find the bug in the code here.

[est31]

@RalfJung it's not urgent for me because my project uses Rust nightly as well as x11-rs. As for helping this crate, I can't because the problem mostly seems to be blocked on maintainers. There is an open PR to fix it but they aren't merging it. And yeah I saw the lint PR as well, it's good to have it (would be good to warn on bad assume_init use too, right now the example right at the top of MaybeUninit docs is linting only for mem::uninitialized, not for MaybeUninit).

[RalfJung]

We can't do the same warning on general assume_init, so we'd have to detect the specific case of zeroed().assume_init(). Sure, would be nice to do, but I think that's way less common. I mean that basically can only happen if people write unsafe code without reading any docs.

[RalfJung]

Indeed the lint is firing:

warning: the type `std::mem::ManuallyDrop<xrandr::Xrandr>` does not permit being left uninitialized
  --> x11-dl/src/link.rs:59:17
   |
59 |                 = ::std::mem::uninitialized();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | 
  ::: x11-dl/src/xrandr.rs:16:1
   |
16 | / x11_link! { Xrandr, xrandr, ["libXrandr.so.2", "libXrandr.so"], 70,
17 | |     pub fn XRRAddOutputMode (dpy: *mut Display, output: RROutput, mode: RRMode) -> (),
18 | |     pub fn XRRAllocGamma (size: c_int) -> *mut XRRCrtcGamma,
19 | |     pub fn XRRAllocModeInfo (name: *const c_char, nameLength: c_int) -> *mut XRRModeInfo,
...  |
88 | | globals:
89 | | }
   | |_- in this macro invocation
   |
   = note: this means that this code causes undefined behavior when executed
   = help: use `MaybeUninit` instead

[RalfJung]

With an ongoing PR, this now even points at the field causing the issue:

warning: the type `std::mem::ManuallyDrop<xrandr::Xrandr>` does not permit being left uninitialized
  --> x11-dl/src/link.rs:59:17
   |
59 |                 = ::std::mem::uninitialized();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | 
  ::: x11-dl/src/xrandr.rs:16:1
   |
16 | / x11_link! { Xrandr, xrandr, ["libXrandr.so.2", "libXrandr.so"], 70,
17 | |     pub fn XRRAddOutputMode (dpy: *mut Display, output: RROutput, mode: RRMode) -> (),
18 | |     pub fn XRRAllocGamma (size: c_int) -> *mut XRRCrtcGamma,
19 | |     pub fn XRRAllocModeInfo (name: *const c_char, nameLength: c_int) -> *mut XRRModeInfo,
...  |
88 | | globals:
89 | | }
   | |_- in this macro invocation
   |
note: Function pointers must be non-null (in this struct field)
  --> x11-dl/src/link.rs:30:9
   |
30 |         $(pub $fn_name: unsafe extern "C" fn ($($param_type),*) -> $ret_type,)*
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | 
  ::: x11-dl/src/xrandr.rs:16:1
   |
16 | / x11_link! { Xrandr, xrandr, ["libXrandr.so.2", "libXrandr.so"], 70,
17 | |     pub fn XRRAddOutputMode (dpy: *mut Display, output: RROutput, mode: RRMode) -> (),
18 | |     pub fn XRRAllocGamma (size: c_int) -> *mut XRRCrtcGamma,
19 | |     pub fn XRRAllocModeInfo (name: *const c_char, nameLength: c_int) -> *mut XRRModeInfo,
...  |
88 | | globals:
89 | | }
   | |_- in this macro invocation
   = note: this means that this code causes undefined behavior when executed
   = help: use `MaybeUninit` instead