M-Labs/nac3
M-Labs
/
nac3
8
0
Fork 10
Code Issues 99 Pull Requests 20 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 +38 -3
derppening commented 2023-09-21 16:19:31 +08:00
Collaborator
Copy Link

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 11 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
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
Author
Collaborator
Copy Link

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
Copy Link

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
Author
Collaborator
Copy Link

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
Copy Link

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
Copy Link

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
Author
Collaborator
Copy Link

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
Copy Link

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
Copy Link

Does this belong in this PR?

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

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
Author
Collaborator
Copy Link

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   
future-extension

   
high-priority

   
low-priority

   
UROP0622
No Label
future-extension
high-priority
low-priority
UROP0622
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
#324 Meta changes from #301
M-Labs/nac3
#325 Meta Changes from #321
M-Labs/nac3
Reference: M-Labs/nac3#323
No description provided.
Delete Branch "issue-301"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?