Global SpinLock/SpinLockHolder in s2/base/spinlock.h collide with gperftools (ODR violation, startup hang in debug builds) #689

Summary

s2/base/spinlock.h defines SpinLock and SpinLockHolder in the global namespace. gperftools (tcmalloc and libprofiler) also defines global classes with the same names in src/base/spinlock.h, but with a different layout and locking protocol. When both libraries are linked into one program, this is an ODR violation.

In unoptimized builds, the inline members are emitted as weak symbols with identical mangled names. The linker keeps one copy, so one library runs the other library's code.

These classes are only an implementation detail of MutableS2ShapeIndex and EncodedS2ShapeIndex. Moving them into a namespace, and keeping global using-declarations for the existing unqualified uses, fixes the problem. The change is confined to this one header.

Environment

• s2geometry at c24b368. s2/base/spinlock.h is unchanged on current master. The classes have been global since at least 2018.
• Abseil 20250814.1
• gperftools 2.10, static libraries. Current gperftools master still defines the same global classes.
• Linux x86_64. Reproduced with Clang 19.1.7 and GCC 10.3.1.

What collides

GCC also emits the C1/D1 variants. The other members have different names or signatures (lock() vs Lock(), SpinLockHolder(SpinLock&) vs SpinLockHolder(SpinLock*)), so they don't collide.

Consequences:

• If S2's constructor is kept, a gperftools lock gets only its first byte initialized. The other 3 bytes keep whatever was in memory. For example, gperftools' ProfileHandler is heap-allocated, so its lock words can start non-zero, and Lock() never sees the lock as free.
• If S2's ~SpinLockHolder() is kept, gperftools' unlock becomes a 1-byte store and skips SlowUnlock(), so sleeping waiters are not woken.
• If gperftools' constructor is kept, it writes 4 bytes into S2's 1-byte object. In current S2 layouts these bytes happen to land in padding, but this is still undefined behavior.

We hit the first case. A debug build of a server application that statically links S2 and gperftools' libprofiler hung during startup, spinning in gperftools' SpinLock code reached from ProfileHandler. Optimized builds did not show the problem.

Minimal reproducer

The reproducer uses only the unmodified S2 header and Abseil. other_lib.cc stands in for gperftools: it defines a global SpinLock with a 4-byte lock word.

s2_side.cc, which uses SpinLock as a data member, the same way MutableS2ShapeIndex does:

#include "s2/base/spinlock.h"

struct IndexLike {
  IndexLike();
  mutable SpinLock lock_;
};

IndexLike::IndexLike() {}

void s2_side_use() {
  IndexLike index;
  SpinLockHolder h(index.lock_);
}

other_lib.cc:

#include <atomic>
#include <cstdio>
#include <cstring>
#include <new>

class SpinLock {
 public:
  SpinLock() : lockword_(0) {}
  int Word() const { return lockword_.load(); }

 private:
  std::atomic<int> lockword_;
};

int other_lib_check() {
  alignas(SpinLock) unsigned char buf[sizeof(SpinLock)];
  std::memset(buf, 0xff, sizeof(buf));  // Simulate non-zeroed heap memory.
  SpinLock* l = new (buf) SpinLock();
  std::printf("other lib: lockword_ after SpinLock() = 0x%08x (expected 0)\n",
              static_cast<unsigned>(l->Word()));
  return l->Word() == 0 ? 0 : 1;
}

main.cc:

int other_lib_check();
void s2_side_use();

int main() {
  s2_side_use();
  return other_lib_check();
}

Build and run ($S2 is the s2geometry checkout, $ABSL the Abseil checkout):

CXX=clang++   # or g++
$CXX -std=c++17 -O0 -I$S2/src -I$ABSL -c s2_side.cc
$CXX -std=c++17 -O0 -c other_lib.cc main.cc
$CXX main.o s2_side.o other_lib.o -o repro && ./repro

Output, the same with Clang 19.1.7 and GCC 10.3.1:

other lib: lockword_ after SpinLock() = 0xffffff00 (expected 0)

If other_lib.o is linked before s2_side.o, the output is correct. So the link order decides which definition every caller gets.

If the S2 side is built with -O1 or higher, both compilers inline these members. No weak symbols are emitted and the reproducer passes, but the ODR violation is still there. GCC reports it with LTO:

g++ -std=c++17 -O0 -flto -I$S2/src -I$ABSL -c s2_side.cc
g++ -std=c++17 -O0 -flto -c other_lib.cc main.cc
g++ -flto -Wodr main.o s2_side.o other_lib.o -o repro_lto

s2/base/spinlock.h:23:48: warning: type 'struct SpinLock' violates the C++ One Definition Rule [-Wodr]
other_lib.cc:8:7: note: a different type is defined in another translation unit
s2/base/spinlock.h:46:20: note: the first difference of corresponding definitions is field 'locked_'

Relation to #68 and #386

#68 tracks putting all of S2 into a namespace. #386 was closed as a duplicate of #68, and a comment there notes the migration cost for internal clients. This issue is narrower: it is a concrete runtime failure with a widely used library, and the fix below needs no changes in S2 sources or in client code.

Proposed fix

--- a/src/s2/base/spinlock.h
+++ b/src/s2/base/spinlock.h
@@ -20,6 +20,8 @@
 
 #include "absl/base/thread_annotations.h"
 
+namespace s2internal {
+
 class ABSL_LOCKABLE ABSL_ATTRIBUTE_WARN_UNUSED SpinLock {
  public:
   SpinLock() = default;
@@ -61,4 +63,11 @@
   SpinLock& lock_;
 };
 
+}  // namespace s2internal
+
+// Keep the unqualified names used by S2 sources; aliases do not
+// affect symbol names.
+using s2internal::SpinLock;
+using s2internal::SpinLockHolder;
+
 #endif  // S2_BASE_SPINLOCK_H_

With this change:

• mutable_s2shape_index.cc and encoded_s2shape_index.cc compile without changes. The emitted symbols become _ZN10s2internal8SpinLock... and _ZN10s2internal14SpinLockHolder....
• The reproducer prints 0x00000000, and -flto -Wodr no longer warns.
• We have not run the S2 test suite with this patch. We use an equivalent change (namespace plus aliases) in our own build, and our application tests pass.

I'm happy to send a PR if this direction is acceptable.