wasmtime / PR #8055 Add initial support for DWARF Fission · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8055 Add initial support for DWARF Fission

Wasmtime GitHub notifications bot (Mar 07 2024 at 01:54):

yowl edited PR #8055.

Wasmtime GitHub notifications bot (Mar 07 2024 at 03:44):

github-actions[bot] commented on PR #8055:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasi", "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Mar 07 2024 at 13:15):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2024 at 13:15):

yowl created PR review comment:

@peterhuene Hi, tagging you as the bot marked you as reviewer, hope that's ok? You'll see I've added a parameter to the constructor here so that I can load the .dwp file at the same time as loading the .wasm (if loading from file). This means there are a zillion tests to update, which is fine with me, but before I do it, do you have any comment about this aspect of this PR?

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton submitted PR review:

Thanks for this! I've got some high level comments about the rough structure here, mostly to help centralize things bit more in the "dwarf processing area" and additionally API guidance for enabling usage of this in all case but not necessarily requiring it to be front-and-center in the embedding API.

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton submitted PR review:

Thanks for this! I've got some high level comments about the rough structure here, mostly to help centralize things bit more in the "dwarf processing area" and additionally API guidance for enabling usage of this in all case but not necessarily requiring it to be front-and-center in the embedding API.

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

I think this snuck in here by accident

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

Would it be possible to thread through the *.dwp bytes through to the debug processing, but otherwise leave them unmodified before then? That'd help keep all the debuginfo processing centralized in one location rather than spread out.

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

Instead of modifying wasmtime-cache, which is pretty carefully crafted as-is to ensure we don't get cache mistakes, could dwarf_package be threaded through with wasm?

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

If these sections are present in the dwarf package are they not present in the original wasm? If so could that be validated here because otherwise this might create dual sections which might confuse some dwarf readers.

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

I think this snuck in by accident

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

If possible I'd recommend avoiding changing the default constructor of Module since, as you've seen, there's a lot of callers. Additionally dealing with split-dwarf-things feels somewhat niche-ish where embedders should all be able to handle it but it shouldn't necessarily be front-and-center of the embedding API.

If you're up for it what I might recommend for something like this is perhaps a ModuleBuilder type, for example:
impl Module {
    pub fn builder() -> ModuleBuilder;
}

impl ModuleBuilder {
    fn allow_wat(&mut self, allow: bool) -> &mut Self; // example configuration method
    fn dwarf_package(&mut self, bytes: &[u8]) -> &mut Self; // explicitly specify dwarf
    fn dwarf_package_file(&mut self, path: impl AsRef<Path>) -> Result<&mut Self>; // explicitly specify path

    // finish the compilation by reading wasm from `path`
    //
    // auto-load `path.dwp` if `dwarf_package` files weren't called
    fn compile_path(&mut self, path: impl AsRef<Path>) -> Result<Module>;

    // finish compilation by using the bytes provided as wasm
    fn compile(&mut self, wasm: &[u8]) -> Result<Module>;
}
or something along those lines. That'll keep Module::new working as-is. Module::from_file would probe for a dwarf package (perhaps with a knob in Config to turn that off), and users with in-memory wasm and dwarf packages can use ModuleBuilder

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

Mind leaving a comment here indicating that this is not parsed from the wasm module and must be configured externally?

Either that or, alternatively, if it makes sense it might be best to thread the dwarf package as an auxiliary parameter through to the debuginfo processing. If there's too many layers to thread through though I think it's reasonable to leave it here.

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

Idiomatically I might recommend some use of ? and early-return to avoid the "tower" here with lots of indentation. For example this line here can be replaced with:
let dwo_id = unit.dwo_id?;
(and no indentation needed afterwards).

The next line could be replaced with:
let cu = dwp.find_cu(dwo_id, parent).ok()??;
(the double ?? is a bit odd but it handles the two layers of Result<Option<_>>)

Wasmtime GitHub notifications bot (Mar 08 2024 at 01:56):

alexcrichton created PR review comment:

Similar to my comment below on Module, I'd recommend avoiding changing this but perhaps have ModuleBuilder::new(engine) work and then have a "finish" method returning just a Vec<u8> rather than a Module, that way the ModuleBuilder could be used with this too.

Wasmtime GitHub notifications bot (Mar 08 2024 at 17:46):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2024 at 17:46):

yowl created PR review comment:

Sounds good to me, thanks! Will get on it.

Wasmtime GitHub notifications bot (Mar 08 2024 at 23:12):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 08 2024 at 23:12):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2024 at 23:12):

yowl created PR review comment:

Thanks, removed

Wasmtime GitHub notifications bot (Mar 09 2024 at 15:02):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2024 at 15:02):

yowl created PR review comment:

From Appendix F in the spec https://dwarfstd.org/doc/DWARF5.pdf:

![image](https://github.com/bytecodealliance/wasmtime/assets/2720041/e86a7273-6cd3-4a39-a549-b2f866b2a7f6)

So they are there, optionally, but with a .dwo suffix. Perhaps I don't need to push any here, which would solve things.

Wasmtime GitHub notifications bot (Mar 10 2024 at 23:34):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2024 at 23:34):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2024 at 23:34):

yowl created PR review comment:

@alexcrichton With the changes for the builder, are you anticipating this lines stays or goes?

Wasmtime GitHub notifications bot (Mar 11 2024 at 00:38):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 11 2024 at 00:38):

yowl created PR review comment:

I'm going to assume it stays as does the extra parameter to build_artifacts except that it will be Option<gimli::DwarfPackage<EndianSlice<'_, LittleEndian>>>

Wasmtime GitHub notifications bot (Mar 11 2024 at 01:47):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 11 2024 at 01:47):

yowl created PR review comment:

Do you mean a tuple (engine, wasm, dwarf_package) or a new struct that has the wasm bytes and the dwarf_package Option<>? Sorry I'm new to this so have a few questtions.

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:49):

alexcrichton created PR review comment:

No worries! And yeah if that works I'd recommend that. The reason for this is that the output is considered to be a function of the inputs so by having an extra parameter it subverts the hashing we use to track when to recompile, so by packing it in the "stuff to hash" we can be sure to trigger a recompile if anything changes.

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:50):

alexcrichton created PR review comment:

Ah ok, sounds good! You could also update push_debug to take an optional suffix perhaps of what to use as the new section name and set that to ".dwo" for these method calls

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:51):

alexcrichton created PR review comment:

I think this line is good to stay. This is sort of the choke point for "everyone before has to figure out how to thread it here" and "everyone after has to figure out how to read it from here".

The build_artifacts function is internal to Wasmtime so we can do whatever we want to its signature.

Wasmtime GitHub notifications bot (Mar 11 2024 at 17:51):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 01:36):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 17 2024 at 01:53):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 01:53):

yowl created PR review comment:

I've taken your outline for ModuleBuilder and have used it as the sole mechanism for creating a module with split dwarf. So I've left Module::from_file and new as is. To be honest I did that, then re-read your comment, so if you think it would be better if from_file understood to read dwarf packages automatically, let me know.

Wasmtime GitHub notifications bot (Mar 17 2024 at 02:00):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 02:00):

yowl created PR review comment:

I've removed this from the signature, but as I have build_component_artifacts taking the dwarf package binary:
https://github.com/bytecodealliance/wasmtime/pull/8055/files#diff-5d6d850074b24b869c91c37ca7a77d9e7d057d0afa44d9ce9fd31ef5774ea1f9R60
I don't quite see how I should be doing this with the builder.

Wasmtime GitHub notifications bot (Mar 17 2024 at 02:01):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 02:01):

yowl created PR review comment:

ok, let me see how that works out.

Wasmtime GitHub notifications bot (Mar 17 2024 at 15:48):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 17 2024 at 15:52):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 15:52):

yowl created PR review comment:

I've passed the bytes through as low as module_environ.cs which is where I'm setting up the debuginfo.dwarf_package. Is that what you had in mine, or having debuginfo.dwarf_packages as bytes also?

Wasmtime GitHub notifications bot (Mar 17 2024 at 16:26):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 16:26):

yowl created PR review comment:

I didn't need these after all, so have removed this change.

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:11):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:11):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:11):

