Updates requested in the review. Mainly:

    - rename collect_lvsa_access_paths_ex -> collect_lvsa_access_paths_extended
    - added Doxygen comments to introduced functions
    - restructured dereference_if_pointer function.

Author: marek-trtik

src/taint-analysis/taint_summary.cpp

@@ -358,21 +358,32 @@ symbol_exprt find_or_create_symbol_expr(
   return symbol_ptr->symbol_expr();
 }
 
+/// \brief If the passed expression in a non-null pointer expression, then
+/// the dereference expression is returned. Otherwise the original expression
+/// is returned.
+/// \param expr: The expression to be dereferenced, if it is non-null pointer.
+/// \return See 'brief' section.
 exprt dereference_if_pointer(exprt expr)
 {
-  if(expr.type().id() == ID_pointer)
-  {
-    if(expr.id() == ID_typecast)
-      return dereference_if_pointer(expr.op0());
-    // Based on experimentation, it looks like the similar IDs:
-    //    ID_nullptr, ID_null, and ID_null_object
-    // are not used in this context. I.e. ID_NULL suffices for the check.
-    if(!(expr.is_constant() && to_constant_expr(expr).get_value() == ID_NULL))
-      return dereference_exprt(expr);
-  }
+  if(expr.type().id() != ID_pointer)
+    return expr;
+  if(expr.id() == ID_typecast)
+    return dereference_if_pointer(expr.op0());
+  // Based on experimentation, it looks like the similar IDs:
+  //    ID_nullptr, ID_null, and ID_null_object
+  // are not used in this context. I.e. ID_NULL suffices for the check.
+  if(!(expr.is_constant() && to_constant_expr(expr).get_value() == ID_NULL))
+    return dereference_exprt(expr);
   return expr;
 }
 
