Stream: git-wasmtime

Topic: wasmtime / issue #4561 Optionally add `log::trace!` calls...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 22:50):

github-actions[bot] commented on issue #4561:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 30 2022 at 00:00):

elliottt commented on issue #4561:

I realized that #4484 hadn't merged yet and I had forgotten to do a review (and now it needs a rebase to merge) but once it merges, we should update this PR to use that trace! macro (which can be conditionally compiled out) rather than explicitly use log::trace!. (I realize you were trying to do something similar earlier but I think it makes sense to have both: the ISLE-level option to exclude from codegen completely, and the Cranelift-level custom trace macro. Probably we should make the codegen option a string option naming the trace macro...)

I like that idea a lot! Being able to control the name of the macro called would also make it easier to use this to instrument the generated rust, which we might be able to take advantage of during testing.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 18:48):

jameysharp commented on issue #4561:

Is it important to have a build-time flag for whether to generate this code? Or would it be okay to just always generate the trace! call? I assume this is a code path that's not supposed to happen, because we're supposed to have ISLE rules covering all of CLIF. If that's true then there shouldn't be any runtime cost to including the logging calls, and actually I wonder if it should be a "warn" level log message or something. Anyway, is the flag just to avoid clutter in the generated code or something?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 18:52):

elliottt commented on issue #4561:

Is it important to have a build-time flag for whether to generate this code? Or would it be okay to just always generate the trace! call? I assume this is a code path that's not supposed to happen, because we're supposed to have ISLE rules covering all of CLIF. If that's true then there shouldn't be any runtime cost to including the logging calls, and actually I wonder if it should be a "warn" level log message or something. Anyway, is the flag just to avoid clutter in the generated code or something?

The reason for the flag was mostly that the tests aren't compiled in a context that has access to the trace! macro.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 19:00):

jameysharp commented on issue #4561:

The reason for the flag was mostly that the tests aren't compiled in a context that has access to the trace! macro.

Huh, I don't understand that answer. It's okay to call log::trace and the like without having any logging backend initialized; they just don't do anything in that case. What do you mean by "has access to" here?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 19:31):

elliottt commented on issue #4561:

The reason for the flag was mostly that the tests aren't compiled in a context that has access to the trace! macro.

Huh, I don't understand that answer. It's okay to call log::trace and the like without having any logging backend initialized; they just don't do anything in that case. What do you mean by "has access to" here?

The tests compile the output of islec with rustc directly, and it's difficult to get the appropriate crate dependencies resolved to run the compiler without cargo.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 08 2022 at 22:57):

jameysharp commented on issue #4561:

Ahhh, that's what I was missing (cranelift/isle/isle/tests/run_tests.rs in particular).

I'm not sure I like relying on the enclosing crate having a trace macro at top-level. From offline discussion with you, I like your idea of making the config option be the name of the macro, if any, so ISLE's caller can use println or log::warn or whatever they want.

Instead of threading the config options through all the method calls, I think you should put them in self (the Codegen struct) so they're available everywhere without all that noise. I'd have a much easier time reviewing this PR then.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 20 2022 at 23:01):

elliottt commented on issue #4561:

After playing around with this a bit, it's not actually that useful: the log message printed tells you that the top-level function you applied failed, which you already knew :)

Last updated: Nov 22 2024 at 16:03 UTC