From de3d258bfbf50d99292045cd0e56993f9386ac57 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 28 Jan 2019 14:00:45 -0800 Subject: [PATCH 1/8] Add Small String Optimization, min String size Reduce String memory overhead from 24 bytes to 16 bytes by limiting the maximum string length to <64Kbytes (which is larger than heap so no effective problem). Add Small String Optimization, SSO, which instead of allocating pointers to small strings on the heap will store the string in place of the pointer in the class. This should reduce memory fragmentation as Strings of 0-3 characters will not need extra malloc()s. No user code changes should be required to work with this optimization. --- cores/esp8266/StreamString.cpp | 4 +- cores/esp8266/WString.cpp | 229 ++++++++++++++++++++------------- cores/esp8266/WString.h | 24 ++-- 3 files changed, 155 insertions(+), 102 deletions(-) diff --git a/cores/esp8266/StreamString.cpp b/cores/esp8266/StreamString.cpp index 7ebf11d0e2..106a05e60e 100644 --- a/cores/esp8266/StreamString.cpp +++ b/cores/esp8266/StreamString.cpp @@ -26,9 +26,9 @@ size_t StreamString::write(const uint8_t *data, size_t size) { if(size && data) { if(reserve(length() + size + 1)) { - memcpy((void *) (buffer + len), (const void *) data, size); + memcpy((void *) (wbuffer() + len), (const void *) data, size); len += size; - *(buffer + len) = 0x00; // add null for string end + *(wbuffer() + len) = 0x00; // add null for string end return size; } } diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 51c91238e2..01091d3736 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -129,39 +129,74 @@ String::~String() { // /*********************************************/ inline void String::init(void) { - buffer = NULL; + bufptr = NULL; capacity = 0; len = 0; + sso = false; } void String::invalidate(void) { - if(buffer) - free(buffer); + if(!sso && bufptr) + free(bufptr); init(); } unsigned char String::reserve(unsigned int size) { - if(buffer && capacity >= size) + if(buffer() && capacity >= size) return 1; if(changeBuffer(size)) { if(len == 0) - buffer[0] = 0; + wbuffer()[0] = 0; return 1; } return 0; } unsigned char String::changeBuffer(unsigned int maxStrLen) { + if (maxStrLen > 0xffff) { + // Sanity check len can fit in 16-bits + return 0; + } + // Can we use SSO here to avoid allocation? + if (maxStrLen < 4) { + if (sso || !bufptr) { + // Already using SSO, nothing to do but set xap properly + sso = true; + capacity = sizeof(sso_buff) - 1; // We subtract off trailing 0 space from available + return 1; + } else { // if bufptr + // Using bufptr, need to shrink into sso_buff + char temp[4]; + memcpy(temp, buffer(), maxStrLen); + free(bufptr); + sso = true; + memcpy(wbuffer(), temp, maxStrLen); + capacity = sizeof(sso_buff) - 1; // We subtract off trailing 0 space from available + return 1; + } + } + // Fallthrough to normal allocator size_t newSize = (maxStrLen + 16) & (~0xf); - char *newbuffer = (char *) realloc(buffer, newSize); + char temp[4]; + if (buffer()) { + memcpy(temp, buffer(), 4); // Just in case SSO was on + } else { + memset(temp, 0, 0); + } + char *newbuffer = (char *) realloc(sso ? nullptr : bufptr, newSize); if(newbuffer) { size_t oldSize = capacity + 1; // include NULL. + if (sso) { + // Copy the SSO buffer into allocated space + memcpy(newbuffer, temp, 4); + sso = false; + } if (newSize > oldSize) { memset(newbuffer + oldSize, 0, newSize - oldSize); } capacity = newSize - 1; - buffer = newbuffer; + bufptr = newbuffer; return 1; } return 0; @@ -177,7 +212,7 @@ String & String::copy(const char *cstr, unsigned int length) { return *this; } len = length; - strcpy(buffer, cstr); + strcpy(wbuffer(), cstr); return *this; } @@ -187,26 +222,36 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) { return *this; } len = length; - strcpy_P(buffer, (PGM_P)pstr); + strcpy_P(wbuffer(), (PGM_P)pstr); return *this; } #ifdef __GXX_EXPERIMENTAL_CXX0X__ void String::move(String &rhs) { - if(buffer) { + if(buffer()) { if(capacity >= rhs.len) { - strcpy(buffer, rhs.buffer); + strcpy(wbuffer(), rhs.buffer()); len = rhs.len; rhs.len = 0; return; } else { - free(buffer); + if (!sso) { + free(bufptr); + bufptr = nullptr; + } } } - buffer = rhs.buffer; + if (rhs.sso) { + sso = true; + memcpy(sso_buff, rhs.sso_buff, sizeof(sso_buff)); + } else { + sso = false; + bufptr = rhs.bufptr; + } capacity = rhs.capacity; len = rhs.len; - rhs.buffer = NULL; + rhs.bufptr = nullptr; + rhs.sso = false; rhs.capacity = 0; rhs.len = 0; } @@ -216,8 +261,8 @@ String & String::operator =(const String &rhs) { if(this == &rhs) return *this; - if(rhs.buffer) - copy(rhs.buffer, rhs.len); + if(rhs.buffer()) + copy(rhs.buffer(), rhs.len); else invalidate(); @@ -264,18 +309,18 @@ unsigned char String::concat(const String &s) { // realloc'ing the buffer and moving s.buffer in the method called if (&s == this) { unsigned int newlen = 2 * len; - if (!s.buffer) + if (!s.buffer()) return 0; if (s.len == 0) return 1; if (!reserve(newlen)) return 0; - memcpy(buffer + len, buffer, len); + memcpy(wbuffer() + len, buffer(), len); len = newlen; - buffer[len] = 0; + wbuffer()[len] = 0; return 1; } else { - return concat(s.buffer, s.len); + return concat(s.buffer(), s.len); } } @@ -287,7 +332,7 @@ unsigned char String::concat(const char *cstr, unsigned int length) { return 1; if(!reserve(newlen)) return 0; - strcpy(buffer + len, cstr); + strcpy(wbuffer() + len, cstr); len = newlen; return 1; } @@ -353,7 +398,7 @@ unsigned char String::concat(const __FlashStringHelper * str) { if (length == 0) return 1; unsigned int newlen = len + length; if (!reserve(newlen)) return 0; - strcpy_P(buffer + len, (PGM_P)str); + strcpy_P(wbuffer() + len, (PGM_P)str); len = newlen; return 1; } @@ -364,7 +409,7 @@ unsigned char String::concat(const __FlashStringHelper * str) { StringSumHelper & operator +(const StringSumHelper &lhs, const String &rhs) { StringSumHelper &a = const_cast(lhs); - if(!a.concat(rhs.buffer, rhs.len)) + if(!a.concat(rhs.buffer(), rhs.len)) a.invalidate(); return a; } @@ -444,14 +489,14 @@ StringSumHelper & operator + (const StringSumHelper &lhs, const __FlashStringHel // /*********************************************/ int String::compareTo(const String &s) const { - if(!buffer || !s.buffer) { - if(s.buffer && s.len > 0) - return 0 - *(unsigned char *) s.buffer; - if(buffer && len > 0) - return *(unsigned char *) buffer; + if(!buffer() || !s.buffer()) { + if(s.buffer() && s.len > 0) + return 0 - *(unsigned char *) s.buffer(); + if(buffer() && len > 0) + return *(unsigned char *) buffer(); return 0; } - return strcmp(buffer, s.buffer); + return strcmp(buffer(), s.buffer()); } unsigned char String::equals(const String &s2) const { @@ -462,8 +507,8 @@ unsigned char String::equals(const char *cstr) const { if(len == 0) return (cstr == NULL || *cstr == 0); if(cstr == NULL) - return buffer[0] == 0; - return strcmp(buffer, cstr) == 0; + return buffer()[0] == 0; + return strcmp(buffer(), cstr) == 0; } unsigned char String::operator<(const String &rhs) const { @@ -489,8 +534,8 @@ unsigned char String::equalsIgnoreCase(const String &s2) const { return 0; if(len == 0) return 1; - const char *p1 = buffer; - const char *p2 = s2.buffer; + const char *p1 = buffer(); + const char *p2 = s2.buffer(); while(*p1) { if(tolower(*p1++) != tolower(*p2++)) return 0; @@ -507,8 +552,8 @@ unsigned char String::equalsConstantTime(const String &s2) const { if(len == 0) return 1; //at this point lenghts are the same and non-zero - const char *p1 = buffer; - const char *p2 = s2.buffer; + const char *p1 = buffer(); + const char *p2 = s2.buffer(); unsigned int equalchars = 0; unsigned int diffchars = 0; while(*p1) { @@ -532,15 +577,15 @@ unsigned char String::startsWith(const String &s2) const { } unsigned char String::startsWith(const String &s2, unsigned int offset) const { - if(offset > len - s2.len || !buffer || !s2.buffer) + if(offset > (unsigned)(len - s2.len) || !buffer() || !s2.buffer()) return 0; - return strncmp(&buffer[offset], s2.buffer, s2.len) == 0; + return strncmp(&buffer()[offset], s2.buffer(), s2.len) == 0; } unsigned char String::endsWith(const String &s2) const { - if(len < s2.len || !buffer || !s2.buffer) + if(len < s2.len || !buffer() || !s2.buffer()) return 0; - return strcmp(&buffer[len - s2.len], s2.buffer) == 0; + return strcmp(&buffer()[len - s2.len], s2.buffer()) == 0; } // /*********************************************/ @@ -553,22 +598,22 @@ char String::charAt(unsigned int loc) const { void String::setCharAt(unsigned int loc, char c) { if(loc < len) - buffer[loc] = c; + wbuffer()[loc] = c; } char & String::operator[](unsigned int index) { static char dummy_writable_char; - if(index >= len || !buffer) { + if(index >= len || !buffer()) { dummy_writable_char = 0; return dummy_writable_char; } - return buffer[index]; + return wbuffer()[index]; } char String::operator[](unsigned int index) const { - if(index >= len || !buffer) + if(index >= len || !buffer()) return 0; - return buffer[index]; + return buffer()[index]; } void String::getBytes(unsigned char *buf, unsigned int bufsize, unsigned int index) const { @@ -581,7 +626,7 @@ void String::getBytes(unsigned char *buf, unsigned int bufsize, unsigned int ind unsigned int n = bufsize - 1; if(n > len - index) n = len - index; - strncpy((char *) buf, buffer + index, n); + strncpy((char *) buf, buffer() + index, n); buf[n] = 0; } @@ -596,10 +641,10 @@ int String::indexOf(char c) const { int String::indexOf(char ch, unsigned int fromIndex) const { if(fromIndex >= len) return -1; - const char* temp = strchr(buffer + fromIndex, ch); + const char* temp = strchr(buffer() + fromIndex, ch); if(temp == NULL) return -1; - return temp - buffer; + return temp - buffer(); } int String::indexOf(const String &s2) const { @@ -609,10 +654,10 @@ int String::indexOf(const String &s2) const { int String::indexOf(const String &s2, unsigned int fromIndex) const { if(fromIndex >= len) return -1; - const char *found = strstr(buffer + fromIndex, s2.buffer); + const char *found = strstr(buffer() + fromIndex, s2.buffer()); if(found == NULL) return -1; - return found - buffer; + return found - buffer(); } int String::lastIndexOf(char theChar) const { @@ -622,13 +667,13 @@ int String::lastIndexOf(char theChar) const { int String::lastIndexOf(char ch, unsigned int fromIndex) const { if(fromIndex >= len) return -1; - char tempchar = buffer[fromIndex + 1]; - buffer[fromIndex + 1] = '\0'; - char* temp = strrchr(buffer, ch); - buffer[fromIndex + 1] = tempchar; + char tempchar = buffer()[fromIndex + 1]; + wbuffer()[fromIndex + 1] = '\0'; + char* temp = strrchr(wbuffer(), ch); + wbuffer()[fromIndex + 1] = tempchar; if(temp == NULL) return -1; - return temp - buffer; + return temp - buffer(); } int String::lastIndexOf(const String &s2) const { @@ -641,12 +686,12 @@ int String::lastIndexOf(const String &s2, unsigned int fromIndex) const { if(fromIndex >= len) fromIndex = len - 1; int found = -1; - for(char *p = buffer; p <= buffer + fromIndex; p++) { - p = strstr(p, s2.buffer); + for(char *p = wbuffer(); p <= wbuffer() + fromIndex; p++) { + p = strstr(p, s2.buffer()); if(!p) break; - if((unsigned int) (p - buffer) <= fromIndex) - found = p - buffer; + if((unsigned int) (p - wbuffer()) <= fromIndex) + found = p - buffer(); } return found; } @@ -662,10 +707,10 @@ String String::substring(unsigned int left, unsigned int right) const { return out; if(right > len) right = len; - char temp = buffer[right]; // save the replaced character - buffer[right] = '\0'; - out = buffer + left; // pointer arithmetic - buffer[right] = temp; //restore character + char temp = buffer()[right]; // save the replaced character + wbuffer()[right] = '\0'; + out = wbuffer() + left; // pointer arithmetic + wbuffer()[right] = temp; //restore character return out; } @@ -674,9 +719,9 @@ String String::substring(unsigned int left, unsigned int right) const { // /*********************************************/ void String::replace(char find, char replace) { - if(!buffer) + if(!buffer()) return; - for(char *p = buffer; *p; p++) { + for(char *p = wbuffer(); *p; p++) { if(*p == find) *p = replace; } @@ -686,20 +731,20 @@ void String::replace(const String& find, const String& replace) { if(len == 0 || find.len == 0) return; int diff = replace.len - find.len; - char *readFrom = buffer; + char *readFrom = wbuffer(); char *foundAt; if(diff == 0) { - while((foundAt = strstr(readFrom, find.buffer)) != NULL) { - memcpy(foundAt, replace.buffer, replace.len); + while((foundAt = strstr(readFrom, find.buffer())) != NULL) { + memcpy(foundAt, replace.buffer(), replace.len); readFrom = foundAt + replace.len; } } else if(diff < 0) { - char *writeTo = buffer; - while((foundAt = strstr(readFrom, find.buffer)) != NULL) { + char *writeTo = wbuffer(); + while((foundAt = strstr(readFrom, find.buffer())) != NULL) { unsigned int n = foundAt - readFrom; memcpy(writeTo, readFrom, n); writeTo += n; - memcpy(writeTo, replace.buffer, replace.len); + memcpy(writeTo, replace.buffer(), replace.len); writeTo += replace.len; readFrom = foundAt + find.len; len += diff; @@ -707,7 +752,7 @@ void String::replace(const String& find, const String& replace) { strcpy(writeTo, readFrom); } else { unsigned int size = len; // compute size needed for result - while((foundAt = strstr(readFrom, find.buffer)) != NULL) { + while((foundAt = strstr(readFrom, find.buffer())) != NULL) { readFrom = foundAt + find.len; size += diff; } @@ -717,11 +762,11 @@ void String::replace(const String& find, const String& replace) { return; // XXX: tell user! int index = len - 1; while(index >= 0 && (index = lastIndexOf(find, index)) >= 0) { - readFrom = buffer + index + find.len; - memmove(readFrom + diff, readFrom, len - (readFrom - buffer)); + readFrom = wbuffer() + index + find.len; + memmove(readFrom + diff, readFrom, len - (readFrom - buffer())); len += diff; - buffer[len] = 0; - memcpy(buffer + index, replace.buffer, replace.len); + wbuffer()[len] = 0; + memcpy(wbuffer() + index, replace.buffer(), replace.len); index--; } } @@ -744,41 +789,41 @@ void String::remove(unsigned int index, unsigned int count) { if(count > len - index) { count = len - index; } - char *writeTo = buffer + index; + char *writeTo = wbuffer() + index; len = len - count; - memmove(writeTo, buffer + index + count, len - index); - buffer[len] = 0; + memmove(writeTo, wbuffer() + index + count, len - index); + wbuffer()[len] = 0; } void String::toLowerCase(void) { - if(!buffer) + if(!buffer()) return; - for(char *p = buffer; *p; p++) { + for(char *p = wbuffer(); *p; p++) { *p = tolower(*p); } } void String::toUpperCase(void) { - if(!buffer) + if(!buffer()) return; - for(char *p = buffer; *p; p++) { + for(char *p = wbuffer(); *p; p++) { *p = toupper(*p); } } void String::trim(void) { - if(!buffer || len == 0) + if(!buffer() || len == 0) return; - char *begin = buffer; + char *begin = wbuffer(); while(isspace(*begin)) begin++; - char *end = buffer + len - 1; + char *end = wbuffer() + len - 1; while(isspace(*end) && end >= begin) end--; len = end + 1 - begin; - if(begin > buffer) - memmove(buffer, begin, len); - buffer[len] = 0; + if(begin > buffer()) + memmove(wbuffer(), begin, len); + wbuffer()[len] = 0; } // /*********************************************/ @@ -786,14 +831,14 @@ void String::trim(void) { // /*********************************************/ long String::toInt(void) const { - if(buffer) - return atol(buffer); + if(buffer()) + return atol(buffer()); return 0; } float String::toFloat(void) const { - if(buffer) - return atof(buffer); + if(buffer()) + return atof(buffer()); return 0; } diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index bbf8ee40ad..b04143616c 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -76,7 +76,7 @@ class String { // invalid string (i.e., "if (s)" will be true afterwards) unsigned char reserve(unsigned int size); inline unsigned int length(void) const { - if(buffer) { + if(buffer()) { return len; } else { return 0; @@ -172,7 +172,7 @@ class String { // comparison (only works w/ Strings and "strings") operator StringIfHelperType() const { - return buffer ? &String::StringIfHelper : 0; + return buffer() ? &String::StringIfHelper : 0; } int compareTo(const String &s) const; unsigned char equals(const String &s) const; @@ -208,9 +208,9 @@ class String { void toCharArray(char *buf, unsigned int bufsize, unsigned int index = 0) const { getBytes((unsigned char *) buf, bufsize, index); } - const char* c_str() const { return buffer; } - char* begin() { return buffer; } - char* end() { return buffer + length(); } + const char* c_str() const { return buffer(); } + char* begin() { return wbuffer(); } + char* end() { return wbuffer() + length(); } const char* begin() const { return c_str(); } const char* end() const { return c_str() + length(); } @@ -243,9 +243,17 @@ class String { float toFloat(void) const; protected: - char *buffer; // the actual char array - unsigned int capacity; // the array length minus one (for the '\0') - unsigned int len; // the String length (not counting the '\0') + union { + char *bufptr; // the actual char array + char sso_buff[4]; // Overwrite the ptr with actual string for < 4 chars + }; + unsigned sso : 1; + unsigned capacity : 15; // the array length minus one (for the '\0') + unsigned unused : 1; + unsigned len : 15; // the String length (not counting the '\0') + // Buffer accessor functions + inline const char *buffer() const { return (const char *)(sso ? sso_buff : bufptr); } + inline char *wbuffer() const { return sso ? const_cast(&sso_buff[0]) : bufptr; } // Writable version of buffer protected: void init(void); void invalidate(void); From c0a6b27799451ae32ea6bc24851fc3b38e0f5d06 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 28 Jan 2019 15:26:22 -0800 Subject: [PATCH 2/8] Fix string max length check We only have 15, not 16, bits of length, so adjust the check accordingly. --- cores/esp8266/WString.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 01091d3736..834572dd3b 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -153,8 +153,8 @@ unsigned char String::reserve(unsigned int size) { } unsigned char String::changeBuffer(unsigned int maxStrLen) { - if (maxStrLen > 0xffff) { - // Sanity check len can fit in 16-bits + if (maxStrLen > 0x7fff) { + // Sanity check len can fit in 15-bits return 0; } // Can we use SSO here to avoid allocation? From 2e09d3459deab86460f58519b9c4c60043711166 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Mon, 28 Jan 2019 20:54:57 -0800 Subject: [PATCH 3/8] Add host test, make SSO 8 bytes, remove magic num Memory fragmentation is worse than not saving any RAM per String instance, so extend the SSO size to 8 bytes (7 chars + '\0'), and making the total String size the same size as it was before. Remove any hardcoded SSO size and allow configuration via an enum, and replace the magic max-capacity with an enum tor clarity. Add a host test that verifies that no memory is allocated until a full 8 characters are assigned to a string, as well as checking all intermediate values. --- cores/esp8266/WString.cpp | 20 +++++++++--------- cores/esp8266/WString.h | 9 ++++++--- tests/host/core/test_string.cpp | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 834572dd3b..876a7347cb 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -153,12 +153,8 @@ unsigned char String::reserve(unsigned int size) { } unsigned char String::changeBuffer(unsigned int maxStrLen) { - if (maxStrLen > 0x7fff) { - // Sanity check len can fit in 15-bits - return 0; - } // Can we use SSO here to avoid allocation? - if (maxStrLen < 4) { + if (maxStrLen < sizeof(sso_buff)) { if (sso || !bufptr) { // Already using SSO, nothing to do but set xap properly sso = true; @@ -166,7 +162,7 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { return 1; } else { // if bufptr // Using bufptr, need to shrink into sso_buff - char temp[4]; + char temp[sizeof(sso_buff)]; memcpy(temp, buffer(), maxStrLen); free(bufptr); sso = true; @@ -177,18 +173,22 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { } // Fallthrough to normal allocator size_t newSize = (maxStrLen + 16) & (~0xf); - char temp[4]; + // Make sure we can fit newsize in the buffer + if (newSize > CAPACITY_MAX) { + return false; + } + char temp[sizeof(sso_buff)]; if (buffer()) { - memcpy(temp, buffer(), 4); // Just in case SSO was on + memcpy(temp, buffer(), sizeof(sso_buff)); // Just in case SSO was on } else { - memset(temp, 0, 0); + memset(temp, 0, sizeof(sso_buff)); } char *newbuffer = (char *) realloc(sso ? nullptr : bufptr, newSize); if(newbuffer) { size_t oldSize = capacity + 1; // include NULL. if (sso) { // Copy the SSO buffer into allocated space - memcpy(newbuffer, temp, 4); + memcpy(newbuffer, temp, sizeof(sso_buff)); sso = false; } if (newSize > oldSize) diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index b04143616c..ae62ad8f4a 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -242,14 +242,17 @@ class String { long toInt(void) const; float toFloat(void) const; + enum { SSOSIZE = 8 }; // Characters to allocate space for SSO + protected: union { - char *bufptr; // the actual char array - char sso_buff[4]; // Overwrite the ptr with actual string for < 4 chars + char *bufptr; // the actual char array + char sso_buff[SSOSIZE]; // Overwrite the ptr with actual string for < SSOSIZE chars }; unsigned sso : 1; + enum { CAPACITY_MAX = 32767 }; // If size of capacity changed, be sure to update this enum unsigned capacity : 15; // the array length minus one (for the '\0') - unsigned unused : 1; + unsigned unused : 1; unsigned len : 15; // the String length (not counting the '\0') // Buffer accessor functions inline const char *buffer() const { return (const char *)(sso ? sso_buff : bufptr); } diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index ff13de8c79..8557b6aa57 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -268,3 +268,39 @@ TEST_CASE("String sizes near 8b", "[core][String]") REQUIRE(!strcmp(s16.c_str(),"123456789012345_")); REQUIRE(!strcmp(s17.c_str(),"1234567890123456_")); } + +TEST_CASE("String SSO works", "[core][String]") +{ + // This test assumes that SSO_SIZE==8, if that changes the test must as well + String s; + s += "0"; + REQUIRE(s == "0"); + const char *savesso = s.c_str(); + s += 1; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "01"); + s += "2"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "012"); + s += 3; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "0123"); + s += "4"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "01234"); + s += "5"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "012345"); + s += "6"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "0123456"); + s += "7"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "01234567"); + s += "8"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "012345678"); + s += "9"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789"); +} From cd71a4ad12e4b80e30745ec1accc6c9ad07de7a2 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Wed, 30 Jan 2019 22:15:13 -0800 Subject: [PATCH 4/8] Implement maximum SSO space possible Save up to 12 chars (11 + \0) in String itself by using the terminating \0 in the inline string as a flag to identify if this is a SSO or a heap string. Fix DOS endlines present in StreamString, for some reason. --- cores/esp8266/StreamString.cpp | 132 ++++++++--------- cores/esp8266/WString.cpp | 243 ++++++++++++++++---------------- cores/esp8266/WString.h | 38 +++-- tests/host/core/test_string.cpp | 28 +++- 4 files changed, 234 insertions(+), 207 deletions(-) diff --git a/cores/esp8266/StreamString.cpp b/cores/esp8266/StreamString.cpp index 106a05e60e..bb99232236 100644 --- a/cores/esp8266/StreamString.cpp +++ b/cores/esp8266/StreamString.cpp @@ -1,66 +1,66 @@ -/** - StreamString.cpp - - Copyright (c) 2015 Markus Sattler. All rights reserved. - This file is part of the esp8266 core for Arduino environment. - - This library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - This library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with this library; if not, write to the Free Software - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA - - */ - -#include -#include "StreamString.h" - -size_t StreamString::write(const uint8_t *data, size_t size) { - if(size && data) { - if(reserve(length() + size + 1)) { - memcpy((void *) (wbuffer() + len), (const void *) data, size); - len += size; - *(wbuffer() + len) = 0x00; // add null for string end - return size; - } - } - return 0; -} - -size_t StreamString::write(uint8_t data) { - return concat((char) data); -} - -int StreamString::available() { - return length(); -} - -int StreamString::read() { - if(length()) { - char c = charAt(0); - remove(0, 1); - return c; - - } - return -1; -} - -int StreamString::peek() { - if(length()) { - char c = charAt(0); - return c; - } - return -1; -} - -void StreamString::flush() { -} - +/** + StreamString.cpp + + Copyright (c) 2015 Markus Sattler. All rights reserved. + This file is part of the esp8266 core for Arduino environment. + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with this library; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + + */ + +#include +#include "StreamString.h" + +size_t StreamString::write(const uint8_t *data, size_t size) { + if(size && data) { + if(reserve(length() + size + 1)) { + memcpy((void *) (wbuffer() + len()), (const void *) data, size); + setLen(len() + size); + *(wbuffer() + len()) = 0x00; // add null for string end + return size; + } + } + return 0; +} + +size_t StreamString::write(uint8_t data) { + return concat((char) data); +} + +int StreamString::available() { + return length(); +} + +int StreamString::read() { + if(length()) { + char c = charAt(0); + remove(0, 1); + return c; + + } + return -1; +} + +int StreamString::peek() { + if(length()) { + char c = charAt(0); + return c; + } + return -1; +} + +void StreamString::flush() { +} + diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 876a7347cb..722ebe4722 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -129,23 +129,23 @@ String::~String() { // /*********************************************/ inline void String::init(void) { - bufptr = NULL; - capacity = 0; - len = 0; - sso = false; + setSSO(false); + ptr.buf = NULL; + setCapacity(0); + setLen(0); } void String::invalidate(void) { - if(!sso && bufptr) - free(bufptr); + if(!sso() && ptr.buf) + free(ptr.buf); init(); } unsigned char String::reserve(unsigned int size) { - if(buffer() && capacity >= size) + if(buffer() && capacity() >= size) return 1; if(changeBuffer(size)) { - if(len == 0) + if(len() == 0) wbuffer()[0] = 0; return 1; } @@ -154,20 +154,18 @@ unsigned char String::reserve(unsigned int size) { unsigned char String::changeBuffer(unsigned int maxStrLen) { // Can we use SSO here to avoid allocation? - if (maxStrLen < sizeof(sso_buff)) { - if (sso || !bufptr) { - // Already using SSO, nothing to do but set xap properly - sso = true; - capacity = sizeof(sso_buff) - 1; // We subtract off trailing 0 space from available + if (maxStrLen < sizeof(sso_buf)) { + if (sso() || !ptr.buf) { + // Already using SSO, nothing to do + setSSO(true); return 1; - } else { // if bufptr + } else { // if bufptr && !sso() // Using bufptr, need to shrink into sso_buff - char temp[sizeof(sso_buff)]; + char temp[sizeof(sso_buf)]; memcpy(temp, buffer(), maxStrLen); - free(bufptr); - sso = true; + free(ptr.buf); + setSSO(true); memcpy(wbuffer(), temp, maxStrLen); - capacity = sizeof(sso_buff) - 1; // We subtract off trailing 0 space from available return 1; } } @@ -177,26 +175,22 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { if (newSize > CAPACITY_MAX) { return false; } - char temp[sizeof(sso_buff)]; - if (buffer()) { - memcpy(temp, buffer(), sizeof(sso_buff)); // Just in case SSO was on - } else { - memset(temp, 0, sizeof(sso_buff)); - } - char *newbuffer = (char *) realloc(sso ? nullptr : bufptr, newSize); + uint16_t oldLen = len(); + char *newbuffer = (char *) realloc(sso() ? nullptr : ptr.buf, newSize); if(newbuffer) { - size_t oldSize = capacity + 1; // include NULL. - if (sso) { + size_t oldSize = capacity() + 1; // include NULL. + if (sso()) { // Copy the SSO buffer into allocated space - memcpy(newbuffer, temp, sizeof(sso_buff)); - sso = false; + memcpy(newbuffer, sso_buf, sizeof(sso_buf)); } + setSSO(false); if (newSize > oldSize) { memset(newbuffer + oldSize, 0, newSize - oldSize); } - capacity = newSize - 1; - bufptr = newbuffer; + setCapacity(newSize - 1); + setLen(oldLen); // Needed in case of SSO where len() never existed + ptr.buf = newbuffer; return 1; } return 0; @@ -211,7 +205,7 @@ String & String::copy(const char *cstr, unsigned int length) { invalidate(); return *this; } - len = length; + setLen(length); strcpy(wbuffer(), cstr); return *this; } @@ -221,7 +215,7 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) { invalidate(); return *this; } - len = length; + setLen(length); strcpy_P(wbuffer(), (PGM_P)pstr); return *this; } @@ -229,31 +223,32 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) { #ifdef __GXX_EXPERIMENTAL_CXX0X__ void String::move(String &rhs) { if(buffer()) { - if(capacity >= rhs.len) { + if(capacity() > rhs.len()) { strcpy(wbuffer(), rhs.buffer()); - len = rhs.len; - rhs.len = 0; + setLen(rhs.len()); + rhs.setLen(0); + // TODO - does this leak rhs.ptr.buf?? return; } else { - if (!sso) { - free(bufptr); - bufptr = nullptr; + if (!sso()) { + free(ptr.buf); + ptr.buf = nullptr; } } } - if (rhs.sso) { - sso = true; - memcpy(sso_buff, rhs.sso_buff, sizeof(sso_buff)); + if (rhs.sso()) { + setSSO(true); + memcpy(sso_buf, rhs.sso_buf, sizeof(sso_buf)); } else { - sso = false; - bufptr = rhs.bufptr; + setSSO(false); + ptr.buf = rhs.ptr.buf; } - capacity = rhs.capacity; - len = rhs.len; - rhs.bufptr = nullptr; - rhs.sso = false; - rhs.capacity = 0; - rhs.len = 0; + setCapacity(rhs.capacity()); + setLen(rhs.len()); + rhs.ptr.buf = nullptr; + rhs.setSSO(false); + rhs.setCapacity(0); + rhs.setLen(0); } #endif @@ -262,7 +257,7 @@ String & String::operator =(const String &rhs) { return *this; if(rhs.buffer()) - copy(rhs.buffer(), rhs.len); + copy(rhs.buffer(), rhs.len()); else invalidate(); @@ -308,32 +303,32 @@ unsigned char String::concat(const String &s) { // Special case if we're concatting ourself (s += s;) since we may end up // realloc'ing the buffer and moving s.buffer in the method called if (&s == this) { - unsigned int newlen = 2 * len; + unsigned int newlen = 2 * len(); if (!s.buffer()) return 0; - if (s.len == 0) + if (s.len() == 0) return 1; if (!reserve(newlen)) return 0; - memcpy(wbuffer() + len, buffer(), len); - len = newlen; - wbuffer()[len] = 0; + memcpy(wbuffer() + len(), buffer(), len()); + setLen(newlen); + wbuffer()[len()] = 0; return 1; } else { - return concat(s.buffer(), s.len); + return concat(s.buffer(), s.len()); } } unsigned char String::concat(const char *cstr, unsigned int length) { - unsigned int newlen = len + length; + unsigned int newlen = len() + length; if(!cstr) return 0; if(length == 0) return 1; if(!reserve(newlen)) return 0; - strcpy(wbuffer() + len, cstr); - len = newlen; + strcpy(wbuffer() + len(), cstr); + setLen(newlen); return 1; } @@ -396,10 +391,10 @@ unsigned char String::concat(const __FlashStringHelper * str) { if (!str) return 0; int length = strlen_P((PGM_P)str); if (length == 0) return 1; - unsigned int newlen = len + length; + unsigned int newlen = len() + length; if (!reserve(newlen)) return 0; - strcpy_P(wbuffer() + len, (PGM_P)str); - len = newlen; + strcpy_P(wbuffer() + len(), (PGM_P)str); + setLen(newlen); return 1; } @@ -409,7 +404,7 @@ unsigned char String::concat(const __FlashStringHelper * str) { StringSumHelper & operator +(const StringSumHelper &lhs, const String &rhs) { StringSumHelper &a = const_cast(lhs); - if(!a.concat(rhs.buffer(), rhs.len)) + if(!a.concat(rhs.buffer(), rhs.len())) a.invalidate(); return a; } @@ -490,9 +485,9 @@ StringSumHelper & operator + (const StringSumHelper &lhs, const __FlashStringHel int String::compareTo(const String &s) const { if(!buffer() || !s.buffer()) { - if(s.buffer() && s.len > 0) + if(s.buffer() && s.len() > 0) return 0 - *(unsigned char *) s.buffer(); - if(buffer() && len > 0) + if(buffer() && len() > 0) return *(unsigned char *) buffer(); return 0; } @@ -500,11 +495,11 @@ int String::compareTo(const String &s) const { } unsigned char String::equals(const String &s2) const { - return (len == s2.len && compareTo(s2) == 0); + return (len() == s2.len() && compareTo(s2) == 0); } unsigned char String::equals(const char *cstr) const { - if(len == 0) + if(len() == 0) return (cstr == NULL || *cstr == 0); if(cstr == NULL) return buffer()[0] == 0; @@ -530,9 +525,9 @@ unsigned char String::operator>=(const String &rhs) const { unsigned char String::equalsIgnoreCase(const String &s2) const { if(this == &s2) return 1; - if(len != s2.len) + if(len() != s2.len()) return 0; - if(len == 0) + if(len() == 0) return 1; const char *p1 = buffer(); const char *p2 = s2.buffer(); @@ -546,10 +541,10 @@ unsigned char String::equalsIgnoreCase(const String &s2) const { unsigned char String::equalsConstantTime(const String &s2) const { // To avoid possible time-based attacks present function // compares given strings in a constant time. - if(len != s2.len) + if(len() != s2.len()) return 0; //at this point lengths are the same - if(len == 0) + if(len() == 0) return 1; //at this point lenghts are the same and non-zero const char *p1 = buffer(); @@ -565,27 +560,27 @@ unsigned char String::equalsConstantTime(const String &s2) const { ++p2; } //the following should force a constant time eval of the condition without a compiler "logical shortcut" - unsigned char equalcond = (equalchars == len); + unsigned char equalcond = (equalchars == len()); unsigned char diffcond = (diffchars == 0); return (equalcond & diffcond); //bitwise AND } unsigned char String::startsWith(const String &s2) const { - if(len < s2.len) + if(len() < s2.len()) return 0; return startsWith(s2, 0); } unsigned char String::startsWith(const String &s2, unsigned int offset) const { - if(offset > (unsigned)(len - s2.len) || !buffer() || !s2.buffer()) + if(offset > (unsigned)(len() - s2.len()) || !buffer() || !s2.buffer()) return 0; - return strncmp(&buffer()[offset], s2.buffer(), s2.len) == 0; + return strncmp(&buffer()[offset], s2.buffer(), s2.len()) == 0; } unsigned char String::endsWith(const String &s2) const { - if(len < s2.len || !buffer() || !s2.buffer()) + if(len() < s2.len() || !buffer() || !s2.buffer()) return 0; - return strcmp(&buffer()[len - s2.len], s2.buffer()) == 0; + return strcmp(&buffer()[len() - s2.len()], s2.buffer()) == 0; } // /*********************************************/ @@ -597,13 +592,13 @@ char String::charAt(unsigned int loc) const { } void String::setCharAt(unsigned int loc, char c) { - if(loc < len) + if(loc < len()) wbuffer()[loc] = c; } char & String::operator[](unsigned int index) { static char dummy_writable_char; - if(index >= len || !buffer()) { + if(index >= len() || !buffer()) { dummy_writable_char = 0; return dummy_writable_char; } @@ -611,7 +606,7 @@ char & String::operator[](unsigned int index) { } char String::operator[](unsigned int index) const { - if(index >= len || !buffer()) + if(index >= len() || !buffer()) return 0; return buffer()[index]; } @@ -619,13 +614,13 @@ char String::operator[](unsigned int index) const { void String::getBytes(unsigned char *buf, unsigned int bufsize, unsigned int index) const { if(!bufsize || !buf) return; - if(index >= len) { + if(index >= len()) { buf[0] = 0; return; } unsigned int n = bufsize - 1; - if(n > len - index) - n = len - index; + if(n > len() - index) + n = len() - index; strncpy((char *) buf, buffer() + index, n); buf[n] = 0; } @@ -639,7 +634,7 @@ int String::indexOf(char c) const { } int String::indexOf(char ch, unsigned int fromIndex) const { - if(fromIndex >= len) + if(fromIndex >= len()) return -1; const char* temp = strchr(buffer() + fromIndex, ch); if(temp == NULL) @@ -652,7 +647,7 @@ int String::indexOf(const String &s2) const { } int String::indexOf(const String &s2, unsigned int fromIndex) const { - if(fromIndex >= len) + if(fromIndex >= len()) return -1; const char *found = strstr(buffer() + fromIndex, s2.buffer()); if(found == NULL) @@ -661,11 +656,11 @@ int String::indexOf(const String &s2, unsigned int fromIndex) const { } int String::lastIndexOf(char theChar) const { - return lastIndexOf(theChar, len - 1); + return lastIndexOf(theChar, len() - 1); } int String::lastIndexOf(char ch, unsigned int fromIndex) const { - if(fromIndex >= len) + if(fromIndex >= len()) return -1; char tempchar = buffer()[fromIndex + 1]; wbuffer()[fromIndex + 1] = '\0'; @@ -677,14 +672,14 @@ int String::lastIndexOf(char ch, unsigned int fromIndex) const { } int String::lastIndexOf(const String &s2) const { - return lastIndexOf(s2, len - s2.len); + return lastIndexOf(s2, len() - s2.len()); } int String::lastIndexOf(const String &s2, unsigned int fromIndex) const { - if(s2.len == 0 || len == 0 || s2.len > len) + if(s2.len() == 0 || len() == 0 || s2.len() > len()) return -1; - if(fromIndex >= len) - fromIndex = len - 1; + if(fromIndex >= len()) + fromIndex = len() - 1; int found = -1; for(char *p = wbuffer(); p <= wbuffer() + fromIndex; p++) { p = strstr(p, s2.buffer()); @@ -703,10 +698,10 @@ String String::substring(unsigned int left, unsigned int right) const { left = temp; } String out; - if(left >= len) + if(left >= len()) return out; - if(right > len) - right = len; + if(right > len()) + right = len(); char temp = buffer()[right]; // save the replaced character wbuffer()[right] = '\0'; out = wbuffer() + left; // pointer arithmetic @@ -728,15 +723,15 @@ void String::replace(char find, char replace) { } void String::replace(const String& find, const String& replace) { - if(len == 0 || find.len == 0) + if(len() == 0 || find.len() == 0) return; - int diff = replace.len - find.len; + int diff = replace.len() - find.len(); char *readFrom = wbuffer(); char *foundAt; if(diff == 0) { while((foundAt = strstr(readFrom, find.buffer())) != NULL) { - memcpy(foundAt, replace.buffer(), replace.len); - readFrom = foundAt + replace.len; + memcpy(foundAt, replace.buffer(), replace.len()); + readFrom = foundAt + replace.len(); } } else if(diff < 0) { char *writeTo = wbuffer(); @@ -744,29 +739,29 @@ void String::replace(const String& find, const String& replace) { unsigned int n = foundAt - readFrom; memcpy(writeTo, readFrom, n); writeTo += n; - memcpy(writeTo, replace.buffer(), replace.len); - writeTo += replace.len; - readFrom = foundAt + find.len; - len += diff; + memcpy(writeTo, replace.buffer(), replace.len()); + writeTo += replace.len(); + readFrom = foundAt + find.len(); + setLen(len() + diff); } strcpy(writeTo, readFrom); } else { - unsigned int size = len; // compute size needed for result + unsigned int size = len(); // compute size needed for result while((foundAt = strstr(readFrom, find.buffer())) != NULL) { - readFrom = foundAt + find.len; + readFrom = foundAt + find.len(); size += diff; } - if(size == len) + if(size == len()) return; - if(size > capacity && !changeBuffer(size)) + if(size > capacity() && !changeBuffer(size)) return; // XXX: tell user! - int index = len - 1; + int index = len() - 1; while(index >= 0 && (index = lastIndexOf(find, index)) >= 0) { - readFrom = wbuffer() + index + find.len; - memmove(readFrom + diff, readFrom, len - (readFrom - buffer())); - len += diff; - wbuffer()[len] = 0; - memcpy(wbuffer() + index, replace.buffer(), replace.len); + readFrom = wbuffer() + index + find.len(); + memmove(readFrom + diff, readFrom, len() - (readFrom - buffer())); + setLen(len() + diff); + wbuffer()[len()] = 0; + memcpy(wbuffer() + index, replace.buffer(), replace.len()); index--; } } @@ -780,19 +775,19 @@ void String::remove(unsigned int index) { } void String::remove(unsigned int index, unsigned int count) { - if(index >= len) { + if(index >= len()) { return; } if(count <= 0) { return; } - if(count > len - index) { - count = len - index; + if(count > len() - index) { + count = len() - index; } char *writeTo = wbuffer() + index; - len = len - count; - memmove(writeTo, wbuffer() + index + count, len - index); - wbuffer()[len] = 0; + setLen(len() - count); + memmove(writeTo, wbuffer() + index + count, len() - index); + wbuffer()[len()] = 0; } void String::toLowerCase(void) { @@ -812,18 +807,18 @@ void String::toUpperCase(void) { } void String::trim(void) { - if(!buffer() || len == 0) + if(!buffer() || len() == 0) return; char *begin = wbuffer(); while(isspace(*begin)) begin++; - char *end = wbuffer() + len - 1; + char *end = wbuffer() + len() - 1; while(isspace(*end) && end >= begin) end--; - len = end + 1 - begin; + setLen(end + 1 - begin); if(begin > buffer()) - memmove(wbuffer(), begin, len); - wbuffer()[len] = 0; + memmove(wbuffer(), begin, len()); + wbuffer()[len()] = 0; } // /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index ae62ad8f4a..1652695b62 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -77,7 +77,7 @@ class String { unsigned char reserve(unsigned int size); inline unsigned int length(void) const { if(buffer()) { - return len; + return len(); } else { return 0; } @@ -224,7 +224,7 @@ class String { int lastIndexOf(const String &str) const; int lastIndexOf(const String &str, unsigned int fromIndex) const; String substring(unsigned int beginIndex) const { - return substring(beginIndex, len); + return substring(beginIndex, len()); } ; String substring(unsigned int beginIndex, unsigned int endIndex) const; @@ -242,21 +242,31 @@ class String { long toInt(void) const; float toFloat(void) const; - enum { SSOSIZE = 8 }; // Characters to allocate space for SSO - protected: - union { - char *bufptr; // the actual char array - char sso_buff[SSOSIZE]; // Overwrite the ptr with actual string for < SSOSIZE chars + // SSO is handled by checking the last byte of sso_buff. + // When not in SSO mode, that byte is set to 0xff, while when in SSO mode it is always 0x00 (so it can serve as the string terminator as well as a flag) + // This allows strings up up to 12 (11 + \0 termination) without any extra space. + enum { SSOSIZE = 12 }; // Characters to allocate space for SSO, must be 12 or more + enum { CAPACITY_MAX = 65535 }; // If size of capacity changed, be sure to update this enum + union { + struct { + uint16_t cap; + uint16_t len; + char * buf; + } ptr; + char sso_buf[SSOSIZE]; }; - unsigned sso : 1; - enum { CAPACITY_MAX = 32767 }; // If size of capacity changed, be sure to update this enum - unsigned capacity : 15; // the array length minus one (for the '\0') - unsigned unused : 1; - unsigned len : 15; // the String length (not counting the '\0') + // Accessor functions + inline bool sso() const { return sso_buf[SSOSIZE - 1] == 0; } + inline unsigned int len() const { return sso() ? strlen(sso_buf) : ptr.len; } + inline unsigned int capacity() const { return sso() ? SSOSIZE - 1 : ptr.cap; } + inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; } + inline void setLen(int len) { if (!sso()) ptr.len = len; } + inline void setCapacity(int cap) { if (!sso()) ptr.cap = cap; } // Buffer accessor functions - inline const char *buffer() const { return (const char *)(sso ? sso_buff : bufptr); } - inline char *wbuffer() const { return sso ? const_cast(&sso_buff[0]) : bufptr; } // Writable version of buffer + inline const char *buffer() const { return (const char *)(sso() ? sso_buf : ptr.buf); } + inline char *wbuffer() const { return sso() ? const_cast(sso_buf) : ptr.buf; } // Writable version of buffer + protected: void init(void); void invalidate(void); diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index 8557b6aa57..fc59e02f22 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -275,32 +275,54 @@ TEST_CASE("String SSO works", "[core][String]") String s; s += "0"; REQUIRE(s == "0"); + REQUIRE(s.length() == 1); const char *savesso = s.c_str(); s += 1; REQUIRE(s.c_str() == savesso); REQUIRE(s == "01"); + REQUIRE(s.length() == 2); s += "2"; REQUIRE(s.c_str() == savesso); REQUIRE(s == "012"); + REQUIRE(s.length() == 3); s += 3; REQUIRE(s.c_str() == savesso); REQUIRE(s == "0123"); + REQUIRE(s.length() == 4); s += "4"; REQUIRE(s.c_str() == savesso); REQUIRE(s == "01234"); + REQUIRE(s.length() == 5); s += "5"; REQUIRE(s.c_str() == savesso); REQUIRE(s == "012345"); + REQUIRE(s.length() == 6); s += "6"; REQUIRE(s.c_str() == savesso); REQUIRE(s == "0123456"); + REQUIRE(s.length() == 7); s += "7"; - REQUIRE(s.c_str() != savesso); + REQUIRE(s.c_str() == savesso); REQUIRE(s == "01234567"); + REQUIRE(s.length() == 8); s += "8"; - REQUIRE(s.c_str() != savesso); + REQUIRE(s.c_str() == savesso); REQUIRE(s == "012345678"); + REQUIRE(s.length() == 9); s += "9"; - REQUIRE(s.c_str() != savesso); + REQUIRE(s.c_str() == savesso); REQUIRE(s == "0123456789"); + REQUIRE(s.length() == 10); + s += "a"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "0123456789a"); + REQUIRE(s.length() == 11); + s += "b"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789ab"); + REQUIRE(s.length() == 12); + s += "c"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abc"); + REQUIRE(s.length() == 13); } From c0529f6f36472f1bdf5c9c2a6da9a5d9cd04eb1f Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 31 Jan 2019 06:55:30 -0800 Subject: [PATCH 5/8] Fix ordering of variable setup for CI --- cores/esp8266/WString.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 722ebe4722..2a72054d16 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -130,9 +130,9 @@ String::~String() { inline void String::init(void) { setSSO(false); - ptr.buf = NULL; setCapacity(0); setLen(0); + ptr.buf = NULL; } void String::invalidate(void) { @@ -183,13 +183,13 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { // Copy the SSO buffer into allocated space memcpy(newbuffer, sso_buf, sizeof(sso_buf)); } - setSSO(false); if (newSize > oldSize) { memset(newbuffer + oldSize, 0, newSize - oldSize); } + setSSO(false); setCapacity(newSize - 1); - setLen(oldLen); // Needed in case of SSO where len() never existed + setLen(oldLen); // Needed in case of SSO where len() never existed ptr.buf = newbuffer; return 1; } @@ -245,10 +245,10 @@ void String::move(String &rhs) { } setCapacity(rhs.capacity()); setLen(rhs.len()); - rhs.ptr.buf = nullptr; rhs.setSSO(false); rhs.setCapacity(0); rhs.setLen(0); + rhs.ptr.buf = nullptr; } #endif From a5a18e1ca07f77a4be46bcdabae84a86157a4111 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 31 Jan 2019 07:15:09 -0800 Subject: [PATCH 6/8] Clean up ::move assignment operator --- cores/esp8266/WString.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 2a72054d16..2e344cf8e0 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -223,11 +223,14 @@ String & String::copy(const __FlashStringHelper *pstr, unsigned int length) { #ifdef __GXX_EXPERIMENTAL_CXX0X__ void String::move(String &rhs) { if(buffer()) { - if(capacity() > rhs.len()) { + if(capacity() >= rhs.len()) { strcpy(wbuffer(), rhs.buffer()); setLen(rhs.len()); + if (!rhs.sso()) free(rhs.ptr.buf); + rhs.setSSO(false); + rhs.setCapacity(0); rhs.setLen(0); - // TODO - does this leak rhs.ptr.buf?? + rhs.ptr.buf = nullptr; return; } else { if (!sso()) { From f2371d94726e8adad0c123921921e3f9b050a90b Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 31 Jan 2019 08:37:43 -0800 Subject: [PATCH 7/8] Fix tab/space issues --- cores/esp8266/WString.cpp | 9 +++------ cores/esp8266/WString.h | 12 ++++++------ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index 2e344cf8e0..dce5710d1c 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -226,11 +226,7 @@ void String::move(String &rhs) { if(capacity() >= rhs.len()) { strcpy(wbuffer(), rhs.buffer()); setLen(rhs.len()); - if (!rhs.sso()) free(rhs.ptr.buf); - rhs.setSSO(false); - rhs.setCapacity(0); - rhs.setLen(0); - rhs.ptr.buf = nullptr; + rhs.invalidate(); return; } else { if (!sso()) { @@ -478,7 +474,8 @@ StringSumHelper & operator +(const StringSumHelper &lhs, double num) { StringSumHelper & operator + (const StringSumHelper &lhs, const __FlashStringHelper *rhs) { StringSumHelper &a = const_cast(lhs); - if (!a.concat(rhs)) a.invalidate(); + if (!a.concat(rhs)) + a.invalidate(); return a; } diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 1652695b62..73977d43e1 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -249,12 +249,12 @@ class String { enum { SSOSIZE = 12 }; // Characters to allocate space for SSO, must be 12 or more enum { CAPACITY_MAX = 65535 }; // If size of capacity changed, be sure to update this enum union { - struct { - uint16_t cap; - uint16_t len; - char * buf; - } ptr; - char sso_buf[SSOSIZE]; + struct { + uint16_t cap; + uint16_t len; + char * buf; + } ptr; + char sso_buf[SSOSIZE]; }; // Accessor functions inline bool sso() const { return sso_buf[SSOSIZE - 1] == 0; } From b1992331a815f9c6a95a0c56c741a16c3e2cfd25 Mon Sep 17 00:00:00 2001 From: "Earle F. Philhower, III" Date: Thu, 31 Jan 2019 09:36:46 -0800 Subject: [PATCH 8/8] Fix memory corruption on x64 Because pointers are 8 bytes (and 8-bytes aligned) on x64, the structure used to store SSO needs to make sure the terminal \0 won't overwrite some of that buffer. Adjust the SSOSIZE dynamically depending on the size of the struct ptr. Refactor to add a setBuffer method, like the setConfig, setSSO, etc. Fix some issues found when using SSO strings and functions which modify len() dynamically. --- cores/esp8266/WString.cpp | 36 ++++++++++++----------- cores/esp8266/WString.h | 20 +++++++------ tests/host/core/test_string.cpp | 51 +++++++++++++++++++++++++++------ 3 files changed, 74 insertions(+), 33 deletions(-) diff --git a/cores/esp8266/WString.cpp b/cores/esp8266/WString.cpp index dce5710d1c..4d4fa3bf21 100644 --- a/cores/esp8266/WString.cpp +++ b/cores/esp8266/WString.cpp @@ -132,12 +132,12 @@ inline void String::init(void) { setSSO(false); setCapacity(0); setLen(0); - ptr.buf = NULL; + setBuffer(nullptr); } void String::invalidate(void) { - if(!sso() && ptr.buf) - free(ptr.buf); + if(!sso() && wbuffer()) + free(wbuffer()); init(); } @@ -155,7 +155,7 @@ unsigned char String::reserve(unsigned int size) { unsigned char String::changeBuffer(unsigned int maxStrLen) { // Can we use SSO here to avoid allocation? if (maxStrLen < sizeof(sso_buf)) { - if (sso() || !ptr.buf) { + if (sso() || !buffer()) { // Already using SSO, nothing to do setSSO(true); return 1; @@ -163,7 +163,7 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { // Using bufptr, need to shrink into sso_buff char temp[sizeof(sso_buf)]; memcpy(temp, buffer(), maxStrLen); - free(ptr.buf); + free(wbuffer()); setSSO(true); memcpy(wbuffer(), temp, maxStrLen); return 1; @@ -176,7 +176,7 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { return false; } uint16_t oldLen = len(); - char *newbuffer = (char *) realloc(sso() ? nullptr : ptr.buf, newSize); + char *newbuffer = (char *) realloc(sso() ? nullptr : wbuffer(), newSize); if(newbuffer) { size_t oldSize = capacity() + 1; // include NULL. if (sso()) { @@ -190,7 +190,7 @@ unsigned char String::changeBuffer(unsigned int maxStrLen) { setSSO(false); setCapacity(newSize - 1); setLen(oldLen); // Needed in case of SSO where len() never existed - ptr.buf = newbuffer; + setBuffer(newbuffer); return 1; } return 0; @@ -230,8 +230,8 @@ void String::move(String &rhs) { return; } else { if (!sso()) { - free(ptr.buf); - ptr.buf = nullptr; + free(wbuffer()); + setBuffer(nullptr); } } } @@ -240,14 +240,14 @@ void String::move(String &rhs) { memcpy(sso_buf, rhs.sso_buf, sizeof(sso_buf)); } else { setSSO(false); - ptr.buf = rhs.ptr.buf; + setBuffer(rhs.wbuffer()); } setCapacity(rhs.capacity()); setLen(rhs.len()); rhs.setSSO(false); rhs.setCapacity(0); rhs.setLen(0); - rhs.ptr.buf = nullptr; + rhs.setBuffer(nullptr); } #endif @@ -785,9 +785,10 @@ void String::remove(unsigned int index, unsigned int count) { count = len() - index; } char *writeTo = wbuffer() + index; - setLen(len() - count); - memmove(writeTo, wbuffer() + index + count, len() - index); - wbuffer()[len()] = 0; + unsigned int newlen = len() - count; + setLen(newlen); + memmove(writeTo, wbuffer() + index + count, newlen - index); + wbuffer()[newlen] = 0; } void String::toLowerCase(void) { @@ -815,10 +816,11 @@ void String::trim(void) { char *end = wbuffer() + len() - 1; while(isspace(*end) && end >= begin) end--; - setLen(end + 1 - begin); + unsigned int newlen = end + 1 - begin; + setLen(newlen); if(begin > buffer()) - memmove(wbuffer(), begin, len()); - wbuffer()[len()] = 0; + memmove(wbuffer(), begin, newlen); + wbuffer()[newlen] = 0; } // /*********************************************/ diff --git a/cores/esp8266/WString.h b/cores/esp8266/WString.h index 73977d43e1..aecf9b4a62 100644 --- a/cores/esp8266/WString.h +++ b/cores/esp8266/WString.h @@ -243,17 +243,20 @@ class String { float toFloat(void) const; protected: + // Contains the string info when we're not in SSO mode + struct _ptr { + char * buff; + uint16_t cap; + uint16_t len; + }; + // SSO is handled by checking the last byte of sso_buff. // When not in SSO mode, that byte is set to 0xff, while when in SSO mode it is always 0x00 (so it can serve as the string terminator as well as a flag) // This allows strings up up to 12 (11 + \0 termination) without any extra space. - enum { SSOSIZE = 12 }; // Characters to allocate space for SSO, must be 12 or more + enum { SSOSIZE = sizeof(struct _ptr) + 4 }; // Characters to allocate space for SSO, must be 12 or more enum { CAPACITY_MAX = 65535 }; // If size of capacity changed, be sure to update this enum union { - struct { - uint16_t cap; - uint16_t len; - char * buf; - } ptr; + struct _ptr ptr; char sso_buf[SSOSIZE]; }; // Accessor functions @@ -263,9 +266,10 @@ class String { inline void setSSO(bool sso) { sso_buf[SSOSIZE - 1] = sso ? 0x00 : 0xff; } inline void setLen(int len) { if (!sso()) ptr.len = len; } inline void setCapacity(int cap) { if (!sso()) ptr.cap = cap; } + inline void setBuffer(char *buff) { if (!sso()) ptr.buff = buff; } // Buffer accessor functions - inline const char *buffer() const { return (const char *)(sso() ? sso_buf : ptr.buf); } - inline char *wbuffer() const { return sso() ? const_cast(sso_buf) : ptr.buf; } // Writable version of buffer + inline const char *buffer() const { return (const char *)(sso() ? sso_buf : ptr.buff); } + inline char *wbuffer() const { return sso() ? const_cast(sso_buf) : ptr.buff; } // Writable version of buffer protected: void init(void); diff --git a/tests/host/core/test_string.cpp b/tests/host/core/test_string.cpp index fc59e02f22..c2ef68025f 100644 --- a/tests/host/core/test_string.cpp +++ b/tests/host/core/test_string.cpp @@ -317,12 +317,47 @@ TEST_CASE("String SSO works", "[core][String]") REQUIRE(s.c_str() == savesso); REQUIRE(s == "0123456789a"); REQUIRE(s.length() == 11); - s += "b"; - REQUIRE(s.c_str() != savesso); - REQUIRE(s == "0123456789ab"); - REQUIRE(s.length() == 12); - s += "c"; - REQUIRE(s.c_str() != savesso); - REQUIRE(s == "0123456789abc"); - REQUIRE(s.length() == 13); + if (sizeof(savesso) == 4) { + s += "b"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789ab"); + REQUIRE(s.length() == 12); + s += "c"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abc"); + REQUIRE(s.length() == 13); + } else { + s += "bcde"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "0123456789abcde"); + REQUIRE(s.length() == 15); + s += "fghi"; + REQUIRE(s.c_str() == savesso); + REQUIRE(s == "0123456789abcdefghi"); + REQUIRE(s.length() == 19); + s += "j"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghij"); + REQUIRE(s.length() == 20); + s += "k"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghijk"); + REQUIRE(s.length() == 21); + s += "l"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghijkl"); + REQUIRE(s.length() == 22); + s += "m"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghijklm"); + REQUIRE(s.length() == 23); + s += "nopq"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghijklmnopq"); + REQUIRE(s.length() == 27); + s += "rstu"; + REQUIRE(s.c_str() != savesso); + REQUIRE(s == "0123456789abcdefghijklmnopqrstu"); + REQUIRE(s.length() == 31); + } }