-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
u32and should accept err #1300
Comments
There are actually quite a few instructions that can trap for various reasons, and have this issue. One can work around this specific case by using I think the larger issue of being able to pinpoint where an error/assert was triggered will need to be solved with debug info. Without debug info, even when you can specify an error code, it is still not particularly helpful in quickly finding the cause. With debug info, we can at the very least provide the location in the original source code, and in cases where we have sources available, we can even render the source snippet that is relevant. More importantly, it would work well across all instructions, whether the error is explicitly triggered with an assert, or implicitly triggered by the semantics of a specific instruction. Maybe we should add a tracking issue for that, where we can work out the details, and link all these issues to that one? |
@bitwalker sounds good to me 👍 |
One note: adding error codes to instructions which also support immediate values may be a bit tricky (or at least may require modifying syntax a bit). So, this may conflict a bit with #1301. |
@bobbinth I think it could be an either or situation:
OR
In other words, validating the sole operand for the immediate variant is trivially done with |
We could do the same for both operands:
And this will actually be faster than using |
Ah right, always forget about |
@bobbinth @bitwalker So what is our approach? Do we stick with an |
IMO if one has to choose, it makes sense to support immediates first, error codes second, since one can use assertions before an op to obtain useful errors and therefore get the best of both worlds. But I think a compromise here is to simple make it an either/or situation, i.e. you can either use immediate operands, or specify an error code, but you can't do both. This lets you choose an option that is most appropriate for the situation. I do think we could support both at the same time, by requiring the Thoughts? |
I agree, I think that an either/or compromise is a good solution for now indeed. Supporting them both at the same time looks a little overloaded for me, but I can't come up with a better solution. Also I think that an ordering of the immediate value and an error code can be any: AFAIK lalrpop allows to parse them in any order. Motivation for this is that user probably won't remember the correct ordering, so it will be more user-friendly to support both. But it will make parsing more complex, so I don't have a strong preference here. |
Here are my thoughts:
Is not all that much worse than:
And actually makes the condition for the error a bit more explicit. Basically, unless someone has a strong preference for the second option above, I'd rather just close this issue. |
Based on the discussion above, closing this as not planned. |
Similar to #1264 , but for the instructions
u32and
. The VM halts if the operands are not u32, and a error code would be benefitial.The text was updated successfully, but these errors were encountered: