[Mono-devel-list] Patch for wrong out BB

[Massimiliano Mantione]

Sorry for the long message.

The short version is: is it really needed to link the last BB to
the end BB?
The standard does not allow the control flow to reach the end of
a method, so that link seems not needed, and the patch gets even
simpler.

Actually, this message started in a different way, I was asking a
different question, then I checked what I was saying, then checked
again... if you want to get bored seeing how long it took me to
get here, read along, otherwise just stop here and go directly to
the patch ;-)

Ciao,
  Massi

On Tue, 2004-12-07 at 18:40, Paolo Molaro wrote:
> Besides CEE_BR, CEE_BR_S, CEE_SWITCH we have also CEE_RET,
> CEE_JMP, CEE_THROW, CEE_ENDFILTER, CEE_ENDFINALLY, CEE_RETHROW,
> CEE_LEAVE, CEE_LEAVE_S. Hope I didn't miss any. Using the ip var
> is better, because after the internal representation is generated,
> there is no easy way to check bblock->last_ins e check for all
> these cases.

Looking at the code, I noticed that "start_new_bblock != 0" if and
only if the last BB instruction is one of the following:

CEE_JMP, CEE_RET, CEE_BR_S, CEE_BR, CEE_THROW, CEE_ENDFINALLY,
CEE_LEAVE, CEE_LEAVE_S, CEE_MONO_RETOBJ, CEE_ENDFILTER, CEE_RETHROW.

Moreover, it is set also when ADD_UNCOND and ADD_UNCOND are used,
which is with the "CEE_BR(FALSE|TRUE)(_S)?" and "CEE_B(..)(_UN)?(_S)?"
opcode families (conditional branches), and in the presence of tail
calls (ins_flag & MONO_INST_TAILCALL).

With respect to your opcode list, CEE_SWITCH is missing... I looked
at the code, and it seems that in the CEE_SWITCH case it is assumed
that there *must* be code just after the switch instruction.

Checking the ECMA specs, I sow that the instruction following the
switch is the "fall through", and it is mandatory just like the
"second" target for conditional branches (as I wasn't sure I got the
spec correctly, I also did a small IL method with a switch as last
instruction, and the verifier refused it).

All this just to say that a switch is just like an unconditional
branch: it cannot be at the end of a method code block, otherwise
the verifier doesn't accept the code (and so, coding the patch as
you proposed, "no_link_needed_at_ip = ip" would not be needed  in
the CEE_SWITCH case).

But, as I said, in any case "start_new_bblock != 0" marks the fact
that the last block ended with a "branch", and therefore does not
need to be linked to the end block, so I was thinking to use this
in my patch, instead of adding the "no_link_needed_at_ip" variable.
In my thoughts, this sounded simpler, and seemed to also fit better
with how the existing code works ("start_new_bblock" is already the
"marker" between BBs).

Then I noticed that "start_new_bblock != 0" *must* be true, the
verification fails otherwise.
After one more test program, I checked the ECMA spec again, and
found out that "Control is not permitted to simply  fall through
the end of a method. All paths shall terminate with one of these
instructions: ret, throw, jmp, or (tail. followed by call, calli,
or callvirt)" (Partition I, section 12.4, point 6).

So the patch became like this...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: branch-on-last-bb.patch
Type: text/x-patch
Size: 960 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20041210/fa027cb1/attachment.bin