[clangd] Don't use zlib when it's unavailable.

[ArcsinX]

Summary

Without this patch clangd crashes at try to load compressed string table when zlib is not available.
Example:

• Build clangd with MinGW (zlib found)
• Build index
• Build clangd with Visual Studio compiler (zlib not found)
• Try to load index

[ArcsinX]

Unsure about test for this

[ArcsinX]

Fix build

[adamcz]

clang-tools-extra/clangd/index/Serialization.cpp

209

nit: skip { } to stay consistent with local style

[ArcsinX]

Remove {}

[sammccall]

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

[dblaikie]

In D87673#2275940, @sammccall wrote:

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib

[sammccall]

In D87673#2290838, @dblaikie wrote:

In D87673#2275940, @sammccall wrote:

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib

Yep, that requires a checked-in binary file, which in the case of that test is in a standard stable format, which this data is not, so we'd also need tools to regenerate it. And we don't have APIs to generate the file without compression if zlib is available (because we never want to do that), so we'd need to add those or require anyone touching the file format to build a no-zlib tree to regenerate the file.
None of this is impossible, but this is pretty unlikely to catch a bug, and it's very unlikely to be a high-impact bug..

[dblaikie]

In D87673#2290926, @sammccall wrote:

In D87673#2290838, @dblaikie wrote:

In D87673#2275940, @sammccall wrote:

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

In D87673#2273347, @ArcsinX wrote:

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib

Yep, that requires a checked-in binary file, which in the case of that test is in a standard stable format, which this data is not, so we'd also need tools to regenerate it. And we don't have APIs to generate the file without compression if zlib is available (because we never want to do that), so we'd need to add those or require anyone touching the file format to build a no-zlib tree to regenerate the file.
None of this is impossible, but this is pretty unlikely to catch a bug, and it's very unlikely to be a high-impact bug..

Ah, fair enough. Might be nice to have a flag to request the uncompressed behavior even when compression's available - for testing purposes and such, but given the current implementation, yeah, don't see a tidy way to test it.

clang-tools-extra/clangd/index/Serialization.cpp

llvm::Expected<StringTableIn> readStringTable(llvm::StringRef Data) {
   size_t UncompressedSize = R.consume32();
   if (R.err())
     return error("Truncated string table");
 
   llvm::StringRef Uncompressed;
   llvm::SmallString<1> UncompressedStorage;
   if (UncompressedSize == 0) // No compression
     Uncompressed = R.rest();
-  else {
+  else if (llvm::zlib::isAvailable()) {
     if (llvm::Error E = llvm::zlib::uncompress(R.rest(), UncompressedStorage,
                                                UncompressedSize))
       return std::move(E);
     Uncompressed = UncompressedStorage;
-  }
+  } else
+    return error("Compressed string table, but zlib is unavailable");
 
   StringTableIn Table;
   llvm::StringSaver Saver(Table.Arena);
   R = Reader(Uncompressed);
   for (Reader R(Uncompressed); !R.eof();) {
     auto Len = R.rest().find(0);
     if (Len == llvm::StringRef::npos)
       return error("Bad string table: not null terminated");