-
Notifications
You must be signed in to change notification settings - Fork 275
Cleanup duplicate static inline functions at/after linking #1987
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
static inline void foo(int x) | ||
{ | ||
x = x + 1; | ||
} | ||
|
||
void bar(int); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
#include "foo.h" | ||
|
||
int main(int argc, char *argv[]) | ||
{ | ||
foo(argc); | ||
bar(argc); | ||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#ifndef TEST2 | ||
# include "foo.h" | ||
#else | ||
inline void foo(int x) | ||
{ | ||
x = 0; | ||
} | ||
#endif | ||
|
||
void bar(int y) | ||
{ | ||
foo(y); | ||
y = 2; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
CORE | ||
main.c | ||
other.c --verbosity 10 | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
^Functions: 5; 5 have a body.$ | ||
-- | ||
^warning: ignoring | ||
^CONVERSION ERROR$ | ||
-- | ||
The implementation of a function named "foo" (static inline void foo(int x)) is | ||
the same in both translation units, thus linking can safely discard one of the | ||
copies and does not need to do any renaming. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
CORE | ||
main.c | ||
other.c --verbosity 10 -DTEST2 | ||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
^Functions: 6; 6 have a body.$ | ||
-- | ||
^warning: ignoring | ||
^CONVERSION ERROR$ | ||
-- | ||
The implementation of a function named "foo" (void foo(int x)) differs between | ||
translation units; as at least one of them is static this is acceptable at link | ||
time and can be resolved by renaming one of them. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
static int foo(int a) | ||
{ | ||
return a+1; | ||
return a + 2; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
static int foo(int a) | ||
{ | ||
return a+1; | ||
return a + 2; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,15 +48,45 @@ static bool link_functions( | |
goto_functionst &dest_functions, | ||
const symbol_tablet &src_symbol_table, | ||
goto_functionst &src_functions, | ||
const rename_symbolt &rename_symbol, | ||
rename_symbolt &rename_symbol, | ||
const std::unordered_set<irep_idt> &weak_symbols) | ||
{ | ||
namespacet ns(dest_symbol_table); | ||
namespacet src_ns(src_symbol_table); | ||
|
||
// remove any unnecessary duplicates | ||
std::unordered_set<irep_idt> duplicates; | ||
for(const auto &f_pair : src_functions.function_map) | ||
{ | ||
rename_symbolt::expr_mapt::iterator rename_it = | ||
rename_symbol.expr_map.find(f_pair.first); | ||
if(rename_it == rename_symbol.expr_map.end()) | ||
continue; | ||
|
||
std::string new_name = id2string(rename_it->second); | ||
std::string::size_type suffix_pos = new_name.find("$link"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this grungy string decomposition, how about annotating such symbols with a comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would seem nice, but I'm struggling to find a good place to put it in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been sticking annotations like this on the type myself. We should agree on a policy though, or make a breaking change to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think that the name and the type are really disjoint, so I'd much rather sort this out via some to-be-added |
||
if(suffix_pos == std::string::npos) | ||
continue; | ||
|
||
irep_idt original_name = new_name.substr(0, suffix_pos); | ||
goto_functionst::function_mapt::iterator existing_func = | ||
dest_functions.function_map.find(original_name); | ||
if(existing_func == dest_functions.function_map.end()) | ||
continue; | ||
|
||
if(f_pair.second.body.equals(existing_func->second.body)) | ||
{ | ||
duplicates.insert(f_pair.first); | ||
rename_symbol.expr_map.erase(rename_it); | ||
} | ||
} | ||
|
||
// merge functions | ||
for(auto &gf_entry : src_functions.function_map) | ||
{ | ||
if(duplicates.find(gf_entry.first) != duplicates.end()) | ||
continue; | ||
|
||
// the function might have been renamed | ||
rename_symbolt::expr_mapt::const_iterator e_it = | ||
rename_symbol.expr_map.find(gf_entry.first); | ||
|
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?
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 makes sure the functions are actually different so that the
static
variant doesn't get ignored after the changes in this PR.