gccrs: Add straight-line BIR drop style analysis - #4730
Conversation
Create the requested number of elements in the IndexVec constructor instead of only reserving space. This fixes an out-of-bounds access when dumping BIR. gcc/rust/ChangeLog: * checks/errors/borrowck/rust-bir-place.h (IndexVec::IndexVec): Create the requested number of elements. Signed-off-by: Lishin <lishin1008@gmail.com>
Add BIR Drop statements at scope exits and classify them as static or dead by tracking whether each local is initialized or moved. Treat function arguments as initialized at function entry and schedule their value drops before returning. Arguments do not receive StorageLive or StorageDead statements. Dump the classification and add tests for static local, whole-local move, a copy, and a function argument. gcc/rust/ChangeLog: * Make-lang.in: Add rust-bir-drop-analysis.o. * checks/errors/borrowck/rust-bir.h: (Statement::DropStyle): New enum. (Statement::Kind): Add DROP. (Statement::make_drop): New function. (Statement::get_drop_style): Likewise. (Statement::set_drop_style): Likewise. * checks/errors/borrowck/rust-bir-builder-internal.h: (AbstractBuilder::declare_argument): New function. (AbstractBuilder::push_drop): Likewise. (AbstractBuilder::push_function_argument_drops): Likewise. (AbstractBuilder::pop_scope): Schedule Drop statements. (AbstractBuilder::unwind_until): Likewise. (AbstractBuilder::push_return): Schedule function argument drops. * checks/errors/borrowck/rust-bir-builder.h (Builder::handle_param): Use declare_argument for function parameters. * checks/errors/borrowck/rust-bir-dump.cc (Dump::visit): Dump Drop classifications. * checks/errors/borrowck/rust-bir-fact-collector.h (FactCollector::visit): Handle Drop statements. * checks/errors/borrowck/rust-borrow-checker.cc (BorrowChecker::go): Run BIR Drop analysis. * checks/errors/borrowck/rust-bir-drop-analysis.cc: New file. * checks/errors/borrowck/rust-bir-drop-analysis.h: New file. gcc/testsuite/ChangeLog: * rust/borrowck/drop_analysis_whole_move.rs: New test. Signed-off-by: Lishin <lishin1008@gmail.com>
| if (block.successors.empty ()) | ||
| break; | ||
|
|
||
| if (block.successors.size () != 1) |
There was a problem hiding this comment.
I limit this first version to straight-line control flow.
If the CFG contains branches or loops, the analysis returns and leaves the Drops unclassified.
| if (expr.get_kind () == ExprKind::ASSIGNMENT) | ||
| { | ||
| PlaceId rhs = static_cast<Assignment &> (expr).get_rhs (); | ||
| const Place &rhs_place = function.place_db[rhs]; | ||
|
|
||
| if (rhs_place.kind == Place::VARIABLE | ||
| && rhs_place.should_be_moved ()) | ||
| initialized[rhs.value] = false; | ||
| } | ||
|
|
||
| initialized[lhs.value] = true; |
There was a problem hiding this comment.
I mark the lhs as initialized after an assignment.
I also mark a non-Copy variable rhs as uninitialized because its value was moved, and leave a copy assignment rhs initialized.
This first version only handles direct whole-local moves.
| public: | ||
| IndexVec () = default; | ||
| IndexVec (size_t size) { internal_vector.reserve (size); } | ||
| IndexVec (size_t size) : internal_vector (size) {} |
There was a problem hiding this comment.
I changed the sized constructor to create the requested number of elements.
The old constructor only reserved capacity, so direct indexing during the BIR dump could access an empty vector.
There was a problem hiding this comment.
I would add a comment on this line. This vector ctor is a bit misleading/tricky (many people think this only reserves the space) and it will either turn a warning in reader's mind or be misused. We need to be explicit about the elements default construction.
There was a problem hiding this comment.
Thanks! I added a comment to make that clear.
| for (PlaceId argument : function.arguments) | ||
| initialized[argument.value] = true; |
There was a problem hiding this comment.
I handle function argument Drops separately because arguments are already
initialized at function entry and do not use the normal local StorageLive
and StorageDead handling.
I emit their Drops in reverse declaration order before the function returns.
P-E-P
left a comment
There was a problem hiding this comment.
I'm not entirely familiar with BIR but this looks great. I've put a few suggestions.
| public: | ||
| IndexVec () = default; | ||
| IndexVec (size_t size) { internal_vector.reserve (size); } | ||
| IndexVec (size_t size) : internal_vector (size) {} |
There was a problem hiding this comment.
I would add a comment on this line. This vector ctor is a bit misleading/tricky (many people think this only reserves the space) and it will either turn a warning in reader's mind or be misused. We need to be explicit about the elements default construction.
gcc/rust/ChangeLog: * checks/errors/borrowck/rust-bir-drop-analysis.cc (BasicBlockIdHash): New helper. (DropAnalysis::analyze): Use unordered_set for visited block tracking. * checks/errors/borrowck/rust-bir-drop-analysis.h: Include rust-bir.h directly and remove Function forward declaration. * checks/errors/borrowck/rust-bir-place.h (IndexVec::IndexVec): Add comment for sized constructor. Signed-off-by: Lishin <lishin1008@gmail.com>
This patch adds an initial BIR Drop state analysis based on rustc's MIR
drop scheduling and Drop style classification.
It contains two commits:
commit 1
Fixes an incorrect BIR
IndexVecsize that could cause an out-of-boundsaccess when dumping BIR.
commit 2
Adds basic Drop state analysis for whole local variables in straight-line control flow.
It adds Drop statements at scope exits and marks them as
StaticorDead.It covers these straight-line cases:
Current scope
This patch supports:
copyversusmoveassignmentsStaticandDeadDrop stylesThe following are left for follow-up patches:
needs_dropfilteringBIR Drop statements are currently emitted for all locals as analysis points.
Filtering them using
needs_dropwill be added separately.