Address review comments

0: explained why floating point test is failing
1: use safe string2int conversion
2: memory_map is indexed by irep_idts
3: data in memory_scope are private and have getters
4: accessing types within this PR (and analyze_symbol.cpp) is a bit more
readable

5: Checking address containment within memory scope and pointer offset distance
shared the address-to-string conversion which was computed twice. It now has a
separate private method so that the conversion is called only once when
computing distance.

Author: petr-bauch

regression/snapshot-harness/static-array-float/test.desc

@@ -11,3 +11,5 @@ array,iterator1,iterator2 --harness-type initialise-with-memory-snapshot --initi
 VERIFICATION FAILED
 --
 unwinding assertion loop \d+: FAILURE
+--
+memory analyzer does not yet allow extract floating point values

src/memory-analyzer/analyze_symbol.cpp

@@ -31,21 +31,28 @@ gdb_value_extractort::gdb_value_extractort(
 {
 }
 
-bool gdb_value_extractort::memory_scopet::contains(
+gdb_value_extractort::memory_scopet::memory_scopet(
+  const memory_addresst &begin,
+  const mp_integer &byte_size,
+  const irep_idt &name)
+  : begin_int(safe_string2size_t(begin.address_string, 0)),
+    byte_size(byte_size),
+    name(name)
+{
+}
+
+size_t gdb_value_extractort::memory_scopet::address2size_t(
   const memory_addresst &point) const
 {
-  size_t begin_int = std::strtoul(begin.address_string.c_str(), NULL, 0);
-  size_t point_int = std::strtoul(point.address_string.c_str(), NULL, 0);
-  return point_int >= begin_int && (begin_int + byte_size) > point_int;
+  return safe_string2size_t(point.address_string, 0);
 }
 
 mp_integer gdb_value_extractort::memory_scopet::distance(
   const memory_addresst &point,
   mp_integer member_size) const
 {
-  CHECK_RETURN(contains(point));
-  size_t begin_int = std::strtoul(begin.address_string.c_str(), NULL, 0);
-  size_t point_int = std::strtoul(point.address_string.c_str(), NULL, 0);
+  auto point_int = address2size_t(point);
+  CHECK_RETURN(check_containment(point_int));
   return (point_int - begin_int) / member_size;
 }
 
@@ -55,7 +62,7 @@ gdb_value_extractort::find_dynamic_allocation(irep_idt name)
   return std::find_if(
     dynamically_allocated.begin(),
     dynamically_allocated.end(),
-    [&name](const memory_scopet &scope) { return scope.name == name; });
+    [&name](const memory_scopet &scope) { return scope.id() == name; });
 }
 
 std::vector<gdb_value_extractort::memory_scopet>::iterator
@@ -75,7 +82,7 @@ optionalt<mp_integer> gdb_value_extractort::get_malloc_size(irep_idt name)
   if(scope_it == dynamically_allocated.end())
     return {};
   else
