[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane updated this revision to Diff 00.
ckissane added a comment.

tidy some code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

Files:
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/FindZSTD.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -23,11 +23,9 @@
 namespace {
 
 #if LLVM_ENABLE_ZLIB
-
-void TestZlibCompression(StringRef Input, int Level) {
+static void testZlibCompression(StringRef Input, int Level) {
   SmallString<32> Compressed;
   SmallString<32> Uncompressed;
-
   zlib::compress(Input, Compressed, Level);
 
   // Check that uncompressed buffer is the same as original.
@@ -43,26 +41,63 @@
 }
 
 TEST(CompressionTest, Zlib) {
-  TestZlibCompression("", zlib::DefaultCompression);
+  testZlibCompression("", zlib::DefaultCompression);
 
-  TestZlibCompression("hello, world!", zlib::NoCompression);
-  TestZlibCompression("hello, world!", zlib::BestSizeCompression);
-  TestZlibCompression("hello, world!", zlib::BestSpeedCompression);
-  TestZlibCompression("hello, world!", zlib::DefaultCompression);
+  testZlibCompression("hello, world!", zlib::NoCompression);
+  testZlibCompression("hello, world!", zlib::BestSizeCompression);
+  testZlibCompression("hello, world!", zlib::BestSpeedCompression);
+  testZlibCompression("hello, world!", zlib::DefaultCompression);
 
   const size_t kSize = 1024;
   char BinaryData[kSize];
-  for (size_t i = 0; i < kSize; ++i) {
+  for (size_t i = 0; i < kSize; ++i)
 BinaryData[i] = i & 255;
-  }
   StringRef BinaryDataStr(BinaryData, kSize);
 
-  TestZlibCompression(BinaryDataStr, zlib::NoCompression);
-  TestZlibCompression(BinaryDataStr, zlib::BestSizeCompression);
-  TestZlibCompression(BinaryDataStr, zlib::BestSpeedCompression);
-  TestZlibCompression(BinaryDataStr, zlib::DefaultCompression);
+  testZlibCompression(BinaryDataStr, zlib::NoCompression);
+  testZlibCompression(BinaryDataStr, zlib::BestSizeCompression);
+  testZlibCompression(BinaryDataStr, zlib::BestSpeedCompression);
+  testZlibCompression(BinaryDataStr, zlib::DefaultCompression);
+}
+#endif
+
+#if LLVM_ENABLE_ZSTD
+static void testZstdCompression(StringRef Input, int Level) {
+  SmallString<32> Compressed;
+  SmallString<32> Uncompressed;
+  zstd::compress(Input, Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, Uncompressed);
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
 }
 
+TEST(CompressionTest, Zstd) {
+  testZstdCompression("", zstd::DefaultCompression);
+
+  testZstdCompression("hello, world!", zstd::NoCompression);
+  testZstdCompression("hello, world!", zstd::BestSizeCompression);
+  testZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  testZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i)
+BinaryData[i] = i & 255;
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  testZstdCompression(BinaryDataStr, zstd::NoCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  testZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  testZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
 #endif
 
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = 

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The patch looks mostly good but please fix the subject and the summary.

Note: if you fix the local commit message, you can use ` arc diff --head=HEAD 
'HEAD^' --verbatim` to update the subject and the summary.




Comment at: llvm/lib/Support/Compression.cpp:87
  UncompressedSize);
-  UncompressedBuffer.truncate(UncompressedSize);
+  if (UncompressedSize < UncompressedBuffer.size()) {
+UncompressedBuffer.truncate(UncompressedSize);

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

ditto below



Comment at: llvm/lib/Support/Compression.cpp:133
+   size_t ) {
+  size_t const Res =
+  ::ZSTD_decompress((char *)UncompressedBuffer, UncompressedSize,

`const size_t`



Comment at: llvm/unittests/Support/CompressionTest.cpp:69
+#if LLVM_ENABLE_ZSTD
+
+void TestZstdCompression(StringRef Input, int Level) {

delete blank line



Comment at: llvm/unittests/Support/CompressionTest.cpp:70
+
+void TestZstdCompression(StringRef Input, int Level) {
+  SmallString<32> Compressed;

`static void testZstd...`



Comment at: llvm/unittests/Support/CompressionTest.cpp:73
+  SmallString<32> Uncompressed;
+
+  zstd::compress(Input, Compressed, Level);

delete blank line 



Comment at: llvm/unittests/Support/CompressionTest.cpp:99
+  for (size_t i = 0; i < kSize; ++i) {
+BinaryData[i] = i & 255;
+  }

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane updated this revision to Diff 444374.
ckissane added a comment.

- Trim CMAKE modifications to minimum needed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

Files:
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/FindZSTD.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -65,4 +65,46 @@
 
 #endif
 
+#if LLVM_ENABLE_ZSTD
+
+void TestZstdCompression(StringRef Input, int Level) {
+  SmallString<32> Compressed;
+  SmallString<32> Uncompressed;
+
+  zstd::compress(Input, Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, Uncompressed);
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
+}
+
+TEST(CompressionTest, Zstd) {
+  TestZstdCompression("", zstd::DefaultCompression);
+
+  TestZstdCompression("hello, world!", zstd::NoCompression);
+  TestZstdCompression("hello, world!", zstd::BestSizeCompression);
+  TestZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  TestZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i) {
+BinaryData[i] = i & 255;
+  }
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  TestZstdCompression(BinaryDataStr, zstd::NoCompression);
+  TestZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  TestZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  TestZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
+
+#endif
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_curl = @LLVM_ENABLE_CURL@
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -20,6 +20,9 @@
 #if LLVM_ENABLE_ZLIB
 #include 
 #endif
+#if LLVM_ENABLE_ZSTD
+#include 
+#endif
 
 using namespace llvm;
 using namespace llvm::compression;
@@ -57,7 +60,9 @@
   // Tell MemorySanitizer that zlib output buffer is fully initialized.
   // This avoids a false report when running LLVM with uninstrumented ZLib.
   __msan_unpoison(CompressedBuffer.data(), CompressedSize);
-  CompressedBuffer.truncate(CompressedSize);
+  if (CompressedSize < CompressedBuffer.size()) {
+CompressedBuffer.truncate(CompressedSize);
+  }
 }
 
 Error zlib::uncompress(StringRef InputBuffer, char *UncompressedBuffer,
@@ -79,7 +84,9 @@
   UncompressedBuffer.resize_for_overwrite(UncompressedSize);
   Error E = zlib::uncompress(InputBuffer, UncompressedBuffer.data(),
  UncompressedSize);
-  UncompressedBuffer.truncate(UncompressedSize);
+  if (UncompressedSize < UncompressedBuffer.size()) {
+UncompressedBuffer.truncate(UncompressedSize);
+  }
   return E;
 }
 
@@ -99,3 +106,67 @@
   llvm_unreachable("zlib::uncompress is unavailable");
 }
 #endif
+
+#if LLVM_ENABLE_ZSTD
+
+bool zstd::isAvailable() { return true; }
+
+void zstd::compress(StringRef InputBuffer,
+SmallVectorImpl , int Level) {
+  unsigned long CompressedBufferSize = ::ZSTD_compressBound(InputBuffer.size());
+  CompressedBuffer.resize_for_overwrite(CompressedBufferSize);
+  unsigned long 

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane added a comment.

In D128465#3646601 , @MaskRay wrote:

> In D128465#3646561 , @ckissane 
> wrote:
>
>> In D128465#3646258 , @MaskRay 
>> wrote:
>>
>>> In D128465#3646203 , @ckissane 
>>> wrote:
>>>
 In D128465#3642997 , @MaskRay 
 wrote:

> As I mentioned, the proper approach is to add zstd functionality along 
> with the CMake change, instead of adding CMake to all llvm-project 
> components without a way to test them.

 @MaskRay, I have now done this and ran the ldd tests as requested:

   With LLVM_ENABLE_ZSTD=ON
   $ ninja check-lld
   Testing Time: 8.98s
 Unsupported  :   17
 Passed   : 2638
 Expectedly Failed:1
   With LLVM_ENABLE_ZSTD=OFF
   $ ninja check-lld
   Testing Time: 8.95s
 Unsupported  :   17
 Passed   : 2638
 Expectedly Failed:1
>>>
>>> I request that you abandon this patch and incorporate the llvm cmake part 
>>> into the llvm patch which you actually use zstd.
>>> It is not appropriate to add zstd cmake to llvm-project components which 
>>> don't use zstd.
>>
>> Let me see if I understand this correctly:
>> Are you saying that I should abandon this patch, and create a new patch that:
>>
>> - adds findZSTD.cmake
>> - adds zstd to compression namespace
>> - adds tests to CompressionTest.cpp
>> - and does the **minimal** amount of cmake work to have the flags and test 
>> work
>>   - thus differing per component cmake config to patches like how you are 
>> doing in https://reviews.llvm.org/D129406
>>
>> Is this correct?
>>
>> I realize though that then https://reviews.llvm.org/D129406 or similar would 
>> be "the [first] llvm patch which actually use[s] zstd"
>
> You can find a component in llvm which uses zstd (e.g. lib/Support). Add the 
> functionality with tests (e.g. a CompressionTest.cpp unittest) along with the 
> CMake/(optional Bazel, gn) changes.
> Keep clang, clang-tools-extra, lld, etc unchanged. These components don't yet 
> use zstd and the CMake changes in this patch will be untestable.

Got it!, this patch already has such tests (CompressionTest.cpp unittest) in 
lib/Support, so I should just remove the untestable cmake config.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane added a comment.

In D128465#3646258 , @MaskRay wrote:

> In D128465#3646203 , @ckissane 
> wrote:
>
>> In D128465#3642997 , @MaskRay 
>> wrote:
>>
>>> As I mentioned, the proper approach is to add zstd functionality along with 
>>> the CMake change, instead of adding CMake to all llvm-project components 
>>> without a way to test them.
>>
>> @MaskRay, I have now done this and ran the ldd tests as requested:
>>
>>   With LLVM_ENABLE_ZSTD=ON
>>   $ ninja check-lld
>>   Testing Time: 8.98s
>> Unsupported  :   17
>> Passed   : 2638
>> Expectedly Failed:1
>>   With LLVM_ENABLE_ZSTD=OFF
>>   $ ninja check-lld
>>   Testing Time: 8.95s
>> Unsupported  :   17
>> Passed   : 2638
>> Expectedly Failed:1
>
> I request that you abandon this patch and incorporate the llvm cmake part 
> into the llvm patch which you actually use zstd.
> It is not appropriate to add zstd cmake to llvm-project components which 
> don't use zstd.

Let me see if I understand this correctly:
Are you saying that I should abandon this patch, and create a new patch that:

- adds findZSTD.cmake
- adds zstd to compression namespace
- adds tests to CompressionTest.cpp
- and does the **minimal** amount of cmake work to have the flags and test work
  - thus differing per component cmake config to patches like how you are doing 
in https://reviews.llvm.org/D129406

Is this correct?

I realize though that then https://reviews.llvm.org/D129406 or similar would be 
"the [first] llvm patch which actually use[s] zstd"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3646561 , @ckissane wrote:

> In D128465#3646258 , @MaskRay wrote:
>
>> In D128465#3646203 , @ckissane 
>> wrote:
>>
>>> In D128465#3642997 , @MaskRay 
>>> wrote:
>>>
 As I mentioned, the proper approach is to add zstd functionality along 
 with the CMake change, instead of adding CMake to all llvm-project 
 components without a way to test them.
>>>
>>> @MaskRay, I have now done this and ran the ldd tests as requested:
>>>
>>>   With LLVM_ENABLE_ZSTD=ON
>>>   $ ninja check-lld
>>>   Testing Time: 8.98s
>>> Unsupported  :   17
>>> Passed   : 2638
>>> Expectedly Failed:1
>>>   With LLVM_ENABLE_ZSTD=OFF
>>>   $ ninja check-lld
>>>   Testing Time: 8.95s
>>> Unsupported  :   17
>>> Passed   : 2638
>>> Expectedly Failed:1
>>
>> I request that you abandon this patch and incorporate the llvm cmake part 
>> into the llvm patch which you actually use zstd.
>> It is not appropriate to add zstd cmake to llvm-project components which 
>> don't use zstd.
>
> Let me see if I understand this correctly:
> Are you saying that I should abandon this patch, and create a new patch that:
>
> - adds findZSTD.cmake
> - adds zstd to compression namespace
> - adds tests to CompressionTest.cpp
> - and does the **minimal** amount of cmake work to have the flags and test 
> work
>   - thus differing per component cmake config to patches like how you are 
> doing in https://reviews.llvm.org/D129406
>
> Is this correct?
>
> I realize though that then https://reviews.llvm.org/D129406 or similar would 
> be "the [first] llvm patch which actually use[s] zstd"

You can find a component in llvm which uses zstd (e.g. lib/Support). Add the 
functionality with tests (e.g. a CompressionTest.cpp unittest) along with the 
CMake/(optional Bazel, gn) changes.
Keep clang, clang-tools-extra, lld, etc unchanged. These components don't yet 
use zstd and the CMake changes in this patch will be untestable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane updated this revision to Diff 444078.
ckissane added a comment.

- add zstd tests to CompressionTest.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/test/lit.cfg.py
  clang-tools-extra/clangd/test/lit.site.cfg.py.in
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  compiler-rt/lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh
  compiler-rt/test/lit.common.cfg.py
  compiler-rt/test/lit.common.configured.in
  flang/CMakeLists.txt
  lld/ELF/CMakeLists.txt
  lld/test/lit.site.cfg.py.in
  lldb/source/Plugins/Process/gdb-remote/CMakeLists.txt
  lldb/test/Shell/lit.site.cfg.py.in
  llvm/CMakeLists.txt
  llvm/cmake/config-ix.cmake
  llvm/cmake/modules/FindZSTD.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/llvm-config.h.cmake
  llvm/include/llvm/Support/Compression.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Compression.cpp
  llvm/test/lit.site.cfg.py.in
  llvm/unittests/Support/CompressionTest.cpp
  utils/bazel/llvm_configs/llvm-config.h.cmake

Index: utils/bazel/llvm_configs/llvm-config.h.cmake
===
--- utils/bazel/llvm_configs/llvm-config.h.cmake
+++ utils/bazel/llvm_configs/llvm-config.h.cmake
@@ -95,6 +95,9 @@
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
+/* Define if zstd compression is available */
+#cmakedefine01 LLVM_ENABLE_ZSTD
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/unittests/Support/CompressionTest.cpp
===
--- llvm/unittests/Support/CompressionTest.cpp
+++ llvm/unittests/Support/CompressionTest.cpp
@@ -65,4 +65,46 @@
 
 #endif
 
+#if LLVM_ENABLE_ZSTD
+
+void TestZstdCompression(StringRef Input, int Level) {
+  SmallString<32> Compressed;
+  SmallString<32> Uncompressed;
+
+  zstd::compress(Input, Compressed, Level);
+
+  // Check that uncompressed buffer is the same as original.
+  Error E = zstd::uncompress(Compressed, Uncompressed, Input.size());
+  consumeError(std::move(E));
+
+  EXPECT_EQ(Input, Uncompressed);
+  if (Input.size() > 0) {
+// Uncompression fails if expected length is too short.
+E = zstd::uncompress(Compressed, Uncompressed, Input.size() - 1);
+EXPECT_EQ("Destination buffer is too small", llvm::toString(std::move(E)));
+  }
+}
+
+TEST(CompressionTest, Zstd) {
+  TestZstdCompression("", zstd::DefaultCompression);
+
+  TestZstdCompression("hello, world!", zstd::NoCompression);
+  TestZstdCompression("hello, world!", zstd::BestSizeCompression);
+  TestZstdCompression("hello, world!", zstd::BestSpeedCompression);
+  TestZstdCompression("hello, world!", zstd::DefaultCompression);
+
+  const size_t kSize = 1024;
+  char BinaryData[kSize];
+  for (size_t i = 0; i < kSize; ++i) {
+BinaryData[i] = i & 255;
+  }
+  StringRef BinaryDataStr(BinaryData, kSize);
+
+  TestZstdCompression(BinaryDataStr, zstd::NoCompression);
+  TestZstdCompression(BinaryDataStr, zstd::BestSizeCompression);
+  TestZstdCompression(BinaryDataStr, zstd::BestSpeedCompression);
+  TestZstdCompression(BinaryDataStr, zstd::DefaultCompression);
+}
+
+#endif
 }
Index: llvm/test/lit.site.cfg.py.in
===
--- llvm/test/lit.site.cfg.py.in
+++ llvm/test/lit.site.cfg.py.in
@@ -37,6 +37,7 @@
 config.llvm_use_intel_jitevents = @LLVM_USE_INTEL_JITEVENTS@
 config.llvm_use_sanitizer = "@LLVM_USE_SANITIZER@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
+config.have_zstd = @LLVM_ENABLE_ZSTD@
 config.have_libxar = @LLVM_HAVE_LIBXAR@
 config.have_libxml2 = @LLVM_ENABLE_LIBXML2@
 config.have_curl = @LLVM_ENABLE_CURL@
Index: llvm/lib/Support/Compression.cpp
===
--- llvm/lib/Support/Compression.cpp
+++ llvm/lib/Support/Compression.cpp
@@ -20,6 +20,9 @@
 #if LLVM_ENABLE_ZLIB
 #include 
 #endif
+#if LLVM_ENABLE_ZSTD
+#include 
+#endif
 
 using namespace llvm;
 using namespace llvm::compression;
@@ -57,7 +60,9 @@
   // Tell MemorySanitizer that zlib output buffer is fully initialized.
   // This avoids a false report when running LLVM with uninstrumented ZLib.
   __msan_unpoison(CompressedBuffer.data(), CompressedSize);
-  CompressedBuffer.truncate(CompressedSize);
+  if (CompressedSize < CompressedBuffer.size()) {
+CompressedBuffer.truncate(CompressedSize);
+  }
 }
 
 Error zlib::uncompress(StringRef InputBuffer, char *UncompressedBuffer,
@@ -79,7 +84,9 @@
   UncompressedBuffer.resize_for_overwrite(UncompressedSize);
   Error E = zlib::uncompress(InputBuffer, UncompressedBuffer.data(),
  UncompressedSize);
-  UncompressedBuffer.truncate(UncompressedSize);
+  if (UncompressedSize < 

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Cole Kissane via Phabricator via lldb-commits]

ckissane added a comment.

In D128465#3642997 , @MaskRay wrote:

> As I mentioned, the proper approach is to add zstd functionality along with 
> the CMake change, instead of adding CMake to all llvm-project components 
> without a way to test them.

@MaskRay, I have now done this and ran the ldd tests as requested:

  With LLVM_ENABLE_ZSTD=ON
  $ ninja check-lld
  Testing Time: 8.98s
Unsupported  :   17
Passed   : 2638
Expectedly Failed:1
  With LLVM_ENABLE_ZSTD=OFF
  $ ninja check-lld
  Testing Time: 8.95s
Unsupported  :   17
Passed   : 2638
Expectedly Failed:1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM and add ZSTD compression namespace

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

In D128465#3646203 , @ckissane wrote:

> In D128465#3642997 , @MaskRay wrote:
>
>> As I mentioned, the proper approach is to add zstd functionality along with 
>> the CMake change, instead of adding CMake to all llvm-project components 
>> without a way to test them.
>
> @MaskRay, I have now done this and ran the ldd tests as requested:
>
>   With LLVM_ENABLE_ZSTD=ON
>   $ ninja check-lld
>   Testing Time: 8.98s
> Unsupported  :   17
> Passed   : 2638
> Expectedly Failed:1
>   With LLVM_ENABLE_ZSTD=OFF
>   $ ninja check-lld
>   Testing Time: 8.95s
> Unsupported  :   17
> Passed   : 2638
> Expectedly Failed:1

I request that you abandon this patch and incorporate the llvm cmake part into 
the llvm patch which you actually use zstd.
It is not appropriate to add zstd cmake to llvm-project components which don't 
use zstd.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128465/new/

https://reviews.llvm.org/D128465

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits