Bugs found during limbo compiler work - Ref counting mistakes in vm_array copy_from() - document perf opportunity - Multiple bugs in addc instruction - set_rune() on vm_string was creating non-null terminated strings - Add support for '*', 'c', and 'u' flags in print formatting

arr committed 9 years ago

Commit 57e9d241 parent 3f81a2c4 files · +86 −44Browse files

Unified · Split

4 files

src/vm/array.cpp +12 −4
src/vm/execution_table.cpp +30 −24
src/vm/string.cpp +16 −10
src/vm/sys/print.cpp +28 −6

     auto dest_arr = _arr + (this_begin_index * _element_type->size_in_bytes);
-    // Increment all pointers if we have reference types.
+    // If we have reference types, increment references in the
+    // source array and decrement them in the destination array.
     if (_element_type->map_in_bytes != 0)
     {
-        auto &type_desc = *_element_type;
+        // [PERF] There is a chance this copy is within the same array, therefore
+        // a check could be made to not ref count memory that is going to be moved.
+        // The Inferno implementation of slicela handles this case (libinterp/xec.c).
+        const auto &type_desc = *_element_type;
         auto source_array_element = source._arr;
+        auto dest_array_element = dest_arr;
         for (auto i = word_t{ 0 }; i < source._length; ++i)
         {
-            source_array_element += (i * type_desc.size_in_bytes);
             inc_ref_count_in_memory(type_desc, source_array_element);
+            source_array_element += type_desc.size_in_bytes;
+            destroy_memory(type_desc, dest_array_element);
+            dest_array_element += type_desc.size_in_bytes;
         }
     }
-    // Moving data within the same array is permitted so use memmove().
+    // Copy data from within the same array is permitted.
     std::memmove(dest_arr, source._arr, (source._length * _element_type->size_in_bytes));
 }

src/vm/execution_table.cpp+30−24

         vt_ref<big_t>(r.dest) = vt_ref<big_t>(r.mid) >> vt_ref<big_t>(r.src);
         // Right shifting a negative number is implementation dependent in C++.
         // Assert the current compiler defaults to arithmetic shifting, not logical.
-        assert(vt_ref<word_t>(r.mid) >= 0 || (vt_ref<word_t>(r.mid) < 0 && vt_ref<word_t>(r.dest) < 0));
+        assert(vt_ref<big_t>(r.mid) >= 0 || (vt_ref<big_t>(r.mid) < 0 && vt_ref<big_t>(r.dest) < 0));
     }
     EXEC_DECL(lsrw) { vt_ref<word_t>(r.dest) = vt_ref<uword_t>(r.mid) >> vt_ref<word_t>(r.src); }
         assert(static_cast<std::size_t>(len) <= buffer.size());
         auto new_string = new vm_string_t(len, reinterpret_cast<uint8_t *>(buffer.data()));
-        auto str = at_val<vm_string_t>(r.dest);
+        auto prev = at_val<vm_alloc_t>(r.dest);
-        dec_ref_count_and_free(str);
+        dec_ref_count_and_free(prev);
         pt_ref(r.dest) = new_string->get_allocation();
     }
         auto new_string = new vm_string_t(len, reinterpret_cast<uint8_t *>(buffer.data()));
-        auto str = at_val<vm_string_t>(r.dest);
+        auto prev = at_val<vm_alloc_t>(r.dest);
+        dec_ref_count_and_free(prev);
         pt_ref(r.dest) = new_string->get_allocation();
-        dec_ref_count_and_free(str);
     }
     EXEC_DECL(cvtcl)
         auto new_string = new vm_string_t(len, reinterpret_cast<uint8_t *>(buffer.data()));
-        auto str = at_val<vm_string_t>(r.dest);
+        auto prev = at_val<vm_alloc_t>(r.dest);
+        dec_ref_count_and_free(prev);
         pt_ref(r.dest) = new_string->get_allocation();
-        dec_ref_count_and_free(str);
     }
     //
         auto index = vt_ref<word_t>(r.mid);
         auto str = at_val<vm_string_t>(r.dest);
         if (str == nullptr)
-            str = new vm_string_t();
+            str = new vm_string_t{};
         else if (str->get_ref_count() > 1)
-            str = new vm_string_t(*str, 0, str->get_length());
+            str = new vm_string_t{ *str, 0, str->get_length() };
         // Set the rune at the supplied index.
         str->set_rune(index, rune);
         }
     }
