Rearchitect var args support (making it actually work)

Previously all the logic to access values of variable argument lists was put
into our va_arg() implementation. This required guessing what the parameters
could have been, and did not take into account the initial value passed to
va_start.

Now va_start triggers constructing an array of pointers to parameters, the first
of which is the original argument to va_start. The va_list is then just a
pointer to the first element of the array. va_arg just increments the pointer.
goto_program conversion places a side_effect_exprt where va_start was, and
goto-symex populates the array with the actual values applicable for a
particular invocation of the function.

Author: tautschnig

regression/cbmc/Variadic1/test.desc

@@ -1,4 +1,4 @@
-FUTURE
+CORE
 main.c
 
 ^EXIT=0$

regression/cbmc/va_list3/test.desc

@@ -1,4 +1,4 @@
-KNOWNBUG
+CORE
 main.c
 
 ^EXIT=0$

regression/cbmc/va_list3/windows-preprocessed.desc

@@ -1,4 +1,4 @@
-KNOWNBUG
+CORE
 windows-preprocessed.i
 --winx64
 ^EXIT=0$

src/ansi-c/expr2c.cpp

@@ -3664,8 +3664,8 @@ std::string expr2ct::convert_with_precedence(
       return convert_prob_uniform(src, precedence=16);
     else if(statement==ID_statement_expression)
       return convert_statement_expression(src, precedence=15);
-    else if(statement==ID_gcc_builtin_va_arg_next)
-      return convert_function(src, "gcc_builtin_va_arg_next", precedence=16);
+    else if(statement == ID_va_start)
+      return convert_function(src, CPROVER_PREFIX "va_start", precedence = 16);
     else
       return convert_norep(src, precedence);
   }

src/goto-instrument/goto_program2code.cpp

@@ -105,9 +105,10 @@ void goto_program2codet::scan_for_varargs()
       const exprt &l = target->get_assign().lhs();
       const exprt &r = target->get_assign().rhs();
 
-      // find va_arg_next
-      if(r.id()==ID_side_effect &&
-         to_side_effect_expr(r).get_statement()==ID_gcc_builtin_va_arg_next)
+      // find va_start
+      if(
+        r.id() == ID_side_effect &&
+        to_side_effect_expr(r).get_statement() == ID_va_start)
       {
         assert(r.has_operands());
         va_list_expr.insert(r.op0());
@@ -309,16 +310,17 @@ goto_programt::const_targett goto_program2codet::convert_assign_varargs(
 
     dest.add(std::move(f));
   }
-  else if(r.id()==ID_address_of)
+  else if(
+    r.id() == ID_side_effect &&
+    to_side_effect_expr(r).get_statement() == ID_va_start)
   {
     code_function_callt f(
-      symbol_exprt("va_start", code_typet({}, empty_typet())),
-      {this_va_list_expr, to_address_of_expr(r).object()});
+      symbol_exprt(ID_va_start, code_typet({}, empty_typet())),
+      {this_va_list_expr, to_address_of_expr(skip_typecast(r.op0())).object()});
 
     dest.add(std::move(f));
   }
