[PATCH] D91405: [clangd] Assert on varint encoding

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:90
       B = consume8();
+      assert((B & ~More) < (1 << (32 - Shift)) && "Invalid varint encoding");
       Val |= (B & ~More) << Shift;
----------------
I'm fine with an assert, but this still leaves us with the suspected UB in NDEBUG mode, which doesn't seem OK.
(As noted on D91258 I'm not sure this is actual UB under C++ rules)

The issue here is integral promotion rules say that uint_8 << anything uses *signed* arithmetic. And same for a bunch of the other operations.

I think the clearest thing to do is explicitly type B as uint32_t, so no expression it is in can ever be promoted to (signed) int.
(Unless int is bigger than 32 bits, in which case we've always got enough bits to shift into)


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:371
   std::string CorruptSrcs =
-      (Srcs->Data.take_front(Pos) + llvm::fromHex("8fffffff7f") +
+      (Srcs->Data.take_front(Pos) + llvm::fromHex("ffffffff0f") +
        "some_random_garbage")
----------------
/shamecube

In my defense, I grew up on a 68k mac... little endian is weird


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91405