wasmtime / Issue #2174 Don't substract 1 from end_addr in... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #2174 Don't substract 1 from end_addr in...

Wasmtime GitHub notifications bot (Aug 29 2020 at 16:37):

Thanks for the quick fix! Let me come up with a regression test for it!

Wasmtime GitHub notifications bot (Aug 29 2020 at 16:50):

Thanks for the quick fix! Let me come up with a regression test for it! (Will take a few hours, but there is no haste...)

Wasmtime GitHub notifications bot (Aug 29 2020 at 22:08):

ggreif commented on Issue #2174:

https://github.com/bjorn3/wasmtime/pull/1

Wasmtime GitHub notifications bot (Aug 29 2020 at 22:08):

ggreif edited a comment on Issue #2174:

PR: https://github.com/bjorn3/wasmtime/pull/1

Wasmtime GitHub notifications bot (Aug 30 2020 at 12:07):

ggreif edited a comment on Issue #2174:

PR: https://github.com/bjorn3/wasmtime/pull/1

Note: the test added is _ignored_, due to the usual. lack of tools on CI for drumming it. Try
cargo test translate -- --ignored
After reverting 053757f, the above fails.

Wasmtime GitHub notifications bot (Aug 30 2020 at 12:07):

ggreif edited a comment on Issue #2174:

PR: https://github.com/bjorn3/wasmtime/pull/1

Note: the test added is _ignored_, due to the usual lack of tools on CI for drumming it. Try
cargo test translate -- --ignored
After reverting 053757f, the above fails.

Wasmtime GitHub notifications bot (Aug 30 2020 at 12:08):

ggreif edited a comment on Issue #2174:

PR: https://github.com/bjorn3/wasmtime/pull/1

Note: the test added is _ignored_, due to the usual lack of tools on CI for running it. Try
cargo test translate -- --ignored
After reverting 053757f, the above fails.

Wasmtime GitHub notifications bot (Aug 31 2020 at 13:35):

ggreif edited a comment on Issue #2174:

~~Thanks for the quick fix! Let me come up with a regression test for it! (Will take a few hours, but there is no haste...)~~ DONE!

Wasmtime GitHub notifications bot (Aug 31 2020 at 14:33):

ggreif commented on Issue #2174:

I'm skeptical about test having hardcoded address. We can keep it though for now with note just it relies on cranelift generator, so the address will be justed when generation changes.

Yeah, right, I was thinking of this too. But it shouldn't be very bad, since

short code is more resistant of CG changes

this part of CD should be mature and rather slowly-moving by now (fingers crossed)

the intent is clear, so easy to fix if a CG change would touch this.

Also the FileCheck Rust library seems to be missing advanced features like captures and evaluation. I don't even know whether the LLVM tool proper provides those.

So I suggest to leave it like this for now, and if it causes headaches, just drop it. Better ideas?

Wasmtime GitHub notifications bot (Aug 31 2020 at 14:41):

ggreif edited a comment on Issue #2174:

I'm skeptical about test having hardcoded address. We can keep it though for now with note just it relies on cranelift generator, so the address will be justed when generation changes.

Yeah, right, I was thinking of this too. But it shouldn't be very bad, since

short code is more resistant of CG changes

this part of CG should be mature and rather slowly-moving by now (fingers crossed)

the intent is clear, so easy to fix if a CG change would touch this.

Also the FileCheck Rust library seems to be missing advanced features like captures and evaluation. I don't even know whether the LLVM tool proper provides those.

So I suggest to leave it like this for now, and if it causes headaches, just drop it. Better ideas?

Wasmtime GitHub notifications bot (Aug 31 2020 at 14:44):

bjorn3 commented on Issue #2174:

Filecheck does have captures: https://github.com/bytecodealliance/wasmtime/blob/a7f7c23bf9c37c642da962d575b7c99007918872/cranelift/filetests/filetests/regalloc/coalesce.clif#L30

Wasmtime GitHub notifications bot (Aug 31 2020 at 14:51):

ggreif commented on Issue #2174:

Filecheck does have captures

That's cool, but I cannot see the ability to do $(expr <var> + 1) or such.

Wasmtime GitHub notifications bot (Sep 01 2020 at 08:46):

ggreif edited a comment on Issue #2174:

I'm skeptical about test having hardcoded address. We can keep it though for now with note just it relies on cranelift generator, so the address will be justed when generation changes.

Yeah, right, I was thinking of this too. But it shouldn't be very bad, since

short code is more resistant of CG changes

this part of CG should be mature and rather slowly-moving by now (fingers crossed)

the test is _ignored_, so it won't run in CI

the intent is clear, so easy to fix if a CG change would touch this.

Also the FileCheck Rust library seems to be missing advanced features like captures and evaluation. I don't even know whether the LLVM tool proper provides those.

So I suggest to leave it like this for now, and if it causes headaches, just drop it. Better ideas?

Wasmtime GitHub notifications bot (Sep 05 2020 at 15:15):

ggreif commented on Issue #2174:

Let's try that. In the past it was always an issue, e.g. I had the following in the dwarf-to-json library:

https://github.com/yurydelendik/dwarf-to-json/blob/d9c6f665f67da3f862a36d88b86eb488ca4bbc4d/src/dwarf.rs#L488-L500

Can you check if we have similar case here?

I can't see how that could be. The DWARF5 standard is pretty much unambiguous in its wording (p151):

end_sequence:
A boolean indicating that the current address is that of the first byte after the end of a sequence of target machine instructions. end_sequence terminates a sequence of lines; therefore other information in the same row is not meaningful.

Wasmtime GitHub notifications bot (Sep 07 2020 at 13:41):

yurydelendik commented on Issue #2174:

Thank you for the patch

Last updated: Nov 22 2024 at 16:03 UTC