yowl created PR review comment:

Yes indeed that makes things simpler, thanks!

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:12):

yowl edited PR review comment.

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:14):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:14):

yowl created PR review comment:

right-o

Wasmtime GitHub notifications bot (Mar 17 2024 at 18:26):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 17 2024 at 19:07):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 17 2024 at 19:46):

yowl commented on PR #8055:

I think I've addressed the first set of feedback and I think it looks cleaner. There are some safe-to-deploy and some feature looking errors to deal with still

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton submitted PR review:

Looking good! I wanted to offer, though, if you'd like I'm happy to help out with the refactoring required to make ModuleBuilder come into existence. That's a big chunk of this change and isn't related directly to DWARF fission, so I can help out land the refactorings required to get ModuleBuilder itself in place and that would help slim down this change to just DWARF fission.

I say this because I've got a number of comments about specific details about the API, but I don't want to bog you down too much if you're mainly interested in the DWARF bits.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton submitted PR review:

Looking good! I wanted to offer, though, if you'd like I'm happy to help out with the refactoring required to make ModuleBuilder come into existence. That's a big chunk of this change and isn't related directly to DWARF fission, so I can help out land the refactorings required to get ModuleBuilder itself in place and that would help slim down this change to just DWARF fission.

I say this because I've got a number of comments about specific details about the API, but I don't want to bog you down too much if you're mainly interested in the DWARF bits.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

If you're ok changing these constructors of Module I think it'd be best to reframe them all in terms of ModuleBuilder, but I can also have a follow-up to do that refactoring.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

Mind doing a pass over the PR (before the end) to remove noop changes like the ones in this file? There's a few other scattered throughout, and they're all minor. No need to do that immediately though, just something that'd be good to do before merging.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

Could this change to using the ModuleBuilder API and have this implementation move to within the ModuleBuilder?

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

I think it's probably best to leave this out of the top-level documentation and instead refer to it in the documentation of Module itself.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

Ah it's ok to leave this out, I can have a follow-up.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

This is probably best left as unconditionally None for components since components will need more support for loading multiple possible *.dwp files or something like that in the future.

Wasmtime GitHub notifications bot (Mar 18 2024 at 15:14):

alexcrichton created PR review comment:

This comment might be a copy/paste error?

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:16):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:19):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:19):

yowl created PR review comment:

oops

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:24):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:30):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:43):

yowl commented on PR #8055:

I wanted to offer, though, if you'd like I'm happy to help out with the refactoring required to make ModuleBuilder come into existence.

That would be very kind, thanks! You are right my primary goal is DWARF fission, so I can land https://github.com/dotnet/runtimelab/pull/2264 which I can't really do properly without wasmtime support.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:44):

github-actions[bot] commented on PR #8055:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

saulecabrera: winch

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:44):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Mar 18 2024 at 23:44):

yowl created PR review comment:

Yes, I'd like to tackle that after this.

Wasmtime GitHub notifications bot (Mar 19 2024 at 00:02):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 00:05):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 01:04):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 01:07):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 01:18):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 01:31):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 15:32):

alexcrichton commented on PR #8055:

Ok sounds good, I'll take some time today and split out the changes here for ModuleBuilder, make a PR for that, and in theory this should be a relatively straightforward "rebase on that and add a few llines to have support"

Wasmtime GitHub notifications bot (Mar 19 2024 at 16:51):

alexcrichton commented on PR #8055:

Ok I've extracted the refactorings I was envisioning to https://github.com/bytecodealliance/wasmtime/pull/8181

Wasmtime GitHub notifications bot (Mar 19 2024 at 21:37):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 22:38):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 22:54):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 19 2024 at 23:44):

yowl commented on PR #8055:

Hi, Ive got a few of these errors in the Test Linux job:

