Switching to opt-level=z on i686-windows-msvc triggers STATUS_ACCESS_VIOLATION

[dignifiedquire]

I have a crate which compiles and tests fine, but as soon as I switch opt-level=z it triggers STATUS_ACCESS_VIOLATION in some tests, but only on the windows targets. Tested on

• nightly-2019-11-06
• nightly-2019-12-20

The corresponding log outputs can be found here https://ci.appveyor.com/project/dignifiedquire/deltachat-core-rust/builds/29699168
and the code deltachat/deltachat-core-rust#1087

I am sorry I don't have a more reduced testcase, but I am quite clueless on how to debug this tbh.

[dignifiedquire]

Tested on windows x86_64, no issues present

[Brooooooklyn]

napi-rs/napi-rs#297 same issue here

[hameerabbasi]

As this is:

• Unsound
• A tier 1 platform
• Occurs on working code

we're assigning P-critical. See the wg-prioritization discussion.

[jyn514]

Is this a regression in 1.48? Could you test on 1.47 and see if the same issue happens there?

[Brooooooklyn]

Tested 1.36.0 ~ 1.48.0, all these versions occurred this bug.

[Brooooooklyn]

It can reproduce in https://github.com/napi-rs/package-template

0. choco install node-lts -x86 -y -f
1. refreshenv
2. checkout napi-1-pre branch
3. yarn install
4. yarn build
5. node simple-test.js

The reproduce step could be found here: https://github.com/napi-rs/package-template/blob/napi-1-pre/.github/workflows/CI.yaml#L189

You can also see Github actions output from napi-rs/package-template#86

[Brooooooklyn]

And I found the error was from this function: https://github.com/napi-rs/napi-rs/blob/master/napi/src/js_values/mod.rs#L270

I add println into this function:

#[repr(C)]
#[derive(Copy, Clone)]
pub struct napi_env__ {
  _unused: [u8; 0],
}

/// Env ptr
pub type napi_env = *mut napi_env__;
#[repr(C)]
#[derive(Copy, Clone)]
pub struct napi_value__ {
  _unused: [u8; 0],
}

/// JsValue ptr
pub type napi_value = *mut napi_value__;


#[derive(Clone, Copy)]
pub struct Value {
  pub env: napi_env,
  pub value: napi_value,
  pub value_type: ValueType,
}

pub struct JsObject(pub(crate) Value);

impl JsObject {
  #[inline]
  pub fn create_named_method(&mut self, name: &str, function: Callback) -> Result<()> {
    let mut js_function = ptr::null_mut();
    let len = name.len();
    let name = CString::new(name.as_bytes())?;
+   println!("{:p}", self.0.env);
    check_status!(unsafe {
      sys::napi_create_function(
        self.0.env,
        name.as_ptr(),
        len,
        Some(function),
        ptr::null_mut(),
        &mut js_function,
      )
    })?;
+   println!("{:p}", self.0.env);
    check_status!(unsafe {
      sys::napi_set_named_property(self.0.env, self.0.value, name.as_ptr(), js_function)
    })
  }
}

The result was:

0x39b5178
0x6

[Brooooooklyn]

Is this line related to this issue? https://github.com/rust-lang/rust/blob/1.48.0/compiler/rustc_target/src/abi/call/mod.rs#L436

[Brooooooklyn]

Fixed by set codegen-units to 32 (or bigger), and disable lto: https://github.com/napi-rs/package-template/blob/main/.github/workflows/CI.yml#L37

[Brooooooklyn]

Still appears in my projects, change codegen-units and disable lto doesn't work anymore

[Brooooooklyn]

#67497 (comment)

@pnkfelix does this one enough to be mcve?

[pnkfelix]

Visiting for P-high review

@wesleywiser does this still occur?

And, to answer the question above: It is rare for links to external repositories to qualify as truly minimal MCVEs. (And also, I've found in practice that links to external repositories run the risk of being deleted, so we really need some code in a spot we have ownership over before I'd be comfortable removing the needs-mcve label.)

[Amanieu]

While investigating a completely unrelated issue on x86 (32-bit) QNX with @hexagonal-sun, we also got a crash with opt-level=z. Forcing the stack alignment to 16 bytes in main with inline assembly seems to fix the issue and avoid the crash.

I think it might be related to this issue because on 32-bit x86 both QNX and Windows have a base stack alignment of 4 bytes instead of 16 on Linux. Note that the stack realignment shouldn't be necessary: since we pass -S32 in the LLVM datalayout the compiler should be able to properly handle non-16-byte aligned stacks.

We haven't fully narrowed down the cause yet, so this may still be a false lead, but it might be worth trying in this case.

[Amanieu]

Here is a minimal example which reproduces the problem on Linux: https://github.com/hexagonal-sun/rust-alignment-error

It uses a modified i585-unknown-linux-gnu target with the stack alignment set to 4 bytes instead of 16. Inline assembly in main is used to deliberately misalign the stack to reproduce the bug. This should be fine in the LLVM datalayout in the target uses S32 to indicate the the stack point should be assumed to be 4-byte aligned. The crash doesn't happen if the stack is 8-byte aligned.

[Amanieu]

I've narrowed this down to what I think is an LLVM bug in the X86 backend. I've uploaded the reduced LLVM IR and assembly.

For some reason LLVM is using an or instruction to calculate a stack address:

# %bb.5:                                # %bb3
    [...]
	pushl	$4
	popl	%esi                <-- esi = 4
    [...]
	leal	72(%esp), %eax      <-- eax = esp + 72
	orl	%eax, %esi              <-- eax = (esp + 72) | 4

If the stack is 8-byte aligned then this adds 4 to the pointer address, and everything works fine. However if the stack is offset by 4 bytes then this or is a no-op, which eventually leads to a crash.

There is no reason for LLVM to generate an or here, the corresponding LLVM IR only contains getelementptr operations which should be lowered to add/sub only.

[nikic]

@Amanieu LLVM converts add to or if there are no common bits. That just means that something claims the low bits are known zero.

I don't think the S32 in the DataLayout affects codegen at all though -- afaik it it only relevant for alignment promotion (to prevent promoting above natural alignment).

The stack alignment is decided by the subtarget and will always be 16 on Linux. It's possible to override it using the override-stack-alignment module metadata.

As such, I'm not sure your reduction here is meaningful (on Windows, codegen should be choosing stack alignment 4 for 32-bit x86).

[nikic]

Adding

!llvm.module.flags = !{!0, !1, !2, !32}
...
!32 = !{i32 1, !"override-stack-alignment", i32 4}

to your reduction does convert the orl into addl.

[Amanieu]

Good point, I guess that was a false lead.