This PR introduces the BrStack member to store the info about
loop, block, if and try. It can check whether br immediate number
out of range and whether type stack is mismatched in br instr.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Could delegate be checked similarly here too?
Thinks for mentioning. I have fixed the behavior of end block to avoid incorrect BrStack.
Principle it is similarly expect the behavior in catch block. I think it can be done in another patch.
This looks pretty good!
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
218–219 | Should this be outside the loop over the params? Otherwise we start with the same stack when checking each parameter, right? | |
llvm/test/MC/WebAssembly/type-checker-control-flow.s | ||
132 | It would be good to add a newline here. | |
llvm/test/MC/WebAssembly/type-checker-errors.s | ||
524–525 | This is actually valid. The rule for br is that if the label has type [t*], the stack before the br can be [t1* t*] for any t1*. The br implicitly drops everything in t1*. |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
54–74 | I think you can move this to the header, given that this doesn't have a body | |
63 | I don't think we need to link the formal spec here; it's assumed the implementer and the reader of this file either knows or is able to look for it. We don't link it elsewhere in the file either. | |
160–161 | ||
180 | Shouldn't we do -1? If not, the message can be something like br: invalid depth 3 (max 3) But -1 also has a problem, because when BrStack is empty it will print something like br: invalid depth 0 (max -1) So if we want to print (max N), it would be better to handle that case smoothly too. | |
185 | ||
195 | ||
196–229 | ||
197 | TODO aheejin copy? do we use this later again? | |
199–200 | ||
204–205 | Why is this and what does this code do? | |
236–241 | I think you applied clang-format to the whole file. While we try to keep files clang-formatted, I think it'd be better not to mix this with other functionality changes. Can you revert this? Tip: git clang-format main will clang-format only the changed lines, not the whole file. You can replace main with another branch or a commit if necessary. | |
279 | Please revert unrelated clang-format changes | |
417–421 | Please revert unrelated clang-format changes | |
433–435 | Please revert unrelated clang-format changes | |
482–484 | While is this here, while other block-like instructions (block, try, and loop) are checked separately (line 391-402) above? | |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h | ||
41 | ||
llvm/test/MC/WebAssembly/basic-assembly.s | ||
84–88 | Why is this change necessary? |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
197 | Sorry, forgot to remove my own memo here, and Phabricator doesn't give a way to delete inline comments... Please ignore the message above. By the way, a question: this looks copying the stack intentionally not to modify BrStack.back().EnterStack, correct? |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
448–452 | I don't think we should restore try's EnterStack here? In a catch body, the only input params are the caught values, not the try's input types. |
I will change those point and add comment also in code.
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
180 | I also think it is -1, but when I use MDN's web runner for following code (module (import "console" "log" (func $log (param i32))) (func $hhh i32.const 1 call $log br 0 i32.const 2 call $log ) (start 1) ;; run the first function automatically ) `` It outputs `1` I am not sure whether func body is a block which can also `br` to? | |
197 | correct. here just checks whether the parameters of label are fulfilled. i32.const 1 block (i32, i32) -> (i32) end_block | |
204–205 | consider this case i32.const 1 if // store op stack return // here can do everything because of stack-polymorphic else // here stack should be recovered as stored end_if | |
236–241 | Could it possible to submit a format patch before it? | |
llvm/test/MC/WebAssembly/basic-assembly.s | ||
84–88 | The old file is invalid but not be detected. (The op stack of true and false branch in if block are not balanced) |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
448–452 | The semantic of catch instructions in try [T1->T2] catch [T3] is [T2 -> T3]. .tagtype TAG f32 i32.const 1 try i32 f32.const 1.0 throw TAG // here ok because unreachable drop catch TAG i32.trunc_f32_s end_try // here type stack is [i32, i32] but without restore we will get error |
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
448–452 | block [T1] xxxxx end_block try [T1->T2] instr1* catch [T3] instr2* end_try after instr1*, the type stack should [T2] and without restore we push T3. After instr2, we will get [T2, T2] which is not incorrect |
D147852 landed, so please rebase onto main and resolve conflicts.
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp | ||
---|---|---|
63 | Stray ; | |
180 | Yes, the function body is also a block you can br to. | |
204–205 | (The comments have moved so it's hard to track what code this was about, but it was about this code) // In some cases (unreachable / br), `Stack` will be invalid and need to be // recovered. if (Unreachable) Stack = BrStack.pop_back_val().applySignature(); else BrStack.pop_back(); I think we already check this case? (The checker/parser had some bugs but some of them were fixed in D147852 and D147881, so please check them too) Can you give me an example that the current checker/parser misses on this case? Also it's a little confusing that how much of the functionalities this CL adds overlaps with the existing ones. Currently we do some type checking in the parser too, which is hard to remove because the parser needs to check some basic things to parse. The CL header claims it is adding br checking, and I don't think we do br checking elsewhere, so I think it's a good thing to add, but this CL adds a lot more things. Can we possibly simplify this CL by adding only the missing checks? | |
236–241 | Yeah a separate formatting CL is fine, but I wonder why your arc diff unconditionally formats it. There should be a way or an option not to do it. My arc diff asks me, doesn't unconditionally formats files, when submitting a CL, and also it only asks me if the lines I modified are not formatted and doesn't check other lines. What's your arc version? | |
437 | D147852 moves this clause into else if (Name == "end_block" || Name == "end_loop" || Name == "end_if" || ..., so please resolve the conflict | |
482–484 | Ping |