-
Notifications
You must be signed in to change notification settings - Fork 273
Fully process always_inline (second attempt) #1898
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,40 @@ | ||
// this is a GCC extension | ||
|
||
#ifdef __GNUC__ | ||
static inline __attribute__((always_inline)) _Bool | ||
__is_kfree_rcu_offset(unsigned long offset) | ||
{ | ||
return offset < 4096; | ||
} | ||
|
||
static inline __attribute__((always_inline)) void | ||
kfree_rcu(unsigned long offset) | ||
{ | ||
// immediate use of a constant as argument to __builtin_constant_p | ||
((void)sizeof(char[1 - 2 * !!(!__builtin_constant_p(offset))])); | ||
// inlining required to turn the array size into a compile-time constant | ||
((void)sizeof(char[1 - 2 * !!(!__is_kfree_rcu_offset(offset))])); | ||
} | ||
|
||
static inline __attribute__((always_inline)) void unused(unsigned long offset) | ||
{ | ||
// this would not be constant as it's never used, the front-end needs to | ||
// discard it | ||
((void)sizeof(char[1 - 2 * !!(!__builtin_constant_p(offset))])); | ||
} | ||
|
||
// unused, but no 'static' | ||
inline __attribute__((__always_inline__)) int also_unused(int _c) | ||
{ | ||
return _c; | ||
} | ||
#endif | ||
|
||
int main() | ||
{ | ||
#ifdef __GNUC__ | ||
kfree_rcu(42); | ||
#endif | ||
|
||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
CORE | ||
main.c | ||
|
||
^EXIT=0$ | ||
^SIGNAL=0$ | ||
-- | ||
^warning: ignoring | ||
^CONVERSION ERROR$ | ||
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. Add a comment to the test specifying why we expect an error 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. Done. |
||
-- | ||
The static asserts (arrays that may have a negative size if the assertion fails) | ||
can only be evaluated if always_inline is correctly applied. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,8 @@ void ansi_c_declarationt::output(std::ostream &out) const | |
out << " is_extern"; | ||
if(get_is_static_assert()) | ||
out << " is_static_assert"; | ||
if(get_is_always_inline()) | ||
out << " is_always_inline"; | ||
out << "\n"; | ||
|
||
out << "Type: " << type().pretty() << "\n"; | ||
|
@@ -164,6 +166,9 @@ void ansi_c_declarationt::to_symbol( | |
symbol.is_extern=false; | ||
else if(get_is_extern()) // traditional GCC | ||
symbol.is_file_local=true; | ||
|
||
if(get_is_always_inline()) | ||
symbol.is_macro = true; | ||
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. Can we please rename this then? It's really confusing that things annotated 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 also note we have 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. Upon further thought I'm inclined to say that |
||
} | ||
|
||
// GCC __attribute__((__used__)) - do not treat those as file-local | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,11 @@ Author: Daniel Kroening, [email protected] | |
#include <util/c_types.h> | ||
#include <util/config.h> | ||
#include <util/cprover_prefix.h> | ||
#include <util/expr_util.h> | ||
#include <util/ieee_float.h> | ||
#include <util/pointer_offset_size.h> | ||
#include <util/pointer_predicates.h> | ||
#include <util/replace_symbol.h> | ||
#include <util/simplify_expr.h> | ||
#include <util/string_constant.h> | ||
|
||
|
@@ -1918,7 +1920,10 @@ void c_typecheck_baset::typecheck_side_effect_function_call( | |
if(entry!=asm_label_map.end()) | ||
identifier=entry->second; | ||
|
||
if(symbol_table.symbols.find(identifier)==symbol_table.symbols.end()) | ||
symbol_tablet::symbolst::const_iterator sym_entry = | ||
symbol_table.symbols.find(identifier); | ||
|
||
if(sym_entry == symbol_table.symbols.end()) | ||
{ | ||
// This is an undeclared function. | ||
// Is this a builtin? | ||
|
@@ -1960,6 +1965,87 @@ void c_typecheck_baset::typecheck_side_effect_function_call( | |
warning() << "function `" << identifier << "' is not declared" << eom; | ||
} | ||
} | ||
else if( | ||
sym_entry->second.type.get_bool(ID_C_inlined) && | ||
sym_entry->second.is_macro && sym_entry->second.value.is_not_nil()) | ||
{ | ||
// calling a function marked as always_inline | ||
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. Or, presumably, a macro? Or is the 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. Yes, |
||
const symbolt &func_sym = sym_entry->second; | ||
const code_typet &func_type = to_code_type(func_sym.type); | ||
|
||
replace_symbolt replace; | ||
|
||
const code_typet::parameterst ¶meters = func_type.parameters(); | ||
auto p_it = parameters.begin(); | ||
for(const auto &arg : expr.arguments()) | ||
{ | ||
if(p_it == parameters.end()) | ||
{ | ||
// we don't support varargs with always_inline | ||
err_location(f_op); | ||
error() << "function call has additional arguments, " | ||
<< "cannot apply always_inline" << eom; | ||
throw 0; | ||
} | ||
|
||
irep_idt p_id = p_it->get_identifier(); | ||
if(p_id.empty()) | ||
{ | ||
p_id = id2string(func_sym.base_name) + "::" + | ||
id2string(p_it->get_base_name()); | ||
} | ||
replace.insert(p_id, arg); | ||
|
||
++p_it; | ||
} | ||
|
||
if(p_it != parameters.end()) | ||
{ | ||
err_location(f_op); | ||
error() << "function call has missing arguments, " | ||
<< "cannot apply always_inline" << eom; | ||
throw 0; | ||
} | ||
|
||
codet body = to_code(func_sym.value); | ||
replace(body); | ||
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. So this will replace formal parameters with actuals, but not actually cause locals to alias because they retain the qualified names ( 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. As said above, we don't actually do preprocessor macros in this way, so it should be fine. |
||
|
||
side_effect_exprt side_effect_expr( | ||
ID_statement_expression, func_type.return_type()); | ||
body.make_block(); | ||
|
||
// simulates parts of typecheck_function_body | ||
typet cur_return_type = return_type; | ||
return_type = func_type.return_type(); | ||
typecheck_code(body); | ||
return_type.swap(cur_return_type); | ||
|
||
// replace final return by an ID_expression | ||
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. Could there be other returns? If so what happens? 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. It's unsupported - now it's being tested for and reported. |
||
codet &last = to_code_block(body).find_last_statement(); | ||
|
||
if(last.get_statement() == ID_return) | ||
last.set_statement(ID_expression); | ||
|
||
// NOLINTNEXTLINE(whitespace/braces) | ||
const bool has_returns = has_subexpr(body, [&](const exprt &e) { | ||
return e.id() == ID_code && to_code(e).get_statement() == ID_return; | ||
}); | ||
if(has_returns) | ||
{ | ||
// we don't support multiple return statements with always_inline | ||
err_location(last); | ||
error() << "function has multiple return statements, " | ||
<< "cannot apply always_inline" << eom; | ||
throw 0; | ||
} | ||
|
||
side_effect_expr.copy_to_operands(body); | ||
typecheck_side_effect_statement_expression(side_effect_expr); | ||
|
||
expr.swap(side_effect_expr); | ||
|
||
return; | ||
} | ||
} | ||
|
||
// typecheck it now | ||
|
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.
Could you minimise this test-case so it's clearer which parts are crucial to the test and which is unrelated stuff the kernel's RCU code happens to do? If you want to keep the real-world example you could include this as a block comment, then use a simplified version for the actual test?
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.
Actually everything is necessary here, but I'll add comments to explain why/what it is testing.