Add check for struct sizes (#4172)

Summary:

C++ Faiss is compiled with (at least) 3 compilers:
- the one compiling the main CPU Faiss
- the one called by nvcc to compile host code on GPU Faiss
- the one compiling the SWIG generated file

Sometimes the compilers are misconfigured and have different ideas of data alignment, size and packing. This is a hard to catch bug.

This test attemtps to check if one of these differences occur.

The test did catch the bug in one configuration in the Github version, see


#4136
https://github.com/facebookresearch/faiss/actions/runs/13177278514/job/36779465009?pr=4136

Reviewed By: mnorris11

Differential Revision: D69243348

Author: mdouze

.github/actions/build_cmake/action.yml

@@ -132,7 +132,7 @@ runs:
               -DPYTHON_EXECUTABLE=$CONDA/bin/python \
               -DCMAKE_BUILD_TYPE=Release \
               -DBLA_VENDOR=${{ runner.arch == 'X64' && 'Intel10_64_dyn' || '' }} \
-              -DCMAKE_CUDA_FLAGS=${{ runner.arch == 'X64' && '"-gencode arch=compute_75,code=sm_75"' || '' }} \
+              -DCMAKE_CUDA_FLAGS=${{ runner.arch == 'X64' && '"--verbose  -ccbin x86_64-conda-linux-gnu-c++ -gencode arch=compute_75,code=sm_75"' || '--verbose -ccbin x86_64-conda-linux-gnu-c++' }} \
               .
         make -k -C build -j$(nproc)
     - name: C++ tests

.github/workflows/build-pull-request.yml

@@ -38,39 +38,6 @@ jobs:
         uses: actions/checkout@v4
       - name: Build and Test (cmake)
         uses: ./.github/actions/build_cmake
-  linux-x86_64-AVX2-cmake:
-    name: Linux x86_64 AVX2 (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          opt_level: avx2
-  linux-x86_64-AVX512-cmake:
-    name: Linux x86_64 AVX512 (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: faiss-aws-m7i.large
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          opt_level: avx512
-  linux-x86_64-AVX512_SPR-cmake:
-    name: Linux x86_64 AVX512_SPR (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: faiss-aws-m7i.large
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          opt_level: avx512_spr
   linux-x86_64-GPU-cmake:
     name: Linux x86_64 GPU (cmake)
     needs: linux-x86_64-cmake
@@ -82,88 +49,3 @@ jobs:
         uses: ./.github/actions/build_cmake
         with:
           gpu: ON
-  linux-x86_64-GPU-w-CUVS-cmake:
-    name: Linux x86_64 GPU w/ cuVS (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: 4-core-ubuntu-gpu-t4
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          gpu: ON
-          cuvs: ON
-  linux-x86_64-GPU-w-ROCm-cmake:
-    name: Linux x86_64 GPU w/ ROCm (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: faiss-amd-MI200
-    container:
-      image: ubuntu:22.04
-      options: --device=/dev/kfd --device=/dev/dri --ipc=host --shm-size 16G --group-add video --cap-add=SYS_PTRACE --cap-add=SYS_ADMIN
-    steps:
-      - name: Container setup
-        run: |
-            if [ -f /.dockerenv ]; then
-              apt-get update && apt-get install -y sudo && apt-get install -y git
-              git config --global --add safe.directory '*'
-            else
-              echo 'Skipping. Current job is not running inside a container.'
-            fi
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          gpu: ON
-          rocm: ON
-  linux-arm64-SVE-cmake:
-    name: Linux arm64 SVE (cmake)
-    needs: linux-x86_64-cmake
-    runs-on: faiss-aws-r8g.large
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-      - name: Build and Test (cmake)
-        uses: ./.github/actions/build_cmake
-        with:
-          opt_level: sve
-        env:
-          # Context: https://github.com/facebookresearch/faiss/wiki/Troubleshooting#surprising-faiss-openmp-and-openblas-interaction
-          OPENBLAS_NUM_THREADS: '1'
-  linux-x86_64-conda:
-    name: Linux x86_64 (conda)
-    needs: linux-x86_64-cmake
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-          fetch-tags: true
-      - name: Build and Package (conda)
-        uses: ./.github/actions/build_conda
-  windows-x86_64-conda:
-    name: Windows x86_64 (conda)
-    needs: linux-x86_64-cmake
-    runs-on: windows-2019
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-          fetch-tags: true
-      - name: Build and Package (conda)
-        uses: ./.github/actions/build_conda
-  linux-arm64-conda:
-    name: Linux arm64 (conda)
-    needs: linux-x86_64-cmake
-    runs-on: 2-core-ubuntu-arm
-    steps:
-      - name: Checkout
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-          fetch-tags: true
-      - name: Build and Package (conda)
-        uses: ./.github/actions/build_conda

faiss/CMakeLists.txt

@@ -214,6 +214,7 @@ set(FAISS_HEADERS
   utils/simdlib_emulated.h
   utils/simdlib_neon.h
   utils/simdlib_ppc64.h
+  utils/struct_packing_test.h
   utils/utils.h
   utils/distances_fused/avx512.h
   utils/distances_fused/distances_fused.h

faiss/gpu/GpuIndex.h

@@ -194,5 +194,7 @@ bool isGpuIndex(faiss::Index* index);
 /// Does the given CPU index instance have a corresponding GPU implementation?
 bool isGpuIndexImplemented(faiss::Index* index);
 
+int struct_packing_test_cuda(int q);
+
 } // namespace gpu
 } // namespace faiss

faiss/gpu/impl/IndexUtils.cu

@@ -7,6 +7,7 @@
 
 #include <faiss/gpu/impl/IndexUtils.h>
 #include <faiss/impl/FaissAssert.h>
+#include <faiss/utils/struct_packing_test.h>
 #include <faiss/gpu/utils/DeviceDefs.cuh>
 #include <limits>
 
@@ -38,5 +39,7 @@ void validateNProbe(size_t nprobe) {
             nprobe);
 }
 
+int struct_packing_test_cuda(int q) STRUCT_PACKING_FUNCTION_BODY
+
 } // namespace gpu
 } // namespace faiss

faiss/gpu/test/test_gpu_basics.py

@@ -469,3 +469,26 @@ class TestGpuFlags(unittest.TestCase):
 
     def test_gpu_flag(self):
         assert "GPU" in faiss.get_compile_options().split()
+
+
+class TestStructPacking(unittest.TestCase): 
+    """ Verify if the size structures as seen from Cuda and C++ are the same """
+
+    def test_swig(self): 
+        " This test is redundant with the CPU tests, but let's check run it just in case"
+        sizes = np.array([
+            (faiss.struct_packing_test_cpp(q),
+            faiss.struct_packing_test_swig(q)) 
+            for q in range(20)
+        ])
+        print(sizes)
+        np.testing.assert_array_equal(sizes[:, 0], sizes[:, 1])
+
+    def test_cuda(self): 
+        sizes = np.array([
+            (faiss.struct_packing_test_cpp(q),
+            faiss.struct_packing_test_cuda(q)) 
+            for q in range(20)
+        ])
+        print(sizes)
+        np.testing.assert_array_equal(sizes[:, 0], sizes[:, 1])

faiss/python/swigfaiss.swig

@@ -679,6 +679,7 @@ struct faiss::simd16uint16 {};
 
 %include  <faiss/gpu/GpuIndicesOptions.h>
 %include  <faiss/gpu/GpuClonerOptions.h>
+
 %include  <faiss/gpu/GpuIndex.h>
 #ifdef FAISS_ENABLE_CUVS
 %include  <faiss/gpu/GpuIndexCagra.h>
@@ -1245,6 +1246,20 @@ REV_SWIG_PTR(uint64_t, NPY_UINT64);
 
 #endif
 
+namespace faiss {
+int struct_packing_test_swig (int q);
+}
+
+%{
+
+#include <faiss/utils/struct_packing_test.h>
+
+namespace faiss {
+int struct_packing_test_swig (int q) STRUCT_PACKING_FUNCTION_BODY
+}
+
+%}
+
 
 
 /*******************************************************************

faiss/utils/struct_packing_test.h

@@ -0,0 +1,156 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/* This file and macro is there to catch discrepancies on the different
+ * compilers used in Faiss align and pack structures. This is a difficult to
+ * catch bug. It works by declaring a structure and measuring its size in
+ * diffetent compilers from a function. The function can be called from a test.
+ * If the size as seen from different compilers changes, it's a guarantee for
+ * fireworks at runtime. */
+
+#pragma once
+#include <vector>
+
+namespace faiss {
+namespace struct_packing_test {
+
+/*******************************************************
+ * Fake structure to detect structure packing/alignment
+ * issues.
+ *******************************************************/
+
+struct StructPackingTestD {
+    int a;
+    bool b, c, d;
+    long e;
+    bool f, g, h, i, j;
+};
+
+struct StructPackingTestC : StructPackingTestD {
+    bool k;
+    int l;
+    bool m;
+};
+
+struct StructPackingTestA {
+    virtual void* operator()(int) {
+        return nullptr;
+    }
+
+    virtual ~StructPackingTestA() {}
+};
+
+struct StructPackingTestB : StructPackingTestA {
+    StructPackingTestC options;
+    std::vector<void*> vres;
+    std::vector<int> devices;
+    int ncall;
+
+    void* operator()(int) override {
+        return nullptr;
+    }
+    virtual ~StructPackingTestB() {}
+};
+
+// This is the object hierachy of GpuProgressiveDimIndexFactory that initially
+// triggered this error (see PR #4135 and #4136)
+
+enum IndicesOptionsB {
+    INDICES_CPU_B = 0,
+    INDICES_IVF_B = 1,
+    INDICES_32_BIT_B = 2,
+    INDICES_64_BIT_B = 3,
+};
+
+struct GpuClonerOptionsB {
+    IndicesOptionsB indicesOptions = INDICES_64_BIT_B;
+
+    bool useFloat16CoarseQuantizer = false;
+
+    bool useFloat16 = false;
+
+    bool usePrecomputed = false;
+
+    long reserveVecs = 0;
+
+    bool storeTransposed = false;
+
+    bool verbose = false;
+
+    bool use_cuvs = false;
+    bool allowCpuCoarseQuantizer = false;
+};
+
+struct GpuMultipleClonerOptionsB : public GpuClonerOptionsB {
+    bool shard = false;
+    int shard_type = 1;
+    bool common_ivf_quantizer = false;
+};
+
+struct ProgressiveDimIndexFactoryB {
+
+    virtual void* operator()(int dim) {
+        return nullptr;
+    }
+
+    virtual ~ProgressiveDimIndexFactoryB() {}
+};
+
+struct GpuProgressiveDimIndexFactoryB : ProgressiveDimIndexFactoryB {
+    GpuMultipleClonerOptionsB options;
+    std::vector<void*> vres;
+    std::vector<int> devices;
+    int ncall;
+
+    explicit GpuProgressiveDimIndexFactoryB(int ngpu) {}
+
+    void* operator()(int dim) override {
+        return nullptr;
+    }
+
+    virtual ~GpuProgressiveDimIndexFactoryB() override {}
+};
+
+} // namespace struct_packing_test
+
+} // namespace faiss
+
+// body of function should be
+// int function_name (int q) STRUCT_PACKING_FUNCTION_BODY
+
+#define STRUCT_PACKING_FUNCTION_BODY                                           \
+    {                                                                          \
+        struct_packing_test::StructPackingTestB sb;                            \
+        switch (q) {                                                           \
+            case 0:                                                            \
+                return sizeof(struct_packing_test::StructPackingTestB);        \
+            case 1:                                                            \
+                return (char*)&sb.ncall - (char*)&sb;                          \
+            case 2:                                                            \
+                return sizeof(struct_packing_test::StructPackingTestD);        \
+            case 3:                                                            \
+                return sizeof(struct_packing_test::StructPackingTestC);        \
+            case 4:                                                            \
+                return sizeof(struct_packing_test::StructPackingTestB);        \
+            case 5:                                                            \
+                return sizeof(struct_packing_test::StructPackingTestA);        \
+            case 6:                                                            \
+                return sizeof(struct_packing_test::IndicesOptionsB);           \
+            case 7:                                                            \
+                return sizeof(struct_packing_test::GpuMultipleClonerOptionsB); \
+            case 8:                                                            \
+                return sizeof(struct_packing_test::GpuClonerOptionsB);         \
+            case 9:                                                            \
+                return sizeof(                                                 \
+                        struct_packing_test::ProgressiveDimIndexFactoryB);     \
+            case 10:                                                           \
+                return sizeof(                                                 \
+                        struct_packing_test::GpuProgressiveDimIndexFactoryB);  \
+            default:                                                           \
+                return -1;                                                     \
+        }                                                                      \
+    }

faiss/utils/utils.cpp

@@ -8,6 +8,7 @@
 // -*- c++ -*-
 
 #include <faiss/Index.h>
+#include <faiss/utils/struct_packing_test.h>
 #include <faiss/utils/utils.h>
 
 #include <cassert>
@@ -641,4 +642,6 @@ void CodeSet::insert(size_t n, const uint8_t* codes, bool* inserted) {
     }
 }
 
+int struct_packing_test_cpp(int q) STRUCT_PACKING_FUNCTION_BODY;
+
 } // namespace faiss