-    vm_string_t *_addc(vm_string_t *s1, const vm_string_t *s2, /* in out */ bool &append_to_s1)
+    // Returns a string instance only if it is new
+    vm_string_t *concat_string(vm_string_t *s1, const vm_string_t *s2, bool try_append_to_s1)
     {
         if (s1 == nullptr)
         {
             return new vm_string_t{ *s2, 0, s2->get_length() };
         }
-        // There are other references to this string so we will not alter it.
+        // If there are other references to the string, it is immutable.
         if (s1->get_ref_count() > 1)
-            append_to_s1 = false;
+            try_append_to_s1 = false;
         if (s2 == nullptr)
         {
-            if (append_to_s1)
+            if (try_append_to_s1)
-                return s1;
+                return nullptr;
             return new vm_string_t(*s1, 0, s1->get_length());
         }
-        return (append_to_s1) ? &(s1->append(*s2)) : new vm_string_t(*s1, *s2);
+        if (!try_append_to_s1)
+            return new vm_string_t(*s1, *s2);
+        s1->append(*s2);
+        return nullptr;
     }
     // [SPEC] 'Concat strings' op code
     //   dst = src2 + src1
     EXEC_DECL(addc)
     {
+        const auto try_append_to_s1 = r.mid == r.dest;
         auto s1 = at_val<vm_string_t>(r.mid);
         const auto s2 = at_val<vm_string_t>(r.src);
-        auto dest_maybe = at_val<vm_string_t>(r.dest);
-        // If the source is the same as the destination and the
+        // If the source is the same as the destination, try to append the string during concatenation.
-        // destination is not null, append the string. This can
+        const auto new_str_maybe = concat_string(s1, s2, try_append_to_s1);
-        // change during the concat process.
+        if (new_str_maybe != nullptr)
-        auto append_to_source = dest_maybe != nullptr && s1 == dest_maybe;
-        const auto str = _addc(s1, s2, append_to_source);
-        if (!append_to_source)
         {
-            pt_ref(r.dest) = str->get_allocation();
+            auto dest_maybe = at_val<vm_alloc_t>(r.dest);
             dec_ref_count_and_free(dest_maybe);
+            pt_ref(r.dest) = new_str_maybe->get_allocation();
         }
     }
             // The remainder of the type is user-defined.
             auto content = e->get_allocation();
             exception_id = at_val<vm_string_t>(content);
+            assert(exception_id != nullptr);
             if (debug::is_component_tracing_enabled<debug::component_trace_t::exception>())
                 debug::log_msg(debug::component_trace_t::exception, debug::log_level_t::debug, "raise: exception: >>%s<<", exception_id->str());

src/vm/string.cpp+16−10

         // Length + 1 (null) + space for string expansion
         return (current_length + 1 + (current_length / 4));
     }
+    template<typename T, word_t S>
+    constexpr word_t compute_max_length(const T(&)[S])
+    {
+        return S - 1;
+    }
 }
 vm_string_t::vm_string_t()
     , _encoded_str{ nullptr }
     , _is_alloc{ false }
     , _length{ 0 }
-    , _length_max{ sizeof(_mem.local) }
+    , _length_max{ compute_max_length(_mem.local) }
     , _mem{}
 { }
     , _encoded_str{ nullptr }
     , _is_alloc{ false }
     , _length{ 0 }
