-
Notifications
You must be signed in to change notification settings - Fork 275
Enhanced local variable table #458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhanced local variable table #458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to merge the style and documentation fixes into the commits modifying the code/making the style fixes necessary?
Overall this looks good to me, except for missing tests. Is it possible to have regression tests that go with this pull request?
local_variable_table_with_holest::iterator varlimit, | ||
const address_mapt& amap, | ||
const java_cfg_dominatorst& doms) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is very long, would it be possible to factor out parts (e.g., loop bodies) into separate methods?
auto &block=get_block_for_pcrange( | ||
root, | ||
root_block, | ||
v.start_pc, | ||
v.start_pc+v.length, | ||
std::numeric_limits<unsigned>::max()); | ||
code_declt d(v.symbol_expr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the need for this line shuffling?
@@ -5,6 +5,7 @@ | |||
#include "java_types.h" | |||
|
|||
#include <climits> | |||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need iostream?
@@ -522,7 +522,7 @@ java_bytecode_convert_methodt::find_variable_for_slot( | |||
{ | |||
bool found_hole=false; | |||
for(auto &hole : var.holes) | |||
if(address>=hole.start_pc && address<(hole.start_pc-hole.length)) | |||
if(address>=hole.start_pc && address<(hole.start_pc+hole.length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So where is the change to the fixed test?
It's a real pain to merge the hunks in to the right commit-- do you know of a good way to do that automatically? (e.g. find the last commit to touch this line and squash the relevant hunk into it)? |
No, I'm afraid I don't know of an automated way of doing it - I generally end up with running git blame in one window and git add -p in the other. There's probably a much better use of your time than massaging those commits. Leave them as they are, I'd say. |
Previously one entry in the local variable table meant one declaration at runtime. We now merge live ranges when it appears they refer to the same variable due to control flow from one live range to another. Variable declarations are placed at a program point that dominates all users. This is currently unused, but will be plumbed in in a subsequent commit.
This uses the local variable table implementation introduced in the previous commit, replacing the existing simple variable table lookup code that could inadvertently split live ranges when a variable's live range occupies multiple disjoint address ranges and so is declared using multiple entries in the local variable table.
Flows from out of range or from a clashing variable declaration can happen because jsr / ret control flow is imprecisely modelled, so control flow edges are present which are in practice infeasible, hence the compiler generating code which naively appears to change a stack slot's name or type without re-initialising it. Consequently, demote these to warnings, and perhaps in the future do proper control-flow analysis to determine which callsites can really reach which jsr subroutines.
2794efd
to
e438bb3
Compare
All pre-requisites are in and rebased upon, this should now be mergeable. |
This PR replaces the existing simple local variable table code, which introduces a distinct variable per local variable table entry, with a simple control-flow analysis that merges table entries that flow into one another.
Recommend looking at each commit individually, as they introduce new functionality, use it, then factor into separate files.