Skip to content

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

Merged
merged 3 commits into from
Jun 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions regression/ansi-c/always_inline1/main.c
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
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

__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;
}
11 changes: 11 additions & 0 deletions regression/ansi-c/always_inline1/test.desc
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$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment to the test specifying why we expect an error

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
2 changes: 2 additions & 0 deletions src/ansi-c/ansi_c_convert_type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ void ansi_c_convert_typet::read_rec(const typet &type)
c_storage_spec.is_weak=true;
else if(type.id() == ID_used)
c_storage_spec.is_used = true;
else if(type.id() == ID_always_inline)
c_storage_spec.is_always_inline = true;
else if(type.id()==ID_auto)
{
// ignore
Expand Down
5 changes: 5 additions & 0 deletions src/ansi-c/ansi_c_declaration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 is_macro and things that are macros are disjoint (!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also note we have symbolt::is_function which returns false if is_macro is set!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further thought I'm inclined to say that is_macro actually is the right terminology for all the current uses: all of them (typedefs, enum values, linker aliases, always-inline functions) are names with some value (which might be code) behind them and need to be expanded in place in order to evaluate the expression or statement they are involved in. We currently consume code after the C/C++ preprocessor has been run, so those kinds of "macros" are not currently seen - but actually, if they were, they would fit the very same definition and would likely be marked as such.

}

// GCC __attribute__((__used__)) - do not treat those as file-local
Expand Down
10 changes: 10 additions & 0 deletions src/ansi-c/ansi_c_declaration.h
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,16 @@ class ansi_c_declarationt:public exprt
set(ID_is_used, is_used);
}

bool get_is_always_inline() const
{
return get_bool(ID_is_always_inline);
}

void set_is_always_inline(bool is_always_inline)
{
set(ID_is_always_inline, is_always_inline);
}

void to_symbol(
const ansi_c_declaratort &,
symbolt &symbol) const;
Expand Down
2 changes: 2 additions & 0 deletions src/ansi-c/c_storage_spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ void c_storage_spect::read(const typet &type)
is_weak=true;
else if(type.id() == ID_used)
is_used = true;
else if(type.id() == ID_always_inline)
is_always_inline = true;
else if(type.id()==ID_auto)
{
// ignore
Expand Down
9 changes: 7 additions & 2 deletions src/ansi-c/c_storage_spec.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,14 @@ class c_storage_spect
is_inline=false;
is_weak=false;
is_used = false;
is_always_inline = false;
alias.clear();
asm_label.clear();
section.clear();
}

bool is_typedef, is_extern, is_static, is_register,
is_inline, is_thread_local, is_weak, is_used;
bool is_typedef, is_extern, is_static, is_register, is_inline,
is_thread_local, is_weak, is_used, is_always_inline;

// __attribute__((alias("foo")))
irep_idt alias;
Expand All @@ -53,6 +54,7 @@ class c_storage_spect

bool operator==(const c_storage_spect &other) const
{
// clang-format off
return is_typedef==other.is_typedef &&
is_extern==other.is_extern &&
is_static==other.is_static &&
Expand All @@ -61,9 +63,11 @@ class c_storage_spect
is_inline==other.is_inline &&
is_weak==other.is_weak &&
is_used == other.is_used &&
is_always_inline == other.is_always_inline &&
alias==other.alias &&
asm_label==other.asm_label &&
section==other.section;
// clang-format on
}

