From 868317b3fd765830c07ecf16cbfcf6c7708adc3c Mon Sep 17 00:00:00 2001 From: Kostya Kortchinsky Date: Thu, 3 Jun 2021 12:11:05 -0700 Subject: [PATCH] [scudo] Rework Vector/String Some platforms (eg: Trusty) are extremelly memory constrained, which doesn't necessarily work well with some of Scudo's current assumptions. `Vector` by default (and as such `String` and `ScopedString`) maps a page, which is a bit of a waste. This CL changes `Vector` to use a buffer local to the class first, then potentially map more memory if needed (`ScopedString` currently are all stack based so it would be stack data). We also want to allow a platform to prevent any dynamic resizing, so I added a `CanGrow` templated parameter that for now is always `true` but would be set to `false` on Trusty. Differential Revision: https://reviews.llvm.org/D103641 --- compiler-rt/lib/scudo/standalone/combined.h | 4 +-- compiler-rt/lib/scudo/standalone/primary64.h | 2 +- compiler-rt/lib/scudo/standalone/report.cpp | 2 +- .../lib/scudo/standalone/size_class_map.h | 2 +- .../lib/scudo/standalone/string_utils.cpp | 6 ++-- .../lib/scudo/standalone/string_utils.h | 7 +++-- .../scudo/standalone/tests/primary_test.cpp | 8 ++--- .../standalone/tests/quarantine_test.cpp | 4 +-- .../scudo/standalone/tests/secondary_test.cpp | 8 ++--- .../scudo/standalone/tests/strings_test.cpp | 8 ++--- .../scudo/standalone/tests/vector_test.cpp | 14 ++++----- compiler-rt/lib/scudo/standalone/vector.h | 31 +++++++++---------- 12 files changed, 47 insertions(+), 49 deletions(-) diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h index 8caffcc533c0..f58eaa945af3 100644 --- a/compiler-rt/lib/scudo/standalone/combined.h +++ b/compiler-rt/lib/scudo/standalone/combined.h @@ -694,7 +694,7 @@ public: // function. This can be called with a null buffer or zero size for buffer // sizing purposes. uptr getStats(char *Buffer, uptr Size) { - ScopedString Str(1024); + ScopedString Str; disable(); const uptr Length = getStats(&Str) + 1; enable(); @@ -708,7 +708,7 @@ public: } void printStats() { - ScopedString Str(1024); + ScopedString Str; disable(); getStats(&Str); enable(); diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h index 0a43f46bb2c7..a3456b6b99c1 100644 --- a/compiler-rt/lib/scudo/standalone/primary64.h +++ b/compiler-rt/lib/scudo/standalone/primary64.h @@ -341,7 +341,7 @@ private: if (UNLIKELY(RegionBase + MappedUser + MapSize > RegionSize)) { if (!Region->Exhausted) { Region->Exhausted = true; - ScopedString Str(1024); + ScopedString Str; getStats(&Str); Str.append( "Scudo OOM: The process has exhausted %zuM for size class %zu.\n", diff --git a/compiler-rt/lib/scudo/standalone/report.cpp b/compiler-rt/lib/scudo/standalone/report.cpp index 292c29918d1a..561c7c51f4e1 100644 --- a/compiler-rt/lib/scudo/standalone/report.cpp +++ b/compiler-rt/lib/scudo/standalone/report.cpp @@ -17,7 +17,7 @@ namespace scudo { class ScopedErrorReport { public: - ScopedErrorReport() : Message(512) { Message.append("Scudo ERROR: "); } + ScopedErrorReport() : Message() { Message.append("Scudo ERROR: "); } void append(const char *Format, ...) { va_list Args; va_start(Args, Format); diff --git a/compiler-rt/lib/scudo/standalone/size_class_map.h b/compiler-rt/lib/scudo/standalone/size_class_map.h index 2736e995dbab..e1c2edac4763 100644 --- a/compiler-rt/lib/scudo/standalone/size_class_map.h +++ b/compiler-rt/lib/scudo/standalone/size_class_map.h @@ -309,7 +309,7 @@ struct SvelteSizeClassConfig { typedef FixedSizeClassMap SvelteSizeClassMap; template inline void printMap() { - ScopedString Buffer(1024); + ScopedString Buffer; uptr PrevS = 0; uptr TotalCached = 0; for (uptr I = 0; I < SCMap::NumClasses; I++) { diff --git a/compiler-rt/lib/scudo/standalone/string_utils.cpp b/compiler-rt/lib/scudo/standalone/string_utils.cpp index 25bddbce34d9..ca80834cca87 100644 --- a/compiler-rt/lib/scudo/standalone/string_utils.cpp +++ b/compiler-rt/lib/scudo/standalone/string_utils.cpp @@ -219,7 +219,7 @@ int formatString(char *Buffer, uptr BufferLength, const char *Format, ...) { } void ScopedString::append(const char *Format, va_list Args) { - DCHECK_LT(Length, String.size()); + RAW_CHECK(Length <= String.size()); va_list ArgsCopy; va_copy(ArgsCopy, Args); // formatString doesn't currently support a null buffer or zero buffer length, @@ -232,7 +232,7 @@ void ScopedString::append(const char *Format, va_list Args) { formatString(String.data() + Length, AdditionalLength, Format, ArgsCopy); va_end(ArgsCopy); Length = strlen(String.data()); - CHECK_LT(Length, String.size()); + RAW_CHECK(Length < String.size()); } FORMAT(2, 3) @@ -247,7 +247,7 @@ FORMAT(1, 2) void Printf(const char *Format, ...) { va_list Args; va_start(Args, Format); - ScopedString Msg(1024); + ScopedString Msg; Msg.append(Format, Args); outputRaw(Msg.data()); va_end(Args); diff --git a/compiler-rt/lib/scudo/standalone/string_utils.h b/compiler-rt/lib/scudo/standalone/string_utils.h index 4880fa1e7cf1..7a57da3a53f4 100644 --- a/compiler-rt/lib/scudo/standalone/string_utils.h +++ b/compiler-rt/lib/scudo/standalone/string_utils.h @@ -18,8 +18,9 @@ namespace scudo { class ScopedString { public: - explicit ScopedString(uptr MaxLength) : String(MaxLength), Length(0) { - String[0] = '\0'; + explicit ScopedString() : String() { + if (String.capacity() > 0) + String[0] = '\0'; } uptr length() { return Length; } const char *data() { return String.data(); } @@ -33,7 +34,7 @@ public: private: Vector String; - uptr Length; + uptr Length = 0; }; int formatString(char *Buffer, uptr BufferLength, const char *Format, ...); diff --git a/compiler-rt/lib/scudo/standalone/tests/primary_test.cpp b/compiler-rt/lib/scudo/standalone/tests/primary_test.cpp index 3574c28c6c09..93157f942c54 100644 --- a/compiler-rt/lib/scudo/standalone/tests/primary_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/primary_test.cpp @@ -132,7 +132,7 @@ SCUDO_TYPED_TEST(ScudoPrimaryTest, BasicPrimary) { } Cache.destroy(nullptr); Allocator->releaseToOS(); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Allocator->getStats(&Str); Str.output(); } @@ -178,7 +178,7 @@ TEST(ScudoPrimaryTest, Primary64OOM) { } Cache.destroy(nullptr); Allocator.releaseToOS(); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Allocator.getStats(&Str); Str.output(); EXPECT_EQ(AllocationFailed, true); @@ -216,7 +216,7 @@ SCUDO_TYPED_TEST(ScudoPrimaryTest, PrimaryIterate) { } Cache.destroy(nullptr); Allocator->releaseToOS(); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Allocator->getStats(&Str); Str.output(); } @@ -263,7 +263,7 @@ SCUDO_TYPED_TEST(ScudoPrimaryTest, PrimaryThreaded) { for (auto &T : Threads) T.join(); Allocator->releaseToOS(); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Allocator->getStats(&Str); Str.output(); } diff --git a/compiler-rt/lib/scudo/standalone/tests/quarantine_test.cpp b/compiler-rt/lib/scudo/standalone/tests/quarantine_test.cpp index 91de56a78c97..972c98d510a9 100644 --- a/compiler-rt/lib/scudo/standalone/tests/quarantine_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/quarantine_test.cpp @@ -214,7 +214,7 @@ TEST(ScudoQuarantineTest, GlobalQuarantine) { Quarantine.drainAndRecycle(&Cache, Cb); EXPECT_EQ(Cache.getSize(), 0UL); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Quarantine.getStats(&Str); Str.output(); } @@ -246,7 +246,7 @@ TEST(ScudoQuarantineTest, ThreadedGlobalQuarantine) { for (scudo::uptr I = 0; I < NumberOfThreads; I++) pthread_join(T[I].Thread, 0); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; Quarantine.getStats(&Str); Str.output(); diff --git a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp index 2320a4d546ab..2dc041b94a8c 100644 --- a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp @@ -64,7 +64,7 @@ template static void testSecondaryBasic(void) { L->deallocate(scudo::Options{}, V.back()); V.pop_back(); } - scudo::ScopedString Str(1024); + scudo::ScopedString Str; L->getStats(&Str); Str.output(); L->unmapTestOnly(); @@ -122,7 +122,7 @@ TEST(ScudoSecondaryTest, SecondaryCombinations) { } } } - scudo::ScopedString Str(1024); + scudo::ScopedString Str; L->getStats(&Str); Str.output(); L->unmapTestOnly(); @@ -146,7 +146,7 @@ TEST(ScudoSecondaryTest, SecondaryIterate) { L->deallocate(scudo::Options{}, V.back()); V.pop_back(); } - scudo::ScopedString Str(1024); + scudo::ScopedString Str; L->getStats(&Str); Str.output(); L->unmapTestOnly(); @@ -217,7 +217,7 @@ TEST(ScudoSecondaryTest, SecondaryThreadsRace) { } for (auto &T : Threads) T.join(); - scudo::ScopedString Str(1024); + scudo::ScopedString Str; L->getStats(&Str); Str.output(); L->unmapTestOnly(); diff --git a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp index eed174dc586a..332bac2f796e 100644 --- a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp @@ -13,7 +13,7 @@ #include TEST(ScudoStringsTest, Basic) { - scudo::ScopedString Str(128); + scudo::ScopedString Str; Str.append("a%db%zdc%ue%zuf%xh%zxq%pe%sr", static_cast(-1), static_cast(-2), static_cast(-4), static_cast(5), static_cast(10), @@ -29,7 +29,7 @@ TEST(ScudoStringsTest, Basic) { } TEST(ScudoStringsTest, Precision) { - scudo::ScopedString Str(128); + scudo::ScopedString Str; Str.append("%.*s", 3, "12345"); EXPECT_EQ(Str.length(), strlen(Str.data())); EXPECT_STREQ("123", Str.data()); @@ -52,7 +52,7 @@ TEST(ScudoStringTest, PotentialOverflows) { // Use a ScopedString that spans a page, and attempt to write past the end // of it with variations of append. The expectation is for nothing to crash. const scudo::uptr PageSize = scudo::getPageSizeCached(); - scudo::ScopedString Str(PageSize); + scudo::ScopedString Str; Str.clear(); fillString(Str, 2 * PageSize); Str.clear(); @@ -68,7 +68,7 @@ TEST(ScudoStringTest, PotentialOverflows) { template static void testAgainstLibc(const char *Format, T Arg1, T Arg2) { - scudo::ScopedString Str(128); + scudo::ScopedString Str; Str.append(Format, Arg1, Arg2); char Buffer[128]; snprintf(Buffer, sizeof(Buffer), Format, Arg1, Arg2); diff --git a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp index d2c6a9b6bb3c..dc23c2a34713 100644 --- a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp @@ -23,14 +23,14 @@ TEST(ScudoVectorTest, Basic) { } TEST(ScudoVectorTest, Stride) { - scudo::Vector V; - for (int i = 0; i < 1000; i++) { - V.push_back(i); - EXPECT_EQ(V.size(), i + 1U); - EXPECT_EQ(V[i], i); + scudo::Vector V; + for (scudo::uptr I = 0; I < 1000; I++) { + V.push_back(I); + EXPECT_EQ(V.size(), I + 1U); + EXPECT_EQ(V[I], I); } - for (int i = 0; i < 1000; i++) - EXPECT_EQ(V[i], i); + for (scudo::uptr I = 0; I < 1000; I++) + EXPECT_EQ(V[I], I); } TEST(ScudoVectorTest, ResizeReduction) { diff --git a/compiler-rt/lib/scudo/standalone/vector.h b/compiler-rt/lib/scudo/standalone/vector.h index 6ca350a25771..2c9a6e2aa655 100644 --- a/compiler-rt/lib/scudo/standalone/vector.h +++ b/compiler-rt/lib/scudo/standalone/vector.h @@ -19,14 +19,13 @@ namespace scudo { // small vectors. The current implementation supports only POD types. template class VectorNoCtor { public: - void init(uptr InitialCapacity) { - CapacityBytes = 0; - Size = 0; - Data = nullptr; + void init(uptr InitialCapacity = 0) { + Data = reinterpret_cast(&LocalData[0]); + CapacityBytes = sizeof(LocalData); reserve(InitialCapacity); } void destroy() { - if (Data) + if (Data != reinterpret_cast(&LocalData[0])) unmap(Data, CapacityBytes); } T &operator[](uptr I) { @@ -82,26 +81,24 @@ private: void reallocate(uptr NewCapacity) { DCHECK_GT(NewCapacity, 0); DCHECK_LE(Size, NewCapacity); - const uptr NewCapacityBytes = - roundUpTo(NewCapacity * sizeof(T), getPageSizeCached()); + NewCapacity = roundUpTo(NewCapacity * sizeof(T), getPageSizeCached()); T *NewData = - reinterpret_cast(map(nullptr, NewCapacityBytes, "scudo:vector")); - if (Data) { - memcpy(NewData, Data, Size * sizeof(T)); - unmap(Data, CapacityBytes); - } + reinterpret_cast(map(nullptr, NewCapacity, "scudo:vector")); + memcpy(NewData, Data, Size * sizeof(T)); + destroy(); Data = NewData; - CapacityBytes = NewCapacityBytes; + CapacityBytes = NewCapacity; } - T *Data; - uptr CapacityBytes; - uptr Size; + T *Data = nullptr; + u8 LocalData[256] = {}; + uptr CapacityBytes = 0; + uptr Size = 0; }; template class Vector : public VectorNoCtor { public: - Vector() { VectorNoCtor::init(1); } + Vector() { VectorNoCtor::init(); } explicit Vector(uptr Count) { VectorNoCtor::init(Count); this->resize(Count);