Skip to content

Commit

Permalink
unix: BOLT fixes
Browse files Browse the repository at this point in the history
As part of investigating failures with BOLT when upgrading to LLVM
19, I found and fixed a few issues with BOLT.

First, `test_embed` had been segfaulting on BOLT instrumented binaries.
Why I'm not entirely sure. But the segfault only seems to occur in
instrumentation mode. These tests are doing low-level things with the
interpreter. So I suspect some kind of global mutable state issue
or something.

I found the exact tests triggering the segfaults and added annotations
to skip them.

The CPython build system treats the segfault as fatal on 3.13 but
not 3.12. This means that on 3.12 we were only running a subset of tests
and not collecting BOLT instrumentation nor applying optimizations for
all tests after `test_embed`.

The removal of the segfault enables us to enable BOLT on 3.13+.

Second, LLVM 19.x has a hard error when handling PIC compiled functions
containing computed gotos. It appears prior versions of LLVM could
silently have buggy behavior in this scenario. We need to skip functions
with computed gotos to allow LLVM 19.x to work with BOLT. It makes sense
to apply this patch before LLVM 19.x upgrade to prevent bugs with
computed gotos.

Third, I noticed BOLT was complaining about the lack of
`-update-debug-sections` during instrumentation.

The 2nd and 3rd issues require common arguments to both BOLT
instrumentation and application invocations. The patch fixing both
introduces a new configure variable to hold common BOLT arguments.
This patch is a good candidate for upstreaming.
  • Loading branch information
indygreg committed Jan 1, 2025
1 parent 916bb0d commit 50395d7
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 6 deletions.
20 changes: 14 additions & 6 deletions cpython-unix/build-cpython.sh
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ if [ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_11}" ]; then
patch -p1 -i ${ROOT}/patch-pwd-remove-conditional.patch
fi

# Adjust BOLT flags to yield better behavior. See inline details in patch.
if [ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_12}" ]; then
patch -p1 -i ${ROOT}/patch-configure-bolt-flags.patch
fi

# The optimization make targets are both phony and non-phony. This leads
# to PGO targets getting reevaluated after a build when you use multiple
# make invocations. e.g. `make install` like we do below. Fix that.
Expand Down Expand Up @@ -287,6 +292,14 @@ if [ -n "${CROSS_COMPILING}" ]; then
patch -p1 -i ${ROOT}/patch-force-cross-compile.patch
fi

# BOLT instrumented binaries segfault in some test_embed tests for unknown reasons.
# On 3.12 (minimum BOLT version), the segfault causes the test harness to
# abort and BOLT optimization uses the partial test results. On 3.13, the segfault
# is a fatal error.
if [ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_10}" ]; then
patch -p1 -i ${ROOT}/patch-test-embed-prevent-segfault.patch
fi

# Most bits look at CFLAGS. But setup.py only looks at CPPFLAGS.
# So we need to set both.
CFLAGS="${EXTRA_TARGET_CFLAGS} -fPIC -I${TOOLS_PATH}/deps/include -I${TOOLS_PATH}/deps/include/ncursesw"
Expand Down Expand Up @@ -387,12 +400,7 @@ fi

if [ -n "${CPYTHON_OPTIMIZED}" ]; then
CONFIGURE_FLAGS="${CONFIGURE_FLAGS} --enable-optimizations"
if [[ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_13}" && -n "${BOLT_CAPABLE}" ]]; then
# Due to a SEGFAULT when running `test_embed` with BOLT instrumented binaries, we can't use
# BOLT on Python 3.13+.
# TODO: Find a fix for this or consider skipping these tests specifically
echo "BOLT is disabled on Python 3.13+"
elif [[ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_12}" && -n "${BOLT_CAPABLE}" ]]; then
if [[ -n "${PYTHON_MEETS_MINIMUM_VERSION_3_12}" && -n "${BOLT_CAPABLE}" ]]; then
CONFIGURE_FLAGS="${CONFIGURE_FLAGS} --enable-bolt"
fi
fi
Expand Down
50 changes: 50 additions & 0 deletions cpython-unix/patch-configure-bolt-flags.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
diff --git a/configure.ac b/configure.ac
index bc8c357e996..eef55d4839a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2104,6 +2104,27 @@ AS_VAR_IF([enable_shared], [yes], [
BOLT_BINARIES="${BOLT_BINARIES} \$(INSTSONAME)"
])

+AC_ARG_VAR(
+ [BOLT_COMMON_FLAGS],
+ [Common arguments to llvm-bolt when instrumenting and applying]
+)
+
+AC_MSG_CHECKING([BOLT_COMMON_FLAGS])
+if test -z "${BOLT_COMMON_FLAGS}"
+then
+ AS_VAR_SET(
+ [BOLT_COMMON_FLAGS],
+ [m4_normalize("
+ [-update-debug-sections]
+
+ dnl At least LLVM 19.x doesn't support computed gotos in PIC compiled code.
+ dnl Exclude functions containing computed gotos.
+ dnl TODO this may be fixed in LLVM 20.x via https://github.com/llvm/llvm-project/pull/120267.
+ [-skip-funcs=_PyEval_EvalFrameDefault,sre_ucs1_match/1,sre_ucs2_match/1,sre_ucs4_match/1]
+ ")]
+ )
+fi
+
AC_ARG_VAR(
[BOLT_INSTRUMENT_FLAGS],
[Arguments to llvm-bolt when instrumenting binaries]
@@ -2111,7 +2132,7 @@ AC_ARG_VAR(
AC_MSG_CHECKING([BOLT_INSTRUMENT_FLAGS])
if test -z "${BOLT_INSTRUMENT_FLAGS}"
then
- BOLT_INSTRUMENT_FLAGS=
+ BOLT_INSTRUMENT_FLAGS="${BOLT_COMMON_FLAGS}"
fi
AC_MSG_RESULT([$BOLT_INSTRUMENT_FLAGS])

@@ -2125,7 +2146,7 @@ then
AS_VAR_SET(
[BOLT_APPLY_FLAGS],
[m4_normalize("
- -update-debug-sections
+ ${BOLT_COMMON_FLAGS}
-reorder-blocks=ext-tsp
-reorder-functions=hfsort+
-split-functions
20 changes: 20 additions & 0 deletions cpython-unix/patch-test-embed-prevent-segfault.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py
index 13713cf37b8..ba23880b15f 100644
--- a/Lib/test/test_embed.py
+++ b/Lib/test/test_embed.py
@@ -1615,6 +1615,7 @@ def test_getpath_abspath_win32(self):
for (_, expected), result in zip(CASES, results):
self.assertEqual(result, expected)

+ @unittest.skip("segfaults on BOLT instrumented binaries")
def test_global_pathconfig(self):
# Test C API functions getting the path configuration:
#
@@ -1866,6 +1867,7 @@ def test_no_memleak(self):
self.assertEqual(blocks, 0, out)


+@unittest.skip("segfaults on BOLT instrumented binaries")
class StdPrinterTests(EmbeddingTestsMixin, unittest.TestCase):
# Test PyStdPrinter_Type which is used by _PySys_SetPreliminaryStderr():
# "Set up a preliminary stderr printer until we have enough

0 comments on commit 50395d7

Please sign in to comment.