bool operator!=(const c_storage_spect &other) const
Expand All @@ -81,6 +85,7 @@ class c_storage_spect
is_thread_local |=other.is_thread_local;
is_weak |=other.is_weak;
is_used |=other.is_used;
is_always_inline |= other.is_always_inline;
if(alias.empty())
alias=other.alias;
if(asm_label.empty())
Expand Down
1 change: 1 addition & 0 deletions src/ansi-c/c_typecheck_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,7 @@ void c_typecheck_baset::typecheck_declaration(
declaration.set_is_typedef(full_spec.is_typedef);
declaration.set_is_weak(full_spec.is_weak);
declaration.set_is_used(full_spec.is_used);
declaration.set_is_always_inline(full_spec.is_always_inline);

symbolt symbol;
declaration.to_symbol(*d_it, symbol);
Expand Down
88 changes: 87 additions & 1 deletion src/ansi-c/c_typecheck_expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, presumably, a macro? Or is the is_macro flag only used for functions that are not in fact macros?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, is_macro isn't ever used with (preprocessor) macros.

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 &parameters = 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 (f::1::x vs. g::1::x) they had originally? That seems right for inlining but wrong for macros?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could there be other returns? If so what happens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
3 changes: 3 additions & 0 deletions src/ansi-c/parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ extern char *yyansi_ctext;
%token TOK_GCC_ATTRIBUTE_DESTRUCTOR "destructor"
%token TOK_GCC_ATTRIBUTE_FALLTHROUGH "fallthrough"
%token TOK_GCC_ATTRIBUTE_USED "used"
%token TOK_GCC_ATTRIBUTE_ALWAYS_INLINE "always_inline"
%token TOK_GCC_LABEL "__label__"
%token TOK_MSC_ASM "__asm"
%token TOK_MSC_BASED "__based"
Expand Down Expand Up @@ -1547,6 +1548,8 @@ gcc_type_attribute:
{ $$=$1; set($$, ID_destructor); }
| TOK_GCC_ATTRIBUTE_USED
{ $$=$1; set($$, ID_used); }
| TOK_GCC_ATTRIBUTE_ALWAYS_INLINE
{ $$=$1; set($$, ID_always_inline); }
;

gcc_attribute:
Expand Down
3 changes: 3 additions & 0 deletions src/ansi-c/scanner.l
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,9 @@ __decltype { if(PARSER.cpp98 &&
"used" |
"__used__" { BEGIN(GCC_ATTRIBUTE3); loc(); return TOK_GCC_ATTRIBUTE_USED; }

"always_inline" |
"__always_inline__" { BEGIN(GCC_ATTRIBUTE3); loc(); return TOK_GCC_ATTRIBUTE_ALWAYS_INLINE; }

{ws} { /* ignore */ }
{newline} { /* ignore */ }
{identifier} { BEGIN(GCC_ATTRIBUTE4); }
Expand Down
2 changes: 1 addition & 1 deletion src/linking/remove_internal_symbols.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ void remove_internal_symbols(
is_file_local=false;
}

if(is_type)
if(is_type || symbol.is_macro)
{
// never EXPORTED by itself
}
Expand Down
2 changes: 2 additions & 0 deletions src/util/irep_ids.def
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,8 @@ IREP_ID_TWO(C_abstract, #abstract)
IREP_ID_ONE(synthetic)
IREP_ID_ONE(interface)
IREP_ID_TWO(C_must_not_throw, #must_not_throw)
IREP_ID_ONE(always_inline)
IREP_ID_ONE(is_always_inline)

// Projects depending on this code base that wish to extend the list of
// available ids should provide a file local_irep_ids.h in their source tree and
Expand Down
10 changes: 10 additions & 0 deletions src/util/replace_symbol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ bool replace_symbolt::replace(
if(c_sizeof_type.is_not_nil() && have_to_replace(c_sizeof_type))
result &= replace(static_cast<typet&>(dest.add(ID_C_c_sizeof_type)));

const typet &type_arg = static_cast<const typet &>(dest.find(ID_type_arg));
if(type_arg.is_not_nil() && have_to_replace(type_arg))
result &= replace(static_cast<typet &>(dest.add(ID_type_arg)));

const typet &va_arg_type =
static_cast<const typet&>(dest.find(ID_C_va_arg_type));
if(va_arg_type.is_not_nil() && have_to_replace(va_arg_type))
Expand Down Expand Up @@ -136,6 +140,12 @@ bool replace_symbolt::have_to_replace(const exprt &dest) const
if(have_to_replace(static_cast<const typet &>(c_sizeof_type)))
return true;

const irept &type_arg = dest.find(ID_type_arg);

if(type_arg.is_not_nil())
if(have_to_replace(static_cast<const typet &>(type_arg)))
return true;

const irept &va_arg_type=dest.find(ID_C_va_arg_type);

if(va_arg_type.is_not_nil())
Expand Down