classification
Title: Create a release branch ABI stability regression test
Type: enhancement Stage: patch review
Components: Build, C API, Interpreter Core, Tests Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, gregory.p.smith, lukasz.langa, ned.deily, pablogsal, petr.viktorin, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2021-04-04 04:35 by gregory.p.smith, last changed 2021-04-28 14:59 by pablogsal.

Files
File name Uploaded Description Edit
compat_report.html pablogsal, 2021-04-04 18:19
Pull Requests
URL Status Linked Edit
PR 25188 closed pablogsal, 2021-04-04 20:33
PR 25230 merged pablogsal, 2021-04-06 22:01
PR 25232 merged pablogsal, 2021-04-06 22:09
PR 25322 merged pablogsal, 2021-04-09 21:45
PR 25323 merged pablogsal, 2021-04-09 21:49
PR 25394 merged pablogsal, 2021-04-13 22:45
Messages (34)
msg390173 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-04-04 04:35
In order to automate prevention of ABI regressions in stable releases, we could create an automated ABI stability test generator and check the specific ABI test it generates into each specific release branch.

I'm envisioning the main branch only having a code generator that creates such a test, and the release branches only having the output of that as Lib/tests/release_X_Y_ABI_stability_test.py and a policy of never updating that within a release branch without *extreme* attention to detail.

Such updates wouldn't happen by default in our current workflow as they're unique and versioned in every release branch so automated backport PRs wouldn't touch them - leaving CI to run them and highlight failures on attempted backports to do inadvertently cause an ABI shift.
msg390184 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 12:24
This is probably complementary or in the avenue of https://www.python.org/dev/peps/pep-0652/
msg390203 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-04-04 17:38
Indeed.  In particular given the 3.9.3 issue I was assuming such a test should include asserting both the sizeof() ABI structs and offsetof() public members of all ABI structs.  On each specific first class supported platform.

This goes beyond what https://www.python.org/dev/peps/pep-0652/#testing-the-stable-abi currently states to check size and layout rather than just symbol presence.  But seems to match the intent.
msg390205 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 18:15
We could have a buildbot using https://github.com/lvc/abi-compliance-checker
msg390207 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 18:19
For example, the tool generates this report for the two 3.9 versions (attached to the issue).
msg390208 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 18:35
Also, we can use libabigail. For instance:

7a3947dec3d8">root@7a3947dec3d8:/pytho# abidiff Python-3.9.2/python Python-3.9.3/python
Functions changes summary: 0 Removed, 3 Changed (53 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added variable

3 functions with some indirect sub-type change:

  [C]'function void PyEval_AcquireThread(PyThreadState*)' at ceval.c:381:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          4 data member changes (2 filtered):
           'char _ts::recursion_critical' offset changed from 296 to 320 (in bits) (by +24 bits)
           'int _ts::stackcheck_counter' offset changed from 320 to 352 (in bits) (by +32 bits)
           'int _ts::tracing' offset changed from 352 to 384 (in bits) (by +32 bits)
           'int _ts::use_tracing' offset changed from 384 to 416 (in bits) (by +32 bits)
          1 data member change:
           type of 'char _ts::overflowed' changed:
             type name changed from 'char' to 'int'
             type size changed from 8 to 32 (in bits)
           and name of '_ts::overflowed' changed to '_ts::recursion_headroom' at pystate.h:61:1

  [C]'function int _PyErr_CheckSignalsTstate(PyThreadState*)' at signalmodule.c:1684:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          no data member changes (6 filtered);
          no data member change (1 filtered);

  [C]'function void _PyErr_Clear(PyThreadState*)' at errors.c:426:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          no data member changes (6 filtered);
          no data member change (1 filtered);
msg390213 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 21:11
Ok, so seems that PR25188 works if the abi dump file for the "correct" version is generated with the same compiler that is used to check the ABI. I think this is acceptable if the workflow is:

- As soon as a version is released, we generate in the stable release branch the dump using some docker container with the same compiler as the GitHub CI.

- We enable the ABI check only for release branches.

- We should make the check mandatory because release managers cannot be checking all backport PRs unfortunately.

We make the changes for 3.8 and 3.9 for the time being.
msg390214 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-04 21:13
Adding the rest of RM to evaluate the proposed solution
msg390310 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-06 11:14
Anything is better than nothing, from my POV. Let's get it running, tweak it, and if it doesn't work for us then take it down.
msg390311 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-06 11:15
Ok, will create PRs for the release branches that are receiving fixes
msg390312 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-06 11:17
> Also, we can use libabigail.

RHEL uses that to provide ABI guarantees on the kernel and the glibc.
msg390313 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2021-04-06 11:20
Do we need separate jobs and ABI dumps for each platform and arch? I guess we need at least separate dumps for 32 and 64bit.
msg390315 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-06 11:57
> Do we need separate jobs and ABI dumps for each platform and arch? I guess we need at least separate dumps for 32 and 64bit.

Not really, check what happened in my 64 build system when I did the changes that broke the ABI in the latest 3.9 release:

7a3947dec3d8">7a3947dec3d8">root@7a3947dec3d8:/pytho# abidiff Python-3.9.2/python Python-3.9.3/python
Functions changes summary: 0 Removed, 3 Changed (53 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added variable

3 functions with some indirect sub-type change:

  [C]'function void PyEval_AcquireThread(PyThreadState*)' at ceval.c:381:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          4 data member changes (2 filtered):
           'char _ts::recursion_critical' offset changed from 296 to 320 (in bits) (by +24 bits)
           'int _ts::stackcheck_counter' offset changed from 320 to 352 (in bits) (by +32 bits)
           'int _ts::tracing' offset changed from 352 to 384 (in bits) (by +32 bits)
           'int _ts::use_tracing' offset changed from 384 to 416 (in bits) (by +32 bits)
          1 data member change:
           type of 'char _ts::overflowed' changed:
             type name changed from 'char' to 'int'
             type size changed from 8 to 32 (in bits)
           and name of '_ts::overflowed' changed to '_ts::recursion_headroom' at pystate.h:61:1

  [C]'function int _PyErr_CheckSignalsTstate(PyThreadState*)' at signalmodule.c:1684:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          no data member changes (6 filtered);
          no data member change (1 filtered);

  [C]'function void _PyErr_Clear(PyThreadState*)' at errors.c:426:1 has some indirect sub-type changes:
    parameter 1 of type 'PyThreadState*' has sub-type changes:
      in pointed to type 'typedef PyThreadState' at pystate.h:20:1:
        underlying type 'struct _ts' at pystate.h:51:1 changed:
          type size hasn't changed
          no data member changes (6 filtered);
          no data member change (1 filtered);
msg390316 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-04-06 12:00
Not sure what platforms libabigail works on, but the set of stable ABI symbols is platform-specific. Currently it's affected by the MS_WINDOWS and HAVE_FORK defines.
msg390319 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-06 12:08
I assume it's only doing source analysis, so we can probably force those flags on for the check? Or run multiple checks with the options, but without having to switch platform.

Even under MS_WINDOWS, I hope we're not using declarations from the Windows header files in our own public API. If so, those are worth replacing (safely, over time).
msg390325 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-06 12:40
If I understand correctly, this is analyzing the DWARF information on the binaries, so is quite coupled to the platform you compile to, but you can detect violations that affect other platforms if they share the same code.
msg390372 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-04-06 20:42
Ideally we'd analyze for a representative set of major platforms we ship binaries on.  32-bit and 64-bit Linux are a start, but we should assume that at least windows and linux toolchains may be different and toss in an additional CPU architecture as default alignment constraints could differ.  But getting to that level can remain future work...  I'm happy libabigail exists!
msg390374 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-06 20:44
Yeah, I subscribe what Greg said.

Let's start with *something* and let's improve upon.
msg390382 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-06 22:19
I started with two PRs against the 64 bits versions for 3.9 and 3.8. This *should* cover 32 bits as well (see previous messages), but if we want specific changes for that we need a 32 bit machine or cross-compilation, but I decided to start with something (that something being 64 bits)
msg390660 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-09 19:47
I got a false positive on my PR at https://github.com/python/cpython/pull/25318/checks?check_run_id=2308871807

1 Changed variable:

  [C]'const unsigned char[45154] const _Py_M__importlib_bootstrap_external' was changed to 'const unsigned char[43681] const _Py_M__importlib_bootstrap_external' at importlib_external.h:2:1:
    size of symbol changed from 45154 to 43681
    type of variable changed:
     'const unsigned char[45154] const' changed to 'const unsigned char[43681] const'


Is there an option to exclude array lengths? Or to treat the API as just a pointer rather than an array?
msg390668 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-09 21:19
> Is there an option to exclude array lengths? Or to treat the API as just a pointer rather than an array?

Technically is not a false positive:

https://developers.redhat.com/blog/2019/05/06/how-c-array-sizes-become-part-of-the-binary-interface-of-a-library/

But in this case I think it is. We can have a ignore file:

https://sourceware.org/libabigail/manual/libabigail-concepts.html#suppr-spec-label

We could ignore all functions that start with _Py.
msg390670 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2021-04-09 22:05
Oh wow, that's terrible... yet another good reason not to export data values.

But yeah, filtering on the name prefix should be fine. These aren't meant to be publicly accessible anyway.
msg390985 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-13 18:55
> Oh wow, that's terrible... yet another good reason not to export data values.

_Py_M__importlib_bootstrap_external symbol doesn't seem to be exported. Why is it seen as a public symbol?

$ objdump -T /lib64/libpython3.10.so.1.0|grep _Py_M__importlib_bootstrap_external
# empty output
msg390986 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-13 18:58
> We could ignore all functions that start with _Py.

Some symbols starting with _Py are indirectly part of the ABI. Example of Include/cpython/pyctype.h:

#define Py_ISLOWER(c)  (_Py_ctype_table[Py_CHARMASK(c)] & PY_CTF_LOWER)
PyAPI_DATA(const unsigned int) _Py_ctype_table[256];

Even if "_Py_ctype_table" is not directly part of the C API, it's technically part of the ABI.

If tomorrow, _Py_ctype_table is truncated to 128 items, it would be an incompatible ABI change.
msg390987 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-13 19:00
In a Python stable version, I would suggest to only ignore an ABI change after a manual validation .Otherwise, we can miss real issues.

Well, I expect that at the beginning, we will discover many issues like _Py_M__importlib_bootstrap_external ;-)
msg390994 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-13 19:37
Ok, then we need to revert by latest 2 PRs and add a new label or something to skip
msg391007 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-13 22:36
> Ok, then we need to revert by latest 2 PRs and add a new label or something to skip

To skip ABI tests? I proposed to ignore ABI changes if they can be ignored safely. But I don't know right now which changes can be ignored or not.
msg391008 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-13 22:39
> To skip ABI tests? I proposed to ignore ABI changes if they can be ignored safely. But I don't know right now which changes can be ignored or not.

As the situation stands we need to choose on keeping my changes to ignore _Py function changes or revert it. Further than that is unexplored land
msg391009 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-13 22:42
> $ objdump -T /lib64/libpython3.10.so.1.0|grep _Py_M__importlib_bootstrap_external
# empty output

This was happening on 3.8 Victor:

d9c5942e274b">root@d9c5942e274b:/src# objdump -T libpython3.8.so | grep _Py_M__importlib_bootstrap_external
0000000000335740 g    DO .rodata        000000000000b062  Base        _Py_M__importlib_bootstrap_external
msg391010 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-13 22:44
I am going to revert the 3.9 PR as 3.9+ is hiding symbols by default.
msg391011 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-13 23:07
> I got a false positive on my PR at https://github.com/python/cpython/pull/25318/checks?check_run_id=2308871807

Oh, this CI failure was on Python 3.8.

Since Python 3.9, Python is now built with -fvisibility=hidden to avoid exporting symbols which are not explicitly exported. So in 3.9, we no longer have to ignore all symbols prefixed by "_Py".
msg392205 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-04-28 14:36
I cannot merge my PR 25685 because the mandatory ABI CI job fails:
---------------
abidiff "libpython3.9.so" ./Doc/data/python3.9.abi --drop-private-types --no-architecture --no-added-syms
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'function int _PyInterpreterState_IDIncref(PyInterpreterState*)' at pystate.c:497:1 has some indirect sub-type changes:
    return type changed:
      type name changed from 'int' to 'void'
      type size changed from 32 to 0 (in bits)
---------------

It is correct that my PR changes an internal C API on purpose.


Pablo suggests me to regenerate the ABI file but I don't know how to do that.


In Python 3.9, the GitHub Action uses:
---
  check_abi:
    name: 'Check if the ABI has changed'
    runs-on: ubuntu-20.04
    needs: check_source
    if: needs.check_source.outputs.run_tests == 'true'
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v2
      - name: Install Dependencies
        run: |
            sudo ./.github/workflows/posix-deps-apt.sh
            sudo apt-get install -yq abigail-tools
      - name: Build CPython
        env:
          CFLAGS: -g3 -O0
        run: |
          # Build Python with the libpython dynamic library
          ./configure --enable-shared
          make -j4
      - name: Check for changes in the ABI
        run: make check-abidump
---

I'm using Fedora 33 with "gcc (GCC) 11.0.1 20210324 (Red Hat 11.0.1-0)".

On Fedora, I used:
---
$ sudo dnf install -y libabigail
$ ./configure --enable-shared CFLAGS="-g3 -O0" && make -j10
$ make regen-abidump
$ git diff --stat
 Doc/data/python3.9.abi | 28478 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 18306 insertions(+), 10172 deletions(-)
---

There are tons of changes!

Also, "make check-abidump" lists many changes:
---
abidiff "libpython3.9.so" ./Doc/data/python3.9.abi --drop-private-types --no-architecture --no-added-syms
Functions changes summary: 0 Removed, 13 Changed (171 filtered out), 0 Added functions
Variables changes summary: 0 Removed, 6 Changed (2 filtered out), 0 Added variables

13 functions with some indirect sub-type change:
(...)
  [C] 'function PyStatus _PyInterpreterState_Enable(_PyRuntimeState*)' at pystate.c:171:1 has some indirect sub-type changes:

  [C] 'function int _Py_DecodeLocaleEx(const char*, wchar_t**, size_t*, const char**, int, _Py_error_handler)' at fileutils.c:574:1 has some indirect sub-type changes:
    parameter 6 of type 'typedef _Py_error_handler' has sub-type changes:

  [C] 'const unsigned char _Py_ctype_tolower[256]' was changed to 'const unsigned char[256] const _Py_ctype_tolower' at pyctype.h:26:1:
    type of variable changed:
      entity changed from 'const unsigned char[256]' to 'const unsigned char[256] const'
(...)
---
msg392207 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-28 14:58
As I mentioned here:

https://bugs.python.org/msg390213

the dump needs to be generated in a docker container using the same compiler version that is used in the CI
msg392208 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-04-28 14:59
You are using Fedora, which is not the same docker container and likely the same compiler version that is used to check the dump
History
Date User Action Args
2021-04-28 14:59:07pablogsalsetmessages: + msg392208
2021-04-28 14:58:25pablogsalsetmessages: + msg392207
2021-04-28 14:36:47vstinnersetmessages: + msg392205
2021-04-13 23:07:32vstinnersetmessages: + msg391011
2021-04-13 22:45:46pablogsalsetpull_requests: + pull_request24127
2021-04-13 22:44:25pablogsalsetmessages: + msg391010
2021-04-13 22:42:25pablogsalsetmessages: + msg391009
2021-04-13 22:39:01pablogsalsetmessages: + msg391008
2021-04-13 22:36:42vstinnersetmessages: + msg391007
2021-04-13 19:37:13pablogsalsetmessages: + msg390994
2021-04-13 19:00:41vstinnersetmessages: + msg390987
2021-04-13 18:58:09vstinnersetmessages: + msg390986
2021-04-13 18:55:24vstinnersetmessages: + msg390985
2021-04-09 22:05:28steve.dowersetmessages: + msg390670
2021-04-09 21:49:14pablogsalsetpull_requests: + pull_request24058
2021-04-09 21:45:07pablogsalsetpull_requests: + pull_request24057
2021-04-09 21:20:00pablogsalsetmessages: + msg390668
2021-04-09 19:47:31steve.dowersetmessages: + msg390660
2021-04-06 22:19:12pablogsalsetmessages: + msg390382
2021-04-06 22:09:53pablogsalsetpull_requests: + pull_request23969
2021-04-06 22:01:10pablogsalsetpull_requests: + pull_request23967
2021-04-06 20:44:40pablogsalsetmessages: + msg390374
2021-04-06 20:42:03gregory.p.smithsetmessages: + msg390372
2021-04-06 12:40:04pablogsalsetmessages: + msg390325
2021-04-06 12:08:01steve.dowersetmessages: + msg390319
2021-04-06 12:00:37petr.viktorinsetmessages: + msg390316
2021-04-06 11:57:25pablogsalsetmessages: + msg390315
2021-04-06 11:20:52christian.heimessetnosy: + christian.heimes
messages: + msg390313
2021-04-06 11:17:05vstinnersetnosy: + vstinner
messages: + msg390312
2021-04-06 11:15:23pablogsalsetmessages: + msg390311
2021-04-06 11:14:23steve.dowersetmessages: + msg390310
2021-04-04 21:13:06pablogsalsetnosy: + ned.deily, lukasz.langa, steve.dower
messages: + msg390214
2021-04-04 21:11:30pablogsalsetmessages: + msg390213
2021-04-04 20:33:14pablogsalsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request23930
2021-04-04 18:35:54pablogsalsetmessages: + msg390208
2021-04-04 18:19:02pablogsalsetfiles: + compat_report.html

messages: + msg390207
2021-04-04 18:15:32pablogsalsetmessages: + msg390205
2021-04-04 17:38:00gregory.p.smithsetmessages: + msg390203
2021-04-04 12:24:32pablogsalsetnosy: + petr.viktorin, pablogsal
messages: + msg390184
2021-04-04 04:35:13gregory.p.smithcreate