-    , _length_max{ sizeof(_mem.local) }
+    , _length_max{ compute_max_length(_mem.local) }
     , _mem{}
 {
     assert((encoded_str_len > 0 && encoded_str != nullptr) || encoded_str_len == 0);
     _character_size = (encoded_str_len != utf8_length.codepoint_count) ? sizeof(rune_t) : sizeof(char);
     _length = static_cast<word_t>(utf8_length.codepoint_count);
-    _length_max = sizeof(_mem.local) / _character_size;
+    _length_max = compute_max_length(_mem.local) / _character_size;
     // Allocate memory based on character count and size
     auto dest = _mem.local;
-    if ((encoded_str_len + 1) > sizeof(_mem.local))
+    if (encoded_str_len > static_cast<std::size_t>(compute_max_length(_mem.local)))
     {
         // If we are going to allocate, make sure it is larger than needed.
         _length_max = compute_max_length(_length);
     , _length_max{ compute_max_length(end_index - begin_index) }
     , _mem{}
 {
-    if (end_index <= begin_index)
+    if (end_index < begin_index)
         throw out_of_range_memory{};
     if (begin_index < 0)
     // Define the values for the appended string
     const auto new_length = local_length + other_length;
-    const auto new_length_max = (new_length == 0) ? static_cast<word_t>(sizeof(_mem.local)) : compute_max_length(new_length);
+    const auto new_length_max = (new_length == 0) ? static_cast<word_t>(compute_max_length(_mem.local)) : compute_max_length(new_length);
     const auto new_is_rune = local_is_rune || other_is_rune;
     uint8_t *new_source = nullptr;
         _character_size = static_cast<uint8_t>(new_character_size);
         assert(_character_size == sizeof(rune_t));
-        _length_max = (_length == 0) ? static_cast<word_t>(sizeof(_mem.local)) : compute_max_length(_length);
+        _length_max = (_length == 0) ? static_cast<word_t>(compute_max_length(_mem.local)) : compute_max_length(_length);
         auto new_alloc = calloc_memory<rune_t>(_length_max * _character_size);
         // Copy source into the new alloc.
     }
     else if (static_cast<word_t>(_length_max) <= index) // Check if string can accomodate the new character.
     {
-        _length_max = (_length == 0) ? static_cast<word_t>(sizeof(_mem.local)) : compute_max_length(_length);
+        _length_max = (_length == 0) ? static_cast<word_t>(compute_max_length(_mem.local)) : compute_max_length(_length);
         auto new_alloc = calloc_memory<uint8_t>(_length_max * _character_size);
-        ::memcpy(new_alloc, source, _length * _character_size);
+        std::memcpy(new_alloc, source, _length * _character_size);
         // Free memory, if allocated
         if (_is_alloc)
         if (_encoded_str != nullptr)
             return _encoded_str;
-        // If the string is ASCII then just return the memory
+        // If the string is ASCII then return the memory
         if (_character_size == sizeof(char))
         {
             _encoded_str = (_is_alloc) ? reinterpret_cast<char *>(_mem.alloc) : reinterpret_cast<char *>(_mem.local);

src/vm/sys/print.cpp+28−6

     const auto fmt_len = msg_fmt.get_length();
     auto i = word_t{ 0 };
+    auto format = std::vector<char>{};
+    format.reserve(32); // Default size in Inferno.
     // Loop over the entire format string
     while (i < fmt_len)
     {
         if (b_curr >= b_end)
             return -1;
-        auto c = msg_fmt.get_rune(i);
+        auto c = msg_fmt.get_rune(i++);
-        ++i;
         if (c != '%')
         {
             b_curr = reinterpret_cast<char *>(utf8::encode(c, reinterpret_cast<uint8_t *>(b_curr)));
             continue;
         }
-        auto format = std::vector<char>{};
+        format.clear();
         format.push_back('%');
         auto wb = int{ 0 };
         auto big_flag = false;
+        auto unsigned_flag = false;
         // Decode the format specifier
         while (i < fmt_len)
         {
-            c = msg_fmt.get_rune(i);
+            c = msg_fmt.get_rune(i++);
-            ++i;
             switch (c)
             {
                 // Format flags
                 format.push_back('l');
                 big_flag = true;
                 continue;
+            case '*': // Placeholder for word argument
+            {
+                const auto w = *reinterpret_cast<word_t *>(msg_args);
+                std::array<char, sizeof("-2147483648")> buffer;
+                const auto len = std::sprintf(buffer.data(), "%d", w);
+                if (len < 0)
+                    return -1;
+                format.insert(std::end(format), buffer.data(), buffer.data() + len);
+                msg_args += sizeof(w);
+                continue;
+            }
+            case 'u':
+                unsigned_flag = true;
+                continue;
                 // Format verbs
             case '%': // Escaped specifier
                 msg_args += sizeof(r);
                 break;
             }
-            case 'o': // octal
             case 'd': // decimal
+                if (unsigned_flag)
+                    c = 'u';
+            case 'o': // octal
             case 'x': // hexadecimal
             case 'X': // Upper-case hexadecimal
+            case 'c': // Unicode
                 format.push_back(static_cast<char>(c));
                 format.push_back('\0');
                 if (big_flag)
                     msg_args += sizeof(w);
                 }
                 break;
+            case 'q':
+                assert(false && "'q' format not implemented");
             case 's': // string
             {
                 auto p = *reinterpret_cast<pointer_t *>(msg_args);

Bugs found during limbo compiler work - Ref counting mistakes in vm_array copy_from() - document perf opportunity - Multiple bugs in addc instruction - set_rune() on vm_string was creating non-null terminated strings - Add support for '*', 'c', and 'u' flags in print formatting

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code