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

[Paolo Molaro]

On 12/06/04 Massimiliano Mantione wrote:
> I noticed that in mono_method_to_ir, when creating BBs, the last
> "proper" BB is always linked to the exit BB, even when it has an
> explicit branch instruction at the end.

Good catch.

> OK to commit?

I don't think your patch is correct/complete.

> Index: mono/mono/mini/mini.c
> ===================================================================
> --- mono/mono/mini/mini.c	(revision 37209)
> +++ mono/mono/mini/mini.c	(working copy)
> @@ -5846,7 +5846,26 @@
>  
>  	bblock->cil_length = ip - bblock->cil_code;
>  	bblock->next_bb = end_bblock;
> -	link_bblock (cfg, bblock, end_bblock);
> +	if (bblock->last_ins != NULL && ((bblock->last_ins->opcode == CEE_BR) ||
> +			(bblock->last_ins->opcode == CEE_BNE_UN) ||

CEE_BR would be ok and it's what is tested in the even-odd test.
But the conditional branches should never happen here, as the not taken
branch is a follow-through and it gets out of the method code. The verifier
has a check for that, so the extra checks would be redundant even if
they were correct.

> +			(bblock->last_ins->opcode == CEE_SWITCH) )) {

CEE_SWITCH is fine, but a few more opcodes are missing. I think
a better way to implement this is to add a local:
	const char *no_link_needed_at_ip;
The var is set with:
	no_link_needed_at_ip = ip;
at the end of each case for opcodes that need this handling.
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.
Care to rework the patch and test it?
Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better