Wasmtime GitHub notifications bot (Dec 19 2024 at 16:27): Wasmtime GitHub notifications bot (Dec 19 2024 at 16:27):SingleAccretion opened PR #9866 from SingleAccretion:DI-Fix to bytecodealliance:main:
Work in progress.
Wasmtime GitHub notifications bot (Dec 19 2024 at 16:55):SingleAccretion updated PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:06):SingleAccretion updated PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:07):SingleAccretion edited PR #9866:
Fixes #9857.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:09):SingleAccretion updated PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:10):SingleAccretion edited PR #9866:
Fixes #9857.
Switches the CI testing for DWARF support to use
--release, to hopefully increase the likelihood of this kind of issue being caught early next time.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:11):SingleAccretion edited PR #9866:
Fixes #9857.
Switches the CI testing for DI to use
--release, to hopefully increase the likelihood of this kind of issue being caught early next time.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:12):SingleAccretion edited PR #9866:
Fixes #9857.
Switches the CI testing for DI to use
--release, to hopefully increase the likelihood of this kind of issue being caught early next time.Another policy which could've caught this early is enabling warn-as-error for C code. But that would be a larger change.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:12):SingleAccretion edited PR #9866:
Fixes #9857.
Switches the CI testing for DI to use
--release, to hopefully increase the likelihood of this kind of issue being caught early next time.Another policy which would've caught this early is enabling warn-as-error for C code. But that would be a larger change.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:19):SingleAccretion has marked PR #9866 as ready for review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:19):SingleAccretion requested alexcrichton for a review on PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:19):SingleAccretion requested wasmtime-core-reviewers for a review on PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:19):SingleAccretion requested wasmtime-default-reviewers for a review on PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 17:19):SingleAccretion edited PR #9866:
Fixes #9857.
Switches the CI testing for DI to use
--release, to increase the likelihood of this kind of issue being caught early next time.Another policy which would've caught this early is enabling warn-as-error for C code. But that would be a larger change.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:03):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:03):alexcrichton created PR review comment:
Question on this, what's the toolchain needed for this? I'm assuming the comment here about the Rust toolchain might be mistaken because Rust isn't involved in compiling this file, but do you know the minimum gcc/clang/etc version needed to process this?
We could look into CI and see which toolchain is lagging behind perhaps (I think x64-linux is one of the oldest due to glibc compat so probably nothing to do about that in the end alas)
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:03):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:03):alexcrichton created PR review comment:
If these tests are fast enough mind adding both
--release-and-not?
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:51):SingleAccretion updated PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:52):SingleAccretion submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:52):SingleAccretion created PR review comment:
I'm assuming the comment here about the Rust toolchain might be mistaken because Rust isn't involved in compiling this file
(I was secretly hoping this uses some bundled clang, but if it's using the host/found in PATH toolchain...)
Question on this, what's the toolchain needed for this? , but do you know the minimum gcc/clang/etc version needed to process this?
It is a relatively recent feature.
__attribute__((retain)) (GCC 11 with modern binutils, Clang 13).
I think the best resolution here is to actually simply delete the comment with
retain.volatileis a more robust than trying to ensure everything has the right toolchain.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:52):SingleAccretion submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:52):SingleAccretion created PR review comment:
Added. Numbers follow.
The tests themselves are fairly quick (because there are so few of them :)), what takes its time is building everything.
This
--releaseversion now takes ~8min, and the previous version took ~5min. Combining these, we conservatively get ~13min, which is on the order of the longest job in this PR (~12min).
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:52):SingleAccretion edited PR review comment.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:53):SingleAccretion edited PR review comment.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:53):SingleAccretion edited PR review comment.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:55):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:55):alexcrichton created PR review comment:
Ah ok good point, in that case let's go ahead and drop the debug build then and only test
--release. Mind adding a comment as to why we're only testing--releaseas well?
Wasmtime GitHub notifications bot (Dec 19 2024 at 18:59):SingleAccretion updated PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 19:03):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 19 2024 at 19:03):alexcrichton has enabled auto merge for PR #9866.
Wasmtime GitHub notifications bot (Dec 19 2024 at 19:35):alexcrichton merged PR #9866.
Last updated: Dec 23 2024 at 12:05 UTC