Stream: git-wasmtime

Topic: wasmtime / Issue #1240 cranelift-codegen fails to compile...

Wasmtime GitHub notifications bot (Mar 05 2020 at 22:14):

iximeow opened Issue #1240:

• What are the steps to reproduce the issue? Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using? 3179dcf (master)
• If relevant, can you include some extra information about your environment? n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Wasmtime GitHub notifications bot (Mar 05 2020 at 22:14):

iximeow labeled Issue #1240:

• What are the steps to reproduce the issue? Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using? 3179dcf (master)
• If relevant, can you include some extra information about your environment? n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Wasmtime GitHub notifications bot (Mar 05 2020 at 22:14):

iximeow labeled Issue #1240:

• What are the steps to reproduce the issue? Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using? 3179dcf (master)
• If relevant, can you include some extra information about your environment? n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Wasmtime GitHub notifications bot (Mar 05 2020 at 22:15):

iximeow edited Issue #1240:

• What are the steps to reproduce the issue?
  • Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using?
  • 3179dcf (master)
• If relevant, can you include some extra information about your environment?
  • n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Wasmtime GitHub notifications bot (Mar 05 2020 at 22:33):

iximeow edited Issue #1240:

• What are the steps to reproduce the issue?
  • Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using?
  • 3179dcf (master)
• If relevant, can you include some extra information about your environment?
  • n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes? Edit: this depends on other questions like, can TlsGetAddr be hoisted in LICM? Eligible for DCE? And interaction with the verifier? Given that it includes a call I think this might be the right way to go about it. And since the call takes an argument in rdi, if there needs to be a signature for verifier reasons, it looks like we can synthesize one of the form (rdi) -> rax?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Wasmtime GitHub notifications bot (Mar 06 2020 at 11:44):

bnjbvr commented on Issue #1240:

This is now biting us in Spidermonkey too, where we only build the relevant backends, so this is high priority for us.

We could add supplementary builds with disabled features in automation, to make sure they don't get broken anymore.

Wasmtime GitHub notifications bot (Mar 06 2020 at 11:45):

bjorn3 commented on Issue #1240:

I initially tried adding is_call, but that requires analyze_call to return a valid CallInfo. As the instructions in question are not normal calls and don't have a Signature, it is not possible to return a valid CallInfo.

can TlsGetAddr be hoisted in LICM? Eligible for DCE?

Yes and yes, I think.

And since the call takes an argument in rdi, if there needs to be a signature for verifier reasons, it looks like we can synthesize one of the form (rdi) -> rax?

How would that signature get assigned to the instruction? It is not possible to lower the instructions to libcalls, as they require a very specific combination of x86 instructions for the linker to handle it correctly.

was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue.

During compilation the build script detects the current architecture and automatically enables support for it. (at least if there are no archs enabled through feature flags)

Wasmtime GitHub notifications bot (Mar 06 2020 at 12:00):

bnjbvr commented on Issue #1240:

So my current thinking is the following: since this can't be emulated properly with a call, maybe the proper way here is to add a new accessor on the opcode? I've got a wip patch that does this, with a new property called "clobbers_all_regs", and which is tested during register allocation in place of the opcode comparisons. I thought about other names, like "emulates_call", but this seems to be the most explicit, at the very least.

Wasmtime GitHub notifications bot (Mar 06 2020 at 12:04):

bjorn3 commented on Issue #1240:

I've got a wip patch that does this, with a new property called "clobbers_all_regs"

:+1:

Wasmtime GitHub notifications bot (Mar 06 2020 at 12:05):

bjorn3 commented on Issue #1240:

Was actually working on the same, but named it clobbers_all :)

Wasmtime GitHub notifications bot (Mar 23 2020 at 11:19):

bnjbvr closed Issue #1240:

• What are the steps to reproduce the issue?
  • Build cranelift-codegen targeting only one architecture (as an example, clone https://github.com/iximeow/cautious-pancake and build it)
• What do you expect to happen?
  • Compilation should succeed.
• What does actually happen?
  • Compilation does not succeed.
• Which Cranelift version / commit hash / branch are you using?
  • 3179dcf (master)
• If relevant, can you include some extra information about your environment?
  • n/a - reproducible anywhere Cargo runs.

This was first mentioned by @stefson in this comment.

I agree with @bjorn3's assessment in reply. Since TLS access assumes a C ABI, would it make sense to just add is_call as an attribute to these opcodes? Edit: this depends on other questions like, can TlsGetAddr be hoisted in LICM? Eligible for DCE? And interaction with the verifier? Given that it includes a call I think this might be the right way to go about it. And since the call takes an argument in rdi, if there needs to be a signature for verifier reasons, it looks like we can synthesize one of the form (rdi) -> rax?

A wider concern: does anyone have ideas on how we'd build Cranelift under these different, more specific, feature flag combinations? I was surprised to learn cargo build --no-default-features --features std didn't work to show this, but that seems to be a consequence of this issue. It seems appropriate to have some test crates that build under feature flag combinations we want to support, so issues like this get caught in CI. It would raise build times a bit, but I'm not sure how much. Does that sound acceptable?

Last updated: Nov 22 2024 at 17:03 UTC