Skip to content

Commit 35220b7

Browse files
authored
Fix tests under bazel test -c opt. (#212)
With certain versions of GCC, `bazel test -c opt ...` failed, apparently due to a compiler bug. This change: 1. Adds a workaround for the GCC bug. 2. Enables the workaround on versions of GCC known to have the bug. 3. Documents the bug (in comments). 4. Enables testing with `-c opt` as part of the commit hook. Note that the versions covered by #2 are a little wider than may be technically necessary. As of the time of writing, this affects all released versions of GCC from 12.1 through 14.1, but the GCC fix has been backported to the 12.X and 13.X branches. It may be possible to reduce the workaround's scope once those are released.
1 parent d7c0ba3 commit 35220b7

File tree

3 files changed

+44
-2
lines changed

3 files changed

+44
-2
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ jobs:
88
strategy:
99
matrix:
1010
cpp-compiler: ["clang", "gcc"]
11+
options: ["-c opt", ""]
1112
steps:
1213
- uses: actions/checkout@v4
1314
- uses: bazel-contrib/[email protected]
@@ -16,7 +17,7 @@ jobs:
1617
disk-cache: "verify-pr:run-bazel-tests:${{ matrix.cpp-compiler }}"
1718
repository-cache: true
1819
- run: echo "CC=${{ matrix.cpp-compiler }}" >> $GITHUB_ENV
19-
- run: bazel test ...
20+
- run: bazel test ${{ matrix.options }} ...
2021
check-formatting:
2122
name: "Check Python formatting"
2223
runs-on: ubuntu-latest

runtime/cpp/emboss_defines.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,27 @@
161161
static_cast</**/ ::std::uintptr_t>((offset)))
162162
#endif // !defined(EMBOSS_CHECK_POINTER_ALIGNMENT)
163163

164+
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115033
165+
//
166+
// This affects Emboss: without the fix in the appropriate bug, we get a
167+
// miscompilation for some Emboss code, including some unit tests.
168+
//
169+
// It is not known whether the workaround on the Emboss side is complete, so
170+
// the recommendation is to move to a version of GCC that is not affected.
171+
// However, changing toolchains can be difficult, so we do provide a
172+
// workaround.
173+
#if !defined(EMBOSS_GCC_BUG_115033)
174+
#if defined(__clang__)
175+
#define EMBOSS_GCC_BUG_115033 0
176+
#elif __GNUC__ >= 12 && __GNUC__ <= 13
177+
#define EMBOSS_GCC_BUG_115033 1
178+
#elif __GNUC__ == 14 && __GNUC_MINOR__ < 2
179+
#define EMBOSS_GCC_BUG_115033 1
180+
#else
181+
#define EMBOSS_GCC_BUG_115033 0
182+
#endif
183+
#endif
184+
164185
// EMBOSS_NO_OPTIMIZATIONS is used to turn off all system-specific
165186
// optimizations. This is mostly intended for testing, but could be used if
166187
// optimizations are causing problems.

runtime/cpp/emboss_memory_util.h

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,8 +412,28 @@ class ContiguousBuffer final {
412412
explicit ContiguousBuffer(::std::nullptr_t) : bytes_{nullptr}, size_{0} {}
413413

414414
// Implicitly construct or assign a ContiguousBuffer from a ContiguousBuffer.
415+
#if !EMBOSS_GCC_BUG_115033
415416
ContiguousBuffer(const ContiguousBuffer &other) = default;
416-
ContiguousBuffer& operator=(const ContiguousBuffer& other) = default;
417+
ContiguousBuffer &operator=(const ContiguousBuffer &other) = default;
418+
#else
419+
// See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115033 for details on the
420+
// bug (determined by bisecting GCC).
421+
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114207 may also be relevant.
422+
//
423+
// A minimized example is available at https://godbolt.org/z/489z7z135
424+
//
425+
// It is not entirely clear how these definitions work around the GCC bug,
426+
// but they appear to. One notable difference (and also the main reason that
427+
// we only use these definitions for affected versions of GCC) is that they
428+
// change the ABI of ContiguousBuffer, at least in the minimized case.
429+
ContiguousBuffer(const ContiguousBuffer &other)
430+
: bytes_{other.bytes_}, size_{other.size_} {}
431+
ContiguousBuffer &operator=(const ContiguousBuffer &other) {
432+
bytes_ = other.bytes_;
433+
size_ = other.size_;
434+
return *this;
435+
}
436+
#endif
417437

418438
// Explicitly construct a ContiguousBuffers from another, compatible
419439
// ContiguousBuffer. A compatible ContiguousBuffer has an

0 commit comments

Comments
 (0)