Skip to content
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

Validation rules? #30

Open
jakobkummerow opened this issue Feb 27, 2025 · 2 comments
Open

Validation rules? #30

jakobkummerow opened this issue Feb 27, 2025 · 2 comments

Comments

@jakobkummerow
Copy link
Contributor

The Overview contains some normative language such as these examples:

Elements of the vector of function branch hints must appear in increasing function index order

Branch hint annotations are allowed only on |BRIF| and |IF| instructions

which sounds like the module should fail to validate if these requirements aren't met; but OTOH we're talking about a custom section which never causes validation errors. So what should implementations do?

Should this be a spec-level question, i.e. is this a reason to introduce a kind of section that in terms of validation rules sits somewhere between regular core sections and arbitrary custom sections? (I'm not saying it should be; just that this would be one option.)

Should these rules be understood to apply to the validity of the section (rather than the module), so if one rule is violated, the entire branch hints section is disregarded?

Should implementations devise their own "warning" scheme, such as printing a message to a browser's developer console, or to the process' stderr, if a branch hints section contains invalid contents? That might work well for some scenarios, but e.g. doesn't lend itself well to automated tests.

Should implementations just be lax and quietly disregard individual bits they don't like?


FWIW, the current state of things in V8 is:

  • when index orders are violated, the branch hints section is entirely disregarded
  • when an entry has an invalid size (anything other than 1) or hint (anything other than 0 or 1), the branch hints section is entirely disregarded
  • any entries referring to nonexistent functions or offsets where there are no branch instructions are silently ignored.

The latter is obviously not very strict. Algorithmically it wouldn't be difficult to change it. However, it would have a runtime cost: checking whether the branch instruction offsets are valid is work that currently simply isn't being done; the way the current implementation works is: it parses the branch hints section into a 2-level map func_index -> instruction_offset -> hint, and when the optimizing compiler encounters a branch instruction it queries this map, so any bogus/useless entries in the section get put in the map but then are simply never retrieved from it. Before spending any time on changing anything there, I wanted to ask what the behavior even should be.

@yuri91
Copy link
Collaborator

yuri91 commented Feb 27, 2025

I agree that the language used could be more specific.
And actually whatever we decide should probably apply to all metadata sections, and added in a chapter here: https://webassembly.github.io/branch-hinting/metadata/code/intro.html

As you said, clearly any "failure" is not a module failure (and this is also specified in the spec document above), but what should happen on a "custom section failure" is not specified.

The current V8 behavior is the result of some discussions in the CG and I think it is reasonable. If the section is malformed it should not be trusted to have any useful info.

That said, some form of warning would be useful for development. There is some logging in V8 IIRC but it's not visible by default, let alone shown in the console.
I think that a console warning would be a good idea, but I am not sure that mandating it makes sense.

@yuri91
Copy link
Collaborator

yuri91 commented Feb 27, 2025

ah and about the bogus entries: this was explicitly discussed in the CG, and the conclusion was indeed that checking their validity pro-actively is not useful and potentially expensive. That also informed the implementation in V8.

I do think that the vagueness here could be improved (for example by suggesting/mandating that the section should be discarded if an error is encountered during parsing), but at the same time since the effect of applying the section is not observable, I don't see a problem with leaving the engines some room to decide on their own.

Discarding the section for any reason is always a valid option after all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants