Fix unreachable BrOn branch analysis

[kripken]

We skipped the branch if we were unreachable, but that is not correct, see
visitBreak above - we need to note all the branches, taken or not, or else
we can hit an assertion. (We rely on optimization passes to remove
unreachable branch instructions.)

[tlively]

The br_on is not actually unreachable, right? It just unconditionally traps because of the null descriptor.

[kripken]

Right, I meant to say it is not reached/taken 😄

[kripken]

No, wait, the comment is already right. It does become unreachable after refinalizing, see the output.

[tlively]

How can that be right? This is the output:

 ;; CHECK-NEXT:    (block ;; (replaces unreachable BrOn we can't emit)
 ;; CHECK-NEXT:     (drop
 ;; CHECK-NEXT:      (ref.null none)
 ;; CHECK-NEXT:     )
 ;; CHECK-NEXT:     (drop
 ;; CHECK-NEXT:      (ref.null none)
 ;; CHECK-NEXT:     )
 ;; CHECK-NEXT:     (unreachable)
 ;; CHECK-NEXT:    )

Neither child of the BrOn is unreachable, so the BrOn should not be unreachable.

[kripken]

The BrOn is not actually unreachable, but it is unPrintable, here is the Printing logic:

binaryen/src/passes/Print.cpp

Lines 2918 to 2921 in a0463cb

	if (Properties::hasUnwritableTypeImmediate(curr)) {
	printUnreachableReplacement(curr);
	return;
	}

When we lack a defined type to write out the br_on* line, we emit this. So it happens for unwritable type immediates, not just for unreachable BrOns.

[tlively]

Right. Let's update the comment to describe it as unconditionally trapping rather than unreachable.

[tlively]

LGTM with wording tweak.