-
Notifications
You must be signed in to change notification settings - Fork 162
Added support for computed goto extension #61
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
base: master
Are you sure you want to change the base?
Conversation
|
Is there any necessary changes in this pull request? |
src/Block.h
Outdated
|
|
||
| int block_size() { return block_size_; } | ||
| std::vector<string> addr_labels; | ||
| std::vector<string> try_only_labels; |
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.
try_only_labels is not referenced anywhere. Delete it.
| } | ||
|
|
||
| void | ||
| StatementGoto::change_label(std::vector<string> addr_labels) const{ |
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.
The function creates an alternative label for the goto statement. Rename it accordingly.
| ss<< "*target["; | ||
| ss<<index; | ||
| ss<<"]"; | ||
| other_name_for_label=""; |
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.
Too verbose. For example line 437 to 440 can be rewritten as ss << "*target[" << index << "]
| Error::set_error(SUCCESS); | ||
|
|
||
|
|
||
| if(CGOptions::computed_goto()){ |
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.
In general, we should not modify the AST nodes contained in a block once the block is fully generated. I suggest we collect addr_labels and create the alternative label based on index in the addr_labels inside StatementGoto::make_random.
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.
I collected addr_labels in StatementGoto::make_random and created alternate label for it. After adding above, I am getting error like some labels are used but not defined. Is it because the generated block is discarded?
for example - in a particular block
{
void *target[] = { &&lbl_123, &&lbl_456};
}
&&lbl_456 is alternate label when lbl_456 is generated. But it is not present in that block, which stats label used but not defined.
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.
I see your point. What about we still do a generation of the computed goto labels after a block is generated, i.e., at the same place in block.cpp, but instead of modifying the StatementGoto nodes, we put the alternative labels in an external data structure? For example, this could be an additional static map in StatemenGoto.h other than stm_labels where each target of the goto-statements is associated with an alternative label.
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.
The above patch does the same. It creates alternate labels after a block is created but do not alter current labels.
I am facing difficulty in understanding implementation of stm_labels in goto::make_random. As per my understanding, stm_labels is map in which source contains goto statement and destination contain respective label.
Can you elaborate about stm_labels?
Also, we are unsure which labels will be there in final block. Even if we created alternative label, for the computed goto extension, below approach is followed.
{
void *target[] = { &&lbl_123, &&lbl_456};
goto *target[0];
}
At the end we want label present in target array. So, basically alternate labels are stored inside addr_label of every block. Do we still need static map in StatementGoto.h ? We can use addr_label to generate goto destination in block make::random
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.
In Csmith, the make_random functions defined in various classes generate an AST node of the class. StatementGoto::make_random generates a goto-statement, and if we add alt_label as a field to StatementGoto, we expect alt_label not to be altered AFTER StatementGoto::make_random is called.
stm_labels maps all destinations of goto-statement (as a Statement*) to their labels. It's there to avoid we assign two labels to the same destination.
addr_label stores labels for a given block. The static map in StatementGoto.h would map all destinations of goto-statement (as Statement*) to their alternative labels in the whole program. This static map is not part of the StatementGoto object (the AST node), thus we are fine to modify it after the block is finalized.
| std::vector<const Variable*> init_skipped_vars; | ||
| static std::map<const Statement*, std::string> stm_labels; | ||
|
|
||
| mutable std::string other_name_for_label; |
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.
other_name_for_label no longer needs to be mutable once we adopt the approach that creates alternative label while a goto statement is being created.
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.
Without mutable keyword, it is not possible to assign constant string to other_name_for_label. Is there any other approach to remove mutable keyword?
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.
If we move the alternative label to a map like I suggested above, we don't need this field anymore.
|
Have you addressed all the comments? @shubhamnarlawar77 |
This pull request contain implementation of computed goto extension.
Command line options for computed goto are as below -
By default, this extension is disabled.