M-Labs
/
nac3
8
0
Fork 5
Code Issues 87 Pull Requests 3 Releases Wiki Activity

Fix restoration of loop target in try statement #323

Merged
sb10q merged 1 commits from issue-301 into master 2023-09-29 06:51:50 +08:00
Conversation 5 Commits 1 Files Changed 3
derppening commented 2023-09-21 16:19:31 +08:00
Collaborator

CFG of old_loop_target:

let mut old_loop_target = None;
// ...

if has_cleanup {
	if let Some((continue_target, break_target)) = ctx.loop_target {
        let break_proxy = ctx.ctx.append_basic_block(current_fun, "try.break");
        let continue_proxy = ctx.ctx.append_basic_block(current_fun, "try.continue");
        old_loop_target = ctx.loop_target.replace((continue_proxy, break_proxy));
    }
}

// ...

ctx.loop_target = old_loop_target;
old_loop_target = None;

old_loop_target stores the previous value of ctx.loop_target only if has_cleanup && ctx.loop_target.is_some(). Therefore, ctx.loop_target should restore from old_loop_target only if ctx.loop_target was overwritten, which is indicated by old_loop_target.is_some().

Fixes #301.

CFG of `old_loop_target`: ```rust let mut old_loop_target = None; // ... if has_cleanup { if let Some((continue_target, break_target)) = ctx.loop_target { let break_proxy = ctx.ctx.append_basic_block(current_fun, "try.break"); let continue_proxy = ctx.ctx.append_basic_block(current_fun, "try.continue"); old_loop_target = ctx.loop_target.replace((continue_proxy, break_proxy)); } } // ... ctx.loop_target = old_loop_target; old_loop_target = None; ``` `old_loop_target` stores the previous value of `ctx.loop_target` only if `has_cleanup && ctx.loop_target.is_some()`. Therefore, `ctx.loop_target` should restore from `old_loop_target` only if `ctx.loop_target` was overwritten, which is indicated by `old_loop_target.is_some()`. Fixes #301.
derppening added 12 commits 2023-09-21 16:19:33 +08:00
72570fbb16 core: Fix missing changes for codegen tests 
Apparently the changes were dropped after rebasing.
20aa094b1f core: Remove emit_llvm from CodeGenLLVMOptions 
We instead output an LLVM bitcode file when the option is specified on
the command-line.
f1664e7158 core: Fix passing structure arguments to extern functions 
All parameters with a structure type in extern functions are marked as
`byref` instead of `byval`, as most ABIs require the first several
arguments to be passed in registers before spilling into the stack.

`byval` breaks this contract by explicitly requiring all arguments to be
 passed in the stack, breaking interop with libraries written in other
 languages.
e8b7d19b47 standalone: Update demos 
- Add `newline` parameter to all output_* functions
- Add `output_str` for printing a string
- Add demo_test.py to test interop
49660c4e6f core: Fix restoration of loop target in try statement 
I am not sure why this occurs either, it only appears that after the
assignment, `old_loop_target` and `loop_target` are both referencing the
same variable, causing the next line to set both variables as None.
derppening requested review from sb10q 2023-09-21 16:22:04 +08:00
derppening added a new dependency 2023-09-21 16:22:17 +08:00
#324 Meta changes from #301
derppening added a new dependency 2023-09-25 11:53:13 +08:00
#325 Meta Changes from #321
derppening force-pushed issue-301 from 49660c4e6f to e07cc64a95 2023-09-25 11:54:42 +08:00 Compare
derppening commented 2023-09-25 11:55:20 +08:00
Poster
Collaborator

v2: Rebased against meta-changes-for-issue-301

v2: Rebased against `meta-changes-for-issue-301`
sb10q reviewed 2023-09-25 15:05:09 +08:00
nac3core/src/codegen/stmt.rs Outdated
@ -790,3 +790,3 @@
ctx.unwind_target = old_unwind;
ctx.return_target = old_return;
ctx.loop_target = old_loop_target;
if let Some(old_loop_target) = old_loop_target {
sb10q commented 2023-09-25 15:05:09 +08:00
Owner

Should be marked as workaround in the code comments and corresponding Issue opened, as discussed.

Should be marked as workaround in the code comments and corresponding Issue opened, as discussed.
derppening force-pushed issue-301 from e07cc64a95 to e97ceb0da9 2023-09-25 16:36:36 +08:00 Compare
derppening commented 2023-09-25 16:42:37 +08:00
Poster
Collaborator

v3: Updated fix which better addresses the problem. Description of the Pull Request is also updated.

v3: Updated fix which better addresses the problem. Description of the Pull Request is also updated.
sb10q reviewed 2023-09-25 18:13:57 +08:00
nac3standalone/demo/demo.c Outdated
@ -0,0 +1,114 @@
#include <assert.h>
sb10q commented 2023-09-25 18:13:57 +08:00
Owner

Should not be in this PR either.

Should not be in this PR either.
sb10q reviewed 2023-09-25 18:14:17 +08:00
nac3core/src/codegen/test.rs Outdated
@ -24,6 +24,7 @@ use nac3parser::{
use parking_lot::RwLock;
use std::collections::{HashMap, HashSet};
use std::sync::Arc;
use inkwell::targets::{InitializationConfig, Target};
sb10q commented 2023-09-25 18:14:17 +08:00
Owner

Should this be here?

Should this be here?
derppening force-pushed issue-301 from e97ceb0da9 to 48c6498d1f 2023-09-28 20:00:51 +08:00 Compare
derppening commented 2023-09-28 20:01:13 +08:00
Poster
Collaborator

v4: Rebased onto meta-changes-from-issue-301.

v4: Rebased onto `meta-changes-from-issue-301`.
sb10q commented 2023-09-28 23:03:15 +08:00
Owner

Why was it broken before? I don't understand what was going on with the Option.

Why was it broken before? I don't understand what was going on with the Option.
sb10q reviewed 2023-09-28 23:03:50 +08:00
nac3standalone/demo/demo.rs
@ -98,2 +98,4 @@
}
#[no_mangle]
pub extern "C" fn __nac3_end_catch() {}
sb10q commented 2023-09-28 23:03:50 +08:00
Owner

Does this belong in this PR?

Does this belong in this PR?
derppening commented 2023-09-29 00:19:06 +08:00
Poster
Collaborator

Yes, this is necessary for loop_try_break to link, possibly due to the use of the catch block.

Yes, this is necessary for `loop_try_break` to link, possibly due to the use of the `catch` block.
derppening commented 2023-09-29 00:17:23 +08:00
Poster
Collaborator

Why was it broken before? I don't understand what was going on with the Option.

old_loop_target is only assigned if has_cleanup is true, otherwise it would be None. Later when the loop target needs to be restored, it is never checked whether old_loop_target is assigned or if has_cleanup is true, meaning that for cases where has_cleanup is false, Nonewill be unconditionally restored toctx.loop_target`, causing the original loop target to be lost forever.

> Why was it broken before? I don't understand what was going on with the Option. `old_loop_target` is only assigned if `has_cleanup` is `true`, otherwise it would be `None`. Later when the loop target needs to be restored, it is never checked whether `old_loop_target` is assigned or if `has_cleanup` is `true`, meaning that for cases where `has_cleanup` is false`, `None` will be unconditionally restored to `ctx.loop_target`, causing the original loop target to be lost forever.
sb10q merged commit 48c6498d1f into master 2023-09-29 06:51:50 +08:00
sb10q deleted branch issue-301 2023-09-29 06:51:53 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
Labels
Clear labels   
UROP0622
   
future-extension
   
high-priority
   
low-priority
No Label
UROP0622
future-extension
high-priority
low-priority
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Depends on
#325 Meta Changes from #321
M-Labs/nac3
You do not have permission to read 1 dependency
Reference: M-Labs/nac3#323
There is no content yet.
Delete Branch "issue-301"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?