Stream: git-wasmtime

Topic: wasmtime / PR #4447 cranelift: Correctly parse ISA value ...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 12:42):

afonso360 opened PR #4447 from clif-util-parse-values to main:

When parsing ISA specific values we were accidentally discarding the value of the flag, and treating it always as a boolean flag.

This would cause a clif-util invocation such as
cargo run -- compile -D --set has_sse41=false --target x86_64 test.clif

to be interpreted as --set has_sse41 and enable that feature instead
of disabling it.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:01):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:01):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:01):

abrown created PR review comment:

I guess I don't see how changing the error type returned fixes the issue you mention in the PR description. Shouldn't BadName still map to UnknownFlag but the UnknownValue be used for some new case where the value isn't understood?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:11):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:11):

afonso360 created PR review comment:

In cranelift/src/utils.rs we parse some flags before knowing the target triple. Once we know the triple we parse those flags again but now with the correct triple. However returning UnknownFlag here discarded the value of this Value and made it a flag, which is incorrect when we are trying to disable a value since the Flags only ever enable values.

Shouldn't BadName still map to UnknownFlag but the UnknownValue be used for some new case where the value isn't understood?

It does in the branch above for TestOption::Flag

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 18:11):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 20:07):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 20:07):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 20:07):

abrown created PR review comment:

Ah, ok, I think I see now; thanks for the explanation.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2022 at 20:08):

abrown merged PR #4447.

Last updated: Nov 22 2024 at 16:03 UTC