-  else if(r.id()==ID_side_effect &&
-          to_side_effect_expr(r).get_statement()==ID_gcc_builtin_va_arg_next)
+  else if(r.id() == ID_plus)
   {
     code_function_callt f(
       symbol_exprt("va_arg", code_typet({}, empty_typet())),

src/goto-programs/builtin_functions.cpp

@@ -1085,10 +1085,10 @@ void goto_convertt::do_function_call_symbol(
   else if(identifier==ID_gcc_builtin_va_arg)
   {
     // This does two things.
-    // 1) Move list pointer to next argument.
-    //    Done by gcc_builtin_va_arg_next.
-    // 2) Return value of argument.
+    // 1) Return value of argument.
     //    This is just dereferencing.
+    // 2) Move list pointer to next argument.
+    //    This is just an increment.
 
     if(arguments.size()!=1)
     {
@@ -1100,27 +1100,21 @@ void goto_convertt::do_function_call_symbol(
 
     exprt list_arg=make_va_list(arguments[0]);
 
-    {
-      side_effect_exprt rhs(
-        ID_gcc_builtin_va_arg_next,
-        list_arg.type(),
-        function.source_location());
-      rhs.copy_to_operands(list_arg);
-      rhs.set(ID_C_va_arg_type, to_code_type(function.type()).return_type());
-      goto_programt::targett t1=dest.add_instruction(ASSIGN);
-      t1->source_location=function.source_location();
-      t1->code=code_assignt(list_arg, rhs);
-    }
-
     if(lhs.is_not_nil())
     {
       typet t=pointer_type(lhs.type());
-      dereference_exprt rhs(typecast_exprt(list_arg, t), lhs.type());
+      dereference_exprt rhs{typecast_exprt(dereference_exprt{list_arg}, t)};
       rhs.add_source_location()=function.source_location();
-      goto_programt::targett t2=dest.add_instruction(ASSIGN);
-      t2->source_location=function.source_location();
-      t2->code=code_assignt(lhs, rhs);
+      dest.add(goto_programt::make_assignment(
+        lhs, std::move(rhs), function.source_location()));
     }
+
+    code_assignt assign{
+      list_arg, plus_exprt{list_arg, from_integer(1, pointer_diff_type())}};
+    assign.rhs().set(
+      ID_C_va_arg_type, to_code_type(function.type()).return_type());
+    dest.add(goto_programt::make_assignment(
+      std::move(assign), function.source_location()));
   }
   else if(identifier=="__builtin_va_copy")
   {
@@ -1146,7 +1140,7 @@ void goto_convertt::do_function_call_symbol(
     t->source_location=function.source_location();
     t->code=code_assignt(dest_expr, src_expr);
   }
-  else if(identifier=="__builtin_va_start")
+  else if(identifier == "__builtin_va_start" || identifier == "__va_start")
   {
     // Set the list argument to be the address of the
     // parameter argument.
@@ -1159,8 +1153,6 @@ void goto_convertt::do_function_call_symbol(
     }
 
     exprt dest_expr=make_va_list(arguments[0]);
-    const typecast_exprt src_expr(
-      address_of_exprt(arguments[1]), dest_expr.type());
 
     if(!is_lvalue(dest_expr))
     {
@@ -1169,9 +1161,13 @@ void goto_convertt::do_function_call_symbol(
       throw 0;
     }
 
-    goto_programt::targett t=dest.add_instruction(ASSIGN);
-    t->source_location=function.source_location();
-    t->code=code_assignt(dest_expr, src_expr);
+    side_effect_exprt rhs{
+      ID_va_start, dest_expr.type(), function.source_location()};
+    rhs.add_to_operands(
+      typecast_exprt{address_of_exprt{arguments[1]}, dest_expr.type()});
+
+    dest.add(goto_programt::make_assignment(
+      std::move(dest_expr), std::move(rhs), function.source_location()));
   }
   else if(identifier=="__builtin_va_end")
   {

src/goto-symex/goto_symex.h

@@ -374,8 +374,8 @@ class goto_symext
   /// \return The resulting expression
   static exprt add_to_lhs(const exprt &lhs, const exprt &what);
 
-  virtual void symex_gcc_builtin_va_arg_next(
-    statet &, const exprt &lhs, const side_effect_exprt &);
+  virtual void
+  symex_va_start(statet &, const exprt &lhs, const side_effect_exprt &);
   virtual void symex_allocate(
     statet &, const exprt &lhs, const side_effect_exprt &);
   virtual void symex_cpp_delete(statet &, const codet &);

src/goto-symex/goto_symex_state.h

@@ -90,6 +90,7 @@ struct framet
   irep_idt function_identifier;
   std::map<goto_programt::const_targett, goto_state_listt> goto_state_map;
   symex_targett::sourcet calling_location;
+  std::vector<irep_idt> parameter_names;
 
   goto_programt::const_targett end_of_function;
   exprt return_value = nil_exprt();

src/goto-symex/symex_assign.cpp

@@ -51,8 +51,8 @@ void goto_symext::symex_assign(
       PRECONDITION(lhs.is_nil());
       symex_printf(state, side_effect_expr);
     }
-    else if(statement==ID_gcc_builtin_va_arg_next)
-      symex_gcc_builtin_va_arg_next(state, lhs, side_effect_expr);
+    else if(statement == ID_va_start)
+      symex_va_start(state, lhs, side_effect_expr);
     else
       UNREACHABLE;
   }

src/goto-symex/symex_builtin_functions.cpp

@@ -14,6 +14,8 @@ Author: Daniel Kroening, [email protected]
 #include <util/arith_tools.h>
 #include <util/c_types.h>
 #include <util/expr_initializer.h>
+#include <util/expr_util.h>
+#include <util/fresh_symbol.h>
 #include <util/invariant_utils.h>
 #include <util/optional.h>
 #include <util/pointer_offset_size.h>
@@ -203,24 +205,51 @@ void goto_symext::symex_allocate(
     code_assignt(lhs, typecast_exprt::conditional_cast(rhs, lhs.type())));
 }
 
-irep_idt get_symbol(const exprt &src)
+/// Construct an entry for the var args array. Visual Studio complicates this as
+/// we need to put immediate values or pointers in there, depending on the size
+/// of the parameter.
+static exprt va_list_entry(
+  const irep_idt &parameter,
+  const pointer_typet &lhs_type,
+  const namespacet &ns)
 {
-  if(src.id()==ID_typecast)
-    return get_symbol(to_typecast_expr(src).op());
-  else if(src.id()==ID_address_of)
+  static const pointer_typet void_ptr_type = pointer_type(empty_typet{});
+
+  symbol_exprt parameter_expr = ns.lookup(parameter).symbol_expr();
+
+  // Visual Studio has va_list == char* and stores parameters of size no
+  // greater than the size of a pointer as immediate values
+  if(lhs_type.subtype().id() != ID_pointer)
   {
-    exprt op=to_address_of_expr(src).object();
-    if(op.id()==ID_symbol &&
-       op.get_bool(ID_C_SSA_symbol))
-      return to_ssa_expr(op).get_object_name();
-    else
-      return irep_idt();
+    exprt parameter_size = size_of_expr(parameter_expr.type(), ns);
+    CHECK_RETURN(parameter_size.is_not_nil());
+
+    binary_predicate_exprt fits_slot{
+      parameter_size,
+      ID_le,
+      from_integer(lhs_type.get_width(), parameter_size.type())};
+
+    // our back-ends don't do type casts from float to pointers, so put an
+    // unsignedbv in the middle
+    exprt float_cast = parameter_expr;
+    if(parameter_expr.type().id() == ID_floatbv)
+      float_cast =
+        typecast_exprt{parameter_expr, unsignedbv_typet{lhs_type.get_width()}};
+
+    return if_exprt{
+      fits_slot,
+      typecast_exprt::conditional_cast(float_cast, void_ptr_type),
+      typecast_exprt::conditional_cast(
+        address_of_exprt{std::move(parameter_expr)}, void_ptr_type)};
   }
   else
-    return irep_idt();
+  {
+    return typecast_exprt::conditional_cast(
+      address_of_exprt{std::move(parameter_expr)}, void_ptr_type);
+  }
 }
 
-void goto_symext::symex_gcc_builtin_va_arg_next(
+void goto_symext::symex_va_start(
   statet &state,
   const exprt &lhs,
   const side_effect_exprt &code)
@@ -230,42 +259,51 @@ void goto_symext::symex_gcc_builtin_va_arg_next(
   if(lhs.is_nil())
     return; // ignore
 
-  exprt tmp=code.op0();
-  state.rename(tmp, ns); // to allow constant propagation
-  do_simplify(tmp);
-  irep_idt id=get_symbol(tmp);
-
-  const auto zero = zero_initializer(lhs.type(), code.source_location(), ns);
-  CHECK_RETURN(zero.has_value());
-  exprt rhs(*zero);
+  // create an array holding pointers to the parameters, starting after the
+  // parameter that the operand points to
+  const exprt &op = skip_typecast(code.op0());
+  // this must be the address of a symbol
+  const irep_idt start_parameter =
+    to_ssa_expr(to_address_of_expr(op).object()).get_object_name();
 
-  if(!id.empty())
+  exprt::operandst va_arg_operands;
+  bool parameter_id_found = false;
+  for(auto &parameter : state.top().parameter_names)
   {
-    // strip last name off id to get function name
-    std::size_t pos=id2string(id).rfind("::");
-    if(pos!=std::string::npos)
+    if(!parameter_id_found)
     {
-      irep_idt function_identifier=std::string(id2string(id), 0, pos);
-      std::string base=id2string(function_identifier)+"::va_arg";
-
-      if(has_prefix(id2string(id), base))
-        id=base+std::to_string(
-          safe_string2unsigned(
-            std::string(id2string(id), base.size(), std::string::npos))+1);
-      else
-        id=base+"0";
-
-      const symbolt *symbol;
-      if(!ns.lookup(id, symbol))
-      {
-        const symbol_exprt symbol_expr(symbol->name, symbol->type);
-        rhs = typecast_exprt::conditional_cast(
-          address_of_exprt(symbol_expr), lhs.type());
-      }
+      parameter_id_found = parameter == start_parameter;
+      continue;
     }
-  }
 
-  symex_assign(state, code_assignt(lhs, rhs));
+    va_arg_operands.push_back(
+      va_list_entry(parameter, to_pointer_type(lhs.type()), ns));
+  }
+  const std::size_t va_arg_size = va_arg_operands.size();
+  array_exprt array{std::move(va_arg_operands),
+                    array_typet{pointer_type(empty_typet{}),
+                                from_integer(va_arg_size, size_type())}};
+
+  symbolt &va_array = get_fresh_aux_symbol(
+    array.type(),
+    id2string(state.source.function_id),
+    "va_args",
+    state.source.pc->source_location,
+    ns.lookup(state.source.function_id).mode,
+    state.symbol_table);
+  va_array.value = array;
+
+  clean_expr(array, state, false);
+  state.rename(array, ns);
+  do_simplify(array);
+  symex_assign(state, code_assignt{va_array.symbol_expr(), std::move(array)});
+
+  address_of_exprt rhs{
+    index_exprt{va_array.symbol_expr(), from_integer(0, index_type())}};
+  state.rename(rhs, ns);
+  symex_assign(
+    state,
+    code_assignt{lhs, typecast_exprt::conditional_cast(rhs, lhs.type())});
 }
 
 irep_idt get_string_argument_rec(const exprt &src)

src/goto-symex/symex_function_call.cpp

@@ -16,6 +16,7 @@ Author: Daniel Kroening, [email protected]
 #include <util/byte_operators.h>
 #include <util/c_types.h>
 #include <util/exception_utils.h>
+#include <util/fresh_symbol.h>
 #include <util/invariant.h>
 
 static void locality(
@@ -60,6 +61,7 @@ void goto_symext::parameter_assignments(
     INVARIANT(
       !identifier.empty(),
       "function pointer parameter must have an identifier");
+    state.top().parameter_names.push_back(identifier);
 
     const symbolt &symbol=ns.lookup(identifier);
     symbol_exprt lhs=symbol.symbol_expr();
@@ -156,30 +158,20 @@ void goto_symext::parameter_assignments(
   if(function_type.has_ellipsis())
   {
     // These are va_arg arguments; their types may differ from call to call
-    std::size_t va_count=0;
-    const symbolt *va_sym=nullptr;
-    while(!ns.lookup(
-        id2string(function_identifier)+"::va_arg"+std::to_string(va_count),
-        va_sym))
-      ++va_count;
-
-    for( ; it1!=arguments.end(); it1++, va_count++)
+    for(; it1 != arguments.end(); it1++)
     {
-      irep_idt id=
-        id2string(function_identifier)+"::va_arg"+std::to_string(va_count);
-
-      // add to symbol table
-      symbolt symbol;
-      symbol.name=id;
-      symbol.base_name="va_arg"+std::to_string(va_count);
-      symbol.mode=ID_C;
-      symbol.type=it1->type();
-
-      state.symbol_table.insert(std::move(symbol));
-
-      symbol_exprt lhs=symbol_exprt(id, it1->type());
-
-      symex_assign(state, code_assignt(lhs, *it1));
+      symbolt &va_arg = get_fresh_aux_symbol(
+        it1->type(),
+        id2string(function_identifier),
+        "va_arg",
+        state.source.pc->source_location,
+        ns.lookup(function_identifier).mode,
+        state.symbol_table);
+      va_arg.is_parameter = true;
+
+      state.top().parameter_names.push_back(va_arg.name);
+
+      symex_assign(state, code_assignt{va_arg.symbol_expr(), *it1});
     }
   }
   else if(it1!=arguments.end())