[PATCH] D141103: [WebAssembly] Ensure 'end_function' in functions

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:880
+        //    without encountering a .functype directive after the label.
+        if (CurrentState != FunctionLabel) {
+          // This .functype indicates a start of a function.
----------------
sbc100 wrote:
> It seems like if this `.functype` is the start of a function this condition should be `CurrentState == FunctionLabel`, no?
No. Because the grammar doesn't seem to be very rigidly fixed, many cases are possible.

```
.functype test () ->()
test:
  .functype test () -> ()
```
This looks to be the case for the generated .s files now. Each function has two `.functype` directives. Because of the first `.functype`, by the time we parse `test:`, we know it is a function label. (This changes the state to `FuncLabel`) And the second `.functype` will run this code. (This changes the state to `FuncStart`) Note that the first `.functype` directive does not run this part of the code because it fails to meet `if (WasmSym->isDefined())` at that point. So in this case the state changes are `FileStart` -> `FuncLabel` -> `FuncStart`.

```
test:
  .functype test () -> ()
```
This lacks the `.functype` before the function. We don't seem to mandate `.functype` before a function label. We currently accept this, and many functions in the tests under `test/MC/WebAssembly` also lack the `.functype` before the function label. In this case, by the time we parse `test:`, we don't know it is a function label. So we don't enter `FuncLabel` state. But by the time we parse `.functype` after that, we run this code and change the state to `FuncStart`. So the state changes are `FileStart` -> `FuncStart`.

```
.functype test () -> ()
test:
```
This is the invalid file https://github.com/llvm/llvm-project/issues/57427 reported. Unlike the case before a label, we mandate `.functype` after a function label. (Otherwise we don't enter `FuncStart` state at all.) But we don't reject this file; we just silently create an invalid binary, because we don't have any other instruction or `end_function` in this function, so we don't end up calling `ensureLocals`. But this `test` is still recognized as a function because of the `.functype` before the label, so this writer tries to write this function to the binary but doesn't end up emitting the local info. What this CL does is just reject this kind of malformed file.


So the state doesn't always move `FuncLabel` -> `FuncStart`. It sometimes moves to `FuncStart` directly. So what I wanted was to push `Function` only once for them. (We can't create two separate things to push because sometimes we don't enter `FuncLabel`) Hence the condition:
```
  if (CurrentState != FunctionLabel) {
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141103/new/

https://reviews.llvm.org/D141103