---- cli_tests::test_programs::run_wasi_http_component stdout ----
thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/object-0.33.0/src/read/wasm.rs:276:64:
index out of bounds: the len is 44 but the index is 357
stack backtrace:
   0: rust_begin_unwind
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14
   2: core::panicking::panic_bounds_check
             at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:208:5
   3: <usize as core::slice::index::SliceIndex<[T]>>::index_mut
   4: <alloc::vec::Vec<T,A> as core::ops::index::IndexMut<I>>::index_mut
   5: object::read::wasm::WasmFile<R>::parse
   6: object::read::any::File<R>::parse
   7: wasmtime::engine::serialization::detect_precompiled_bytes
   8: wasmtime::engine::Engine::detect_precompiled
   9: wasmtime_cli::common::RunCommon::load_module_contents
  10: wasmtime_cli::common::RunCommon::load_module
  11: wasmtime_cli::commands::run::RunCommand::execute
  12: wasmtime::Wasmtime::execute
  13: wasmtime::old_cli::main
  14: wasmtime::main
  15: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Does it ring any bells, I don't immediately see a connection with this PR? If not, I'll start up a Linux system to debug it.

Thanks!

Wasmtime GitHub notifications bot (Mar 20 2024 at 02:10):

philipc commented on PR #8055:

It's a bug in the object crate in a code path that is now being executed because this PR enables the wasm feature for object.

Wasmtime GitHub notifications bot (Mar 20 2024 at 02:23):

philipc commented on PR #8055:

Workaround:

--- a/crates/wasmtime/src/engine/serialization.rs
+++ b/crates/wasmtime/src/engine/serialization.rs
@@ -24,7 +24,7 @@
 use crate::{Engine, ModuleVersionStrategy, Precompiled};
 use anyhow::{anyhow, bail, ensure, Context, Result};
 use object::write::{Object, StandardSegment};
-use object::{File, FileFlags, Object as _, ObjectSection, SectionKind};
+use object::{NativeFile as File, FileFlags, Object as _, ObjectSection, SectionKind};
 use serde_derive::{Deserialize, Serialize};
 use std::str::FromStr;
 use wasmtime_environ::obj;

Alternatively ElfFile64 instead of NativeFile.

Wasmtime GitHub notifications bot (Mar 20 2024 at 17:14):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 20 2024 at 17:38):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 20 2024 at 18:10):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 20 2024 at 18:52):

yowl commented on PR #8055:

@philipc Thanks, that gets this green. I'll merge in from main when #8181 is in, give it a once over, see if there is a sensible test to add, then mark it ready for review.

Wasmtime GitHub notifications bot (Mar 21 2024 at 15:32):

alexcrichton commented on PR #8055:

With https://github.com/bytecodealliance/wasmtime/pull/8181 now landed if you want to rebase @yowl I can help review any remaining bits

Wasmtime GitHub notifications bot (Mar 29 2024 at 17:15):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 18:10):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 18:46):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 19:30):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 20:04):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 20:06):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 29 2024 at 20:36):

yowl commented on PR #8055:

Having merged with main, looks like I have a problem with wasmparser versions, 0.202.0 is the version generally used and listed in the toml, but object (version 0.33 and 0.34) wants 0.201.0. Is there are sensible way to resolver this?

Wasmtime GitHub notifications bot (Mar 29 2024 at 20:36):

yowl edited a comment on PR #8055:

Having merged with main, looks like I have a problem with wasmparser versions, 0.202.0 is the version generally used and listed in the toml, but object (version 0.33 and 0.34) wants 0.201.0. Is there are sensible way to resolve this?

Wasmtime GitHub notifications bot (Mar 29 2024 at 21:40):

philipc commented on PR #8055:

I'm not a wasmtime developer, but as the object maintainer I can say that I think object has a slower release cadence, and this is likely to be an ongoing problem. wasmtime already has code to read DWARF sections from Wasm files; have you considered using that instead of enabling the wasm feature for object?

Wasmtime GitHub notifications bot (Mar 29 2024 at 23:16):

alexcrichton commented on PR #8055:

Ah yeah no need to worry about that, we already have an exception for wit-bindgen's transitive dependency on wasmparser and we can add another for object if needed. We definitely don't expect object to be released at the same pace as wasmparser.

As @philipc said though if the wasm feature isn't needed and/or could be disabled that'd probably be best, but whatever works best for you @yowl we can make it work.

Wasmtime GitHub notifications bot (Mar 31 2024 at 10:38):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Mar 31 2024 at 10:39):

yowl commented on PR #8055:

Seems like whatever reason I had for adding the wasm feature, has gone now, so will revert that change.

Wasmtime GitHub notifications bot (Apr 05 2024 at 13:03):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 13:35):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 16:52):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 19:26):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 19:40):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 22:36):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 05 2024 at 23:19):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 06 2024 at 15:36):

yowl commented on PR #8055:

I've been looking at the tests in tests\all\debug\lldb.rs which seems like a place where a test could go checking the result of fr v for a simple c program with some dwp. I note that all the tests here are marked ignore which presumably means they dont run as part of the CI. Why is that?

Wasmtime GitHub notifications bot (Apr 10 2024 at 19:43):

yowl commented on PR #8055:

@alexcrichton Hi, do you know why the lldb tests are ignored ? Is there a reason they dont run in the CI ?

Wasmtime GitHub notifications bot (Apr 11 2024 at 14:50):

alexcrichton commented on PR #8055:

Oh the setup is actually a little wonky, but they're ignored-by-default but run-by-default here. The original intention was to avoid placing extra platform restrictions on those running tests since lldb-with-wasm-support I think required a patch or was otherwise somewhat nontrivial to acquire at the time.

Wasmtime GitHub notifications bot (Apr 13 2024 at 22:49):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 13 2024 at 22:59):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 13 2024 at 23:28):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 13 2024 at 23:43):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 14 2024 at 01:06):

yowl commented on PR #8055:

I added a test for lldb with split dwarf using a c program I compiled with emscripten which checks for the ability to look at frame variables. This is passing so I'll mark this ready for review. Thanks for the help!

Wasmtime GitHub notifications bot (Apr 14 2024 at 01:06):

yowl has marked PR #8055 as ready for review.

Wasmtime GitHub notifications bot (Apr 14 2024 at 01:06):

yowl requested alexcrichton for a review on PR #8055.

Wasmtime GitHub notifications bot (Apr 14 2024 at 01:06):

yowl requested wasmtime-core-reviewers for a review on PR #8055.

Wasmtime GitHub notifications bot (Apr 14 2024 at 01:06):

yowl requested wasmtime-default-reviewers for a review on PR #8055.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

In addition to setting self.dwarf_package, could this file also set self.dwarf_package_path? For example the mutation happening below in dwarf_package_from_wasm_path would be pushed into this method. That way if this method is called independently then it'll set self.dwarf_package_path automatically.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Mind making this not-pub? (I think it's only intended for internal bits)

As a minor nit here this might most appropriately return Option<&[u8]> as well, but that's just a minor thing.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Could you expand the documentation here about what this method is doing under the hood in terms of when this is required and what the *.dwp is used for?

For example this method isn't required with wasm_file since it'll probe for the *.dwp automatically.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Could you update the documentation to indicate that the *.dwp file is probed for?

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

If it works for you I'd prefer to leave out this in this method if we can. I think it's good to have some "pure in memory methods" for the builder that don't touch the filesystem, and this wasm method was intended for that. The wasm_file is a nice convenience for probing the filesystem for *.dwp, but here wasm_path is mostly an error message hint rather than a location on disk per se.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Could this method change to returning a Result? That'd avoid the need for .unwrap() and the eprintln! could be translated into an error to get propagated upwards as well.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

(this is present in a number of other modified manifests in this PR as well, so if not used here could you remove all the other additions too?)

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Could the unwrap() be removed here and handled? I'd be otherwise worried that a malformed file might accidentally cause a panic here.

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

If you're ok with it, could this be omitted? This is a convenience for CodeBuilder methods, and I'd personally prefer to leave combining the method to embedders where we can. (in retrospect we can probably trim the constructors of Module by deferring more use cases to CodeBuilder as well)

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Or, alternatively, load_dwp failures are intended to be squashed here, mind changing the eprintln to a log::warn or similar?

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

Searching around I wasn't able to find new uses of these deps, so are these still needed or perhaps leftover from a previous iteration?

Wasmtime GitHub notifications bot (Apr 15 2024 at 16:20):

alexcrichton created PR review comment:

This pattern you can also express as:
if let Some(program_unit) = split_unit {
    // ...
}

Wasmtime GitHub notifications bot (Apr 20 2024 at 02:17):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 02:17):

yowl created PR review comment:

Thanks

Wasmtime GitHub notifications bot (Apr 20 2024 at 02:34):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 02:34):

yowl created PR review comment:

The thing is program_unit is reassigned here, it starts as unit from the main module and is reassigned if there is a split unit, so I can't use let as it is declared a few lines higher up.

Wasmtime GitHub notifications bot (Apr 20 2024 at 13:51):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 20 2024 at 13:52):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 13:52):

yowl created PR review comment:

Used a if let Some and have ignored the None case.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:02):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:23):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:45):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:45):

yowl created PR review comment:

Thanks, done.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:45):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:45):

yowl created PR review comment:

Removed.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:46):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:46):

yowl created PR review comment:

sure, deleted

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl created PR review comment:

Thanks, done.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl created PR review comment:

sure, removed.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl created PR review comment:

added some more docs

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:48):

yowl created PR review comment:

Thanks, done.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:50):

yowl submitted PR review.

Wasmtime GitHub notifications bot (Apr 20 2024 at 14:50):

yowl created PR review comment:

Its used in https://github.com/bytecodealliance/wasmtime/pull/8055/files#diff-92db9b7b9b675f97d5c8d9d94e0680d63aab09fb1454f5441cb61ddbff8d3d81R17

Have changed the return type, thanks.

Wasmtime GitHub notifications bot (Apr 22 2024 at 17:28):

alexcrichton updated PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 17:30):

alexcrichton submitted PR review:

Thanks again for this! I had some more minor nits but I went ahead and pushed up the changes directly. If anything there looks off to you though please let me know

Wasmtime GitHub notifications bot (Apr 22 2024 at 17:30):

alexcrichton has enabled auto merge for PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 17:57):

alexcrichton updated PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 17:57):

alexcrichton has enabled auto merge for PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 18:21):

alexcrichton updated PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 18:21):

alexcrichton has enabled auto merge for PR #8055.

Wasmtime GitHub notifications bot (Apr 22 2024 at 18:58):

alexcrichton commented on PR #8055:

@yowl this failure looks legitimate in the sense of probably-caused-by-this PR. Do you know if that perhaps means the LLDB we're using in CI is missing something and/or is too old?

Wasmtime GitHub notifications bot (Apr 22 2024 at 19:05):

yowl commented on PR #8055:

This looks relevant https://github.com/llvm/llvm-project/issues/55575 Seems like we have to add some missing python stuff.

Wasmtime GitHub notifications bot (Apr 23 2024 at 02:08):

yowl commented on PR #8055:

Although its odd the other tests are passing. What version of Ubuntu is it? Can we run lldb -P to see what paths are expected to be present?

Wasmtime GitHub notifications bot (Apr 23 2024 at 14:33):

alexcrichton commented on PR #8055:

I'm not sure myself, we don't have anything special beyond the standard github actions runners. If you'd like you can push up a commit with "prtest:full" in the commit message and it'll run the full test suite here on the PR to help debug.

Wasmtime GitHub notifications bot (Apr 23 2024 at 15:15):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 16:14):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 16:44):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 17:11):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 17:43):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 18:10):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 18:29):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 19:02):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 19:24):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 20:02):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 23 2024 at 20:25):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 24 2024 at 02:13):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 24 2024 at 12:31):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 24 2024 at 12:59):

yowl updated PR #8055.

Wasmtime GitHub notifications bot (Apr 24 2024 at 13:22):

yowl commented on PR #8055:

@alexcrichton , change a couple of things

Added workaround for https://bugs.launchpad.net/ubuntu/+source/llvm-defaults/+bug/1972855
Forced use of latest lldb available, 15
Added logic to CodeBuilder to load dwp when a wasm path is supplied.
Fixed inconsistency in source file names

Wasmtime GitHub notifications bot (Apr 24 2024 at 14:23):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 24 2024 at 14:54):

alexcrichton merged PR #8055.

Last updated: Dec 23 2024 at 13:07 UTC