Skip to content

Commit

Permalink
Fix off-by-2 in codegen
Browse files Browse the repository at this point in the history
There were two separate off-by-1's in the codegen code that is
trying to detect assignmnents to slots inside try/catch regions.

First, it was asking to include the value of the catch label,
which is actually the first statement *not* in the try region.
Second, there was a confusion of 0 and 1 based indexing in
the iteration bounds. The end result of this was that the code
was also looking at the first two statements of the catch region.

This didn't used to be a problem (other than a potentially
over-conservative marking of some slots as volatile), because
our catch blocks always had at least two statements (a :leave
and a terminator), but with the `:leave` change, it is possible
to have catch blocks with only one statement. If these happened
to be at the end of the function, things would blow up.

As a side node, this code isn't particularly sound, because it
assumes that try/catch regions are lexical, which they are not.
The assumption happens to work out ok for the code we generate
in the frontend and optimized IR doesn't have slots, so we don't
use this code, but it is not in general sound.
  • Loading branch information
Keno committed Nov 22, 2023
1 parent 8ff9d34 commit 09dfdf9
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/codegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2924,7 +2924,7 @@ static bool local_var_occurs(jl_value_t *e, int sl)
static std::set<int> assigned_in_try(jl_array_t *stmts, int s, long l)
{
std::set<int> av;
for(int i=s; i <= l; i++) {
for(int i=s; i < l; i++) {
jl_value_t *st = jl_array_ptr_ref(stmts,i);
if (jl_is_expr(st)) {
if (((jl_expr_t*)st)->head == jl_assign_sym) {
Expand All @@ -2946,7 +2946,7 @@ static void mark_volatile_vars(jl_array_t *stmts, SmallVectorImpl<jl_varinfo_t>
if (jl_is_expr(st)) {
if (((jl_expr_t*)st)->head == jl_enter_sym) {
int last = jl_unbox_long(jl_exprarg(st, 0));
std::set<int> as = assigned_in_try(stmts, i + 1, last);
std::set<int> as = assigned_in_try(stmts, i + 1, last - 1);
for (int j = 0; j < (int)slength; j++) {
if (j < i || j > last) {
std::set<int>::iterator it = as.begin();
Expand Down

0 comments on commit 09dfdf9

Please sign in to comment.