+/// \brief Checks whether the passed expression represents an up cast to a
+/// parent.
+/// \param expr: The examined expression.
+/// \param hierarchy: The class heierarchy used for checking parent-child
+/// relation.
+/// \param ns: A namespace; the 'follow' method is needed.
+/// \return True, if 'expr' is an up cast, and false otherwise.
 bool is_parent_member_access(
   const member_exprt &expr,
   const class_hierarchyt &hierarchy,
@@ -384,8 +395,7 @@ bool is_parent_member_access(
   if(parent_type.id() != ID_struct)
     return false;
   const typet &child_type = ns.follow(expr.compound().type());
-  if(child_type.id() != ID_struct)
-    return false;
+  INVARIANT(child_type.id() == ID_struct, "Compound of member_exprt is struct");
   const std::string parent_name =
     "java::" + as_string(to_struct_type(parent_type).get_tag());
   const std::string child_name =
@@ -399,8 +409,23 @@ bool is_parent_member_access(
   return false;
 }
 
+/// \brief An extended version of the original LVSA utility function
+/// 'collect_lvsa_access_paths'. Namely, when the original function
+/// returns an empty set, then the extended one introduces a fresh
+/// EVS object for the passed (and dereferenced) expression. For non-empy
+/// output from the original function it iterates over all retrieved
+/// numbers and for each number representing an upcast (member_exprt) it calls
+/// itself in order to retrieve numbers also to parent accesses. This is because
+/// an upcast is represented in LVSA by one number and we also need LVSA
+/// number(s) of the actual parent object.
+/// \param expr: The expression whose dereference is passed to LVSA.
+/// \param it: The current program location (instruction iteratior)
+/// \param lvsa: The LVSA analysis.
+/// \param result: The resulting points-to set.
+/// \return True, if the resulting points-to set is definitelly singular, and
+/// false otherwise.
 bool taint_algorithm_computing_summary_of_functiont::
-  collect_lvsa_access_paths_ex(
+  collect_lvsa_access_paths_extended(
     const exprt &expr,
     const instruction_iteratort &it,
     local_value_set_analysist &lvsa,
@@ -423,7 +448,7 @@ bool taint_algorithm_computing_summary_of_functiont::
   {
     external_value_set_exprt new_lhs(
       query_expr.type(),
-      constant_exprt("external_objects", string_typet()),
+      constant_exprt("", string_typet()),
       external_value_set_typet::ROOT_OBJECT,
       false);
     // Use access path "[]" to dereference the pointer
@@ -446,9 +471,11 @@ bool taint_algorithm_computing_summary_of_functiont::
           program->get_namespace()))
       {
         if(
-          !collect_lvsa_access_paths_ex(
+          !collect_lvsa_access_paths_extended(
             member_expr->compound(), it, lvsa, result))
+        {
           singular = false;
+        }
       }
     }
     result.insert(raw_number);
@@ -629,7 +656,7 @@ void  taint_algorithm_computing_summary_of_functiont::
 
       assert(param_idx<fn_call.arguments().size());
 
-      collect_lvsa_access_paths_ex(
+      collect_lvsa_access_paths_extended(
         fn_call.arguments().at(param_idx), Iit, lvsa, argument_lvalues);
     }
     else
@@ -651,7 +678,7 @@ void  taint_algorithm_computing_summary_of_functiont::
       {
         for(const auto param_index : it->second)
         {
-          collect_lvsa_access_paths_ex(
+          collect_lvsa_access_paths_extended(
             fn_call.arguments().at(param_index), Iit, lvsa, resolved_lvalues);
         }
       }
@@ -687,11 +714,7 @@ void taint_algorithm_computing_summary_of_functiont::
   for(const std::pair<taint_lvalue_numbert, taint_sett> &lvalue_taint :
       summary->get_output())
   {
-    const auto &lvalue=numbering->at(lvalue_taint.first);
-    if(lvalue.is_nil())
-      continue;
     lvalue_numbers_sett argument_lvalues;
-    if(symbol_utilst(program->get_namespace()).is_parameter(lvalue))
     {
       const auto it = summary->get_map_output_lvalues_to_param_indices().find(
         lvalue_taint.first);
@@ -701,7 +724,7 @@ void taint_algorithm_computing_summary_of_functiont::
       {
         for(const auto param_index : it->second)
         {
-          collect_lvsa_access_paths_ex(
+          collect_lvsa_access_paths_extended(
             fn_call.arguments().at(param_index), Iit, lvsa, argument_lvalues);
         }
       }
@@ -752,8 +775,8 @@ void taint_algorithm_computing_summary_of_functiont::initialise_domain(
     if(it->type==ASSIGN)
     {
       const code_assignt &asgn = to_code_assign(it->code);
-      collect_lvsa_access_paths_ex(asgn.lhs(), it, lvsa, environment);
-      collect_lvsa_access_paths_ex(asgn.lhs(), it, lvsa, written);
+      collect_lvsa_access_paths_extended(asgn.lhs(), it, lvsa, environment);
+      collect_lvsa_access_paths_extended(asgn.lhs(), it, lvsa, written);
       if(asgn.rhs().id()==ID_side_effect)
       {
         const side_effect_exprt see=to_side_effect_expr(asgn.rhs());
@@ -763,12 +786,8 @@ void taint_algorithm_computing_summary_of_functiont::initialise_domain(
             program->get_NONDET_retvals_replacements().find(it);
           if(replace_it != program->get_NONDET_retvals_replacements().cend())
           {
-            const auto env_size = environment.size();
-            collect_lvsa_access_paths_ex(
+            collect_lvsa_access_paths_extended(
               replace_it->second, it, lvsa, environment);
-            if(env_size == environment.size())
-              collect_lvsa_access_paths_ex(
-                replace_it->second, it, lvsa, environment);
           }
           else
           {
@@ -778,10 +797,10 @@ void taint_algorithm_computing_summary_of_functiont::initialise_domain(
         }
       }
       else
-        collect_lvsa_access_paths_ex(asgn.rhs(), it, lvsa, environment);
+        collect_lvsa_access_paths_extended(asgn.rhs(), it, lvsa, environment);
     }
     else if(it->type == ASSERT)
-      collect_lvsa_access_paths_ex(it->guard, it, lvsa, environment);
+      collect_lvsa_access_paths_extended(it->guard, it, lvsa, environment);
     else if(it->type==FUNCTION_CALL)
     {
       const code_function_callt &fn_call = to_code_function_call(it->code);
@@ -805,7 +824,7 @@ void taint_algorithm_computing_summary_of_functiont::initialise_domain(
         // In the future we could be more precise about exactly which arguments
         // are used in the rule.
         for(const auto &arg : fn_call.arguments())
-          collect_lvsa_access_paths_ex(arg, it, lvsa, environment);
+          collect_lvsa_access_paths_extended(arg, it, lvsa, environment);
         if(!database.contains(callee_id))
           continue;
       }
@@ -829,7 +848,7 @@ void taint_algorithm_computing_summary_of_functiont::initialise_domain(
             "Parameter symbol doesn't match?");
           const auto paramidx=
             std::distance(fn_type.parameters().begin(), findit);
-          collect_lvsa_access_paths_ex(
+          collect_lvsa_access_paths_extended(
             fn_call.arguments()[paramidx], it, lvsa, environment);
         }
         else if(!symbol_utilst(program->get_namespace()).is_parameter(
@@ -958,7 +977,7 @@ void taint_algorithm_computing_summary_of_functiont::handle_assignment(
     asgn.rhs(), Iit, lvsa, input, a);
 
   lvalue_numbers_sett lhs;
-  if(collect_lvsa_access_paths_ex(asgn.lhs(), Iit, lvsa, lhs))
+  if(collect_lvsa_access_paths_extended(asgn.lhs(), Iit, lvsa, lhs))
   {
     // Singular implies that lhs has exactly one element,
     // so we can access it directly
@@ -984,7 +1003,7 @@ taint_sett taint_algorithm_computing_summary_of_functiont::
   TMPROF_BLOCK();
 
   lvalue_numbers_sett numbers_of_aliases;
-  collect_lvsa_access_paths_ex(lvalue, Iit, lvsa, numbers_of_aliases);
+  collect_lvsa_access_paths_extended(lvalue, Iit, lvsa, numbers_of_aliases);
   taint_sett result;
   for(taint_lvalue_numbert lvalue_number : numbers_of_aliases)
   {
@@ -1014,7 +1033,7 @@ void taint_algorithm_computing_summary_of_functiont::
 
   lvalue_numbers_sett numbers_of_aliases;
   bool singular =
-    collect_lvsa_access_paths_ex(lvalue, Iit, lvsa, numbers_of_aliases);
+    collect_lvsa_access_paths_extended(lvalue, Iit, lvsa, numbers_of_aliases);
   if(singular || (is_allowed_pure_assignment && numbers_of_aliases.size() == 1))
   {
     // Singular implies that lhs has exactly one element,
@@ -1070,7 +1089,7 @@ numbered_lvalue_to_taint_mapt taint_algorithm_computing_summary_of_functiont::
                 replace_it->second, Iit, lvsa, input, a);
 
             lvalue_numbers_sett numbers_of_aliases;
-            collect_lvsa_access_paths_ex(
+            collect_lvsa_access_paths_extended(
               replace_it->second, Iit, lvsa, numbers_of_aliases);
             assert(!numbers_of_aliases.empty());
             std::stringstream sstr;
@@ -1093,7 +1112,7 @@ numbered_lvalue_to_taint_mapt taint_algorithm_computing_summary_of_functiont::
                sstr.str()});
 
             lvalue_numbers_sett lhs;
-            collect_lvsa_access_paths_ex(asgn.lhs(), Iit, lvsa, lhs);
+            collect_lvsa_access_paths_extended(asgn.lhs(), Iit, lvsa, lhs);
             for(const auto& path : lhs)
             {
               if(lhs.size() > 1UL)
@@ -1584,11 +1603,11 @@ void taint_algorithm_computing_summary_of_functiont::
     program->get_functions().function_map.at(fn_id);
   const code_typet &fn_type = to_code_type(func_ref.type);
   const auto fn_end_iter = std::prev(fn_to_summarise.body.instructions.cend());
-  for(std::size_t i = 0UL, n = fn_type.parameters().size(); i != n; ++i)
+  for(std::size_t i = 0UL; i != fn_type.parameters().size(); ++i)
   {
     const code_typet::parametert &param = fn_type.parameters().at(i);
     lvalue_numbers_sett numbers;
-    collect_lvsa_access_paths_ex(param, fn_end_iter, lvsa, numbers);
+    collect_lvsa_access_paths_extended(param, fn_end_iter, lvsa, numbers);
     for(const auto number : numbers)
     {
       if(summary.output.count(number) != 0UL)

src/taint-analysis/taint_summary.h

@@ -206,7 +206,7 @@ class taint_algorithm_computing_summary_of_functiont
     local_value_set_analysist::dbt &lvsa_db);
 
 private:
-  bool collect_lvsa_access_paths_ex(
+  bool collect_lvsa_access_paths_extended(
     const exprt &expr,
     const instruction_iteratort &it,
     local_value_set_analysist &lvsa,