-    return scope_it->byte_size;
+    return scope_it->size();
 }
 
 optionalt<std::string> gdb_value_extractort::get_malloc_pointee(
@@ -87,7 +94,7 @@ optionalt<std::string> gdb_value_extractort::get_malloc_pointee(
     return {};
 
   const auto pointer_distance = scope_it->distance(point, member_size);
-  return id2string(scope_it->name) +
+  return id2string(scope_it->id()) +
          (pointer_distance > 0 ? "+" + integer2string(pointer_distance) : "");
 }
 
@@ -114,14 +121,13 @@ void gdb_value_extractort::analyze_symbols(
     values.insert({value.address, symbol_expr});
 
     const symbolt &symbol = ns.lookup(id);
-    const symbol_exprt actual_expr = symbol.symbol_expr();
-    if(actual_expr.type().id() != ID_pointer)
+    if(symbol.type.id() != ID_pointer)
     {
-      memory_map[id2string(id)] = value;
+      memory_map[id] = value;
       continue;
     }
 
-    const std::string c_symbol = c_converter.convert(actual_expr);
+    const std::string c_symbol = c_converter.convert(symbol.symbol_expr());
     const pointer_valuet &symbol_value = gdb_api.get_memory(c_symbol);
     size_t symbol_size = gdb_api.query_malloc_size(c_symbol);
 
@@ -287,29 +293,30 @@ exprt gdb_value_extractort::get_pointer_to_member_value(
   const symbolt *struct_symbol = symbol_table.lookup(struct_name);
   DATA_INVARIANT(struct_symbol != nullptr, "unknown struct");
 
-  if(memory_map.count(struct_name) == 0)
+  if(!has_known_memory_location(struct_name))
   {
     memory_map[struct_name] = gdb_api.get_memory(struct_name);
     analyze_symbol(irep_idt{struct_name});
   }
 
-  if(struct_symbol->symbol_expr().type().id() == ID_array)
+  const auto &struct_symbol_expr = struct_symbol->symbol_expr();
+  if(struct_symbol->type.id() == ID_array)
   {
     return index_exprt{
-      struct_symbol->symbol_expr(),
+      struct_symbol_expr,
       from_integer(
         member_offset / get_type_size(expr.type().subtype()), index_type())};
   }
-  if(struct_symbol->symbol_expr().type().id() == ID_pointer)
+  if(struct_symbol->type.id() == ID_pointer)
   {
     return dereference_exprt{
-      plus_exprt{struct_symbol->symbol_expr(),
+      plus_exprt{struct_symbol_expr,
                  from_integer(member_offset, size_type()),
                  expr.type()}};
   }
 
   const auto maybe_member_expr = get_subexpression_at_offset(
-    struct_symbol->symbol_expr(), member_offset, expr.type().subtype(), ns);
+    struct_symbol_expr, member_offset, expr.type().subtype(), ns);
   DATA_INVARIANT(
     maybe_member_expr.has_value(), "structure doesn't have member");
 

src/memory-analyzer/analyze_symbol.h

@@ -93,37 +93,70 @@ class gdb_value_extractort
 
   struct memory_scopet
   {
-    memory_addresst begin;
+  private:
+    size_t begin_int;
     mp_integer byte_size;
     irep_idt name;
 
-    memory_scopet() = delete;
-    memory_scopet(
-      const memory_addresst &begin,
-      mp_integer byte_size,
-      irep_idt name)
-      : begin(begin), byte_size(byte_size), name(name)
+    /// Convert base-16 memory address to a natural number
+    /// \param point: the memory address to be converted
+    /// \return base-10 unsigned integer equal in value to \p point
+    size_t address2size_t(const memory_addresst &point) const;
+
+    /// Helper function that check if a point in memory points inside this scope
+    /// \param point_int: memory point as natural number
+    /// \return true if the point is inside this scope
+    bool check_containment(const size_t &point_int) const
     {
+      return point_int >= begin_int && (begin_int + byte_size) > point_int;
     }
 
+  public:
+    memory_scopet(
+      const memory_addresst &begin,
+      const mp_integer &byte_size,
+      const irep_idt &name);
+
     /// Check if \p point points somewhere in this memory scope
     /// \param point: memory address to be check for presence
     /// \return true if \p point is inside *this
-    bool contains(const memory_addresst &point) const;
+    bool contains(const memory_addresst &point) const
+    {
+      return check_containment(address2size_t(point));
+    }
 
     /// Compute the distance of \p point from the beginning of this scope
     /// \param point: memory address to have the offset computed
     /// \param member_size: size of one element of this scope in bytes
     /// \return `n' such that \p point is the n-th element of this scope
     mp_integer
     distance(const memory_addresst &point, mp_integer member_size) const;
+
+    /// Getter for the name of this memory scope
+    /// \return the name as irep id
+    irep_idt id() const
+    {
+      return name;
+    }
+
+    /// Getter for the allocation size of this memory scope
+    /// \return the size in bytes
+    mp_integer size() const
+    {
+      return byte_size;
+    }
   };
 
   /// Keep track of the dynamically allocated memory
   std::vector<memory_scopet> dynamically_allocated;
 
   /// Keep track of the memory location for the analyzed symbols
-  std::map<std::string, pointer_valuet> memory_map;
+  std::map<irep_idt, pointer_valuet> memory_map;
+
+  bool has_known_memory_location(const irep_idt &id) const
+  {
+    return memory_map.count(id) != 0;
+  }
 
   /// Search for a memory scope allocated under \p name
   /// \param name: name of the pointer used during allocation