[Mono-bugs] [Bug 75738][Maj] Changed - remove_block_if_useless trips on valid IL

Fri Aug 19 15:42:37 EDT 2005

Please do not reply to this email- if you want to comment on the bug, go to the
URL shown below and enter your comments there.

Changed by massi at ximian.com.

http://bugzilla.ximian.com/show_bug.cgi?id=75738

--- shadow/75738	2005-08-09 07:05:50.000000000 -0400
+++ shadow/75738.tmp.15090	2005-08-19 15:42:37.000000000 -0400
@@ -1,12 +1,12 @@
 Bug#: 75738
 Product: Mono: Runtime
 Version: 1.1
 OS: unknown
 OS Details: amd64 pld linux
-Status: NEW   
+Status: ASSIGNED   
 Resolution: 
 Severity: Unknown
 Priority: Major
 Component: JIT
 AssignedTo: mono-bugs at ximian.com                            
 ReportedBy: malekith at pld-linux.org               
@@ -81,6 +81,107 @@
 It happens both on x86 and amd64.
 
 ------- Additional Comments From vargaz at gmail.com  2005-08-07 15:13 -------
 This seems to be yet another remove_block_if_useless bug. massi, could
 you look at it ?
 
+
+------- Additional Comments From massi at ximian.com  2005-08-19 15:42 -------
+
+This bug, after all, does not seem to be related to
+"remove_block_if_useless".
+To be *really* sure, I #ifdeffed it out, and the bug persists.
+
+The real problem seems that "move_basic_block_to_end" does not
+"keep connected" two consecutive BBs.
+
+It can be seen this way: the following IL (part of the test)
+
+        IL_000d:  brfalse IL_0017
+        IL_0012:  br IL_0021
+// This is BB 6
+        IL_0017:  ldstr "ble"
+        IL_001c:  call void class
+[mscorlib]System.Console::WriteLine(string)
+// This is BB 7
+        IL_0021:  ldstr ""
+        IL_0026:  newobj instance void class
+[mscorlib]System.Exception::.ctor(string)
+        IL_002b:  throw
+
+becomes this sequence in the JIT
+
+	(bne.un[B7B6] (compare (ldind.u1 regoffset[-0x9(%ebp)]) iconst[0]))
+DUMP BLOCK 6:
+	(outarg iconst[136716224])
+	voidcall[WriteLine]
+DUMP BLOCK 1:
+DUMP BLOCK 7:
+	(outarg iconst[136045336])
+	(stind.ref regoffset[-0x4(%ebp)] call[mono_object_new_fast])
+	(outarg iconst[136728544])
+	(outarg (ldind.ref regoffset[-0x4(%ebp)]))
+	voidcall[.ctor]
+	(throw (ldind.ref regoffset[-0x4(%ebp)]))
+	not_reached
+
+Don't care about the BB with a CEE_BR at IL_0012, it is not the
+point, "remove_block_if_useless" safely eliminates it, but in any
+case also other branch optimizations take it away, logged like this:
+
+cbranch2 to branch triggered 5 -> (9) 7 (0x38)
+nullify block triggered 9
+
+The real issue is that BB 1 is inserted between BB 6 and BB 7 because
+BB 7 is "out_of_line", and so gets moved to the end.
+However, BB 6 *requires* to be followed by BB 7 (the test should 1st
+print "ble" and then throw the exception).
+The "move_basic_block_to_end" function looks for the previous BB, but
+just to link it to the new next BB, without checking if a CEE_BR must
+be added to preserve the fact that the previous BB expected to "fall
+through" into its next (which is being moved at the end).
+
+So, the bug is the fact that BB 1 (which on x86 emits the sequence
+"leave, ret") is executed just after BB6, so that BB7 is skipped.
+
+Obviously the fix would be to check if a CEE_BR must be added to the
+"previous BB" (in this example BB 6), and adding it if needed (in this
+case jumping to BB 7).
+
+The reason why I'm writing this long comment is that I can imagine one
+case where this cannot be done: if the "previous BB" (think BB 6) ends
+with a CEE_SWITCH.
+In this case the fall through behavior is crucial (it is the "default"
+of the switch), but we cannot add a CEE_BR after a CEE_SWITCH in the
+same BB.
+So, to preserve the fall through, a new BB should be created and put
+just after the "previous BB", and the CEE_BR should be put in this new
+BB.
+
+What bothers me is that creating a BB after "cfg->bblocks" has been
+allocated and "df_visit" has been called is (IMHO) a nightmare.
+
+Luckily, optimize_branches is called two times: one before df_visit,
+and the other after mono_ssa_remove.
+One could assume that the need to move out of band BBs emerges just
+in the 1st call to optimize_branches, where creating BBs is still an
+easy thing.
+
+So, my plan is:
+- Add a gboolean parameter to optimize_branches, stating if BBs can
+  be created (let's call it "can_create_blocks").
+  Pass TRUE in the 1st call, FALSE in the 2nd.
+- Inside optimize_branches, do one of the following:
+  [a] Modify the "cbranch to throw block triggered" code to check if
+      the optimization can be applied (if the "previous BB" ends with
+      a CEE_SWITCH, can_create_blocks must be TRUE).
+  [b] If can_create_blocks is FALSE, skip the "cbranch to throw block
+      triggered" section completely.
+- In any case add the missing CEE_BR :-)
+
+Now, my questions are:
+Is this analysis correct?
+Do you prefer option a or b?
+
+Ciao,
+  Massi
+
