capi-workgroup / decisions

Discussion and voting on specific issues
4 stars 1 forks source link

C standard for public headers #30

Open encukou opened 1 week ago

encukou commented 1 week ago

Previous discussion: https://github.com/capi-workgroup/problems/issues/42 & https://github.com/capi-workgroup/api-evolution/issues/22

It seems the consensus of the people involved is that Python.h should be usable with:

Now that there's a report about the 3.13 headers containing a C11 feature (anonymous structs/unions), I guess it's time to make an official guideline.

vstinner commented 1 week ago

The C API is now tested by test_cext and test_cppext: these tests checks that including <Python.h> does not issue any compiler warning.

test_cext has 5 tests using -Werror -Werror=declaration-after-statement:

test_cppext has 4 tests using -Werror:

Without extra compiler flags, we're good. The problem of the issue gh-120293 are the usage of -Werror=pedantic and -Werror=cast-qual. I don't think that we want to support these flags.

encukou commented 1 week ago

This issue is not about the tests, but about the C standard we want the headers to support. After we decide that, we can test as much of it as we can, and make triage easy for issues about untestable things. But IMO we should write this down explicitly; it shouldn't be “whatever the tests do”.

Again, the tests currently pass even though the headers use a feature that's not in the C99 standard. If the intent is to follow the standard, then the test should change.

vstinner commented 1 week ago

Again, the tests currently pass even though the headers use a feature that's not in the C99 standard.

Well, "standard" is one thing, "what's being used in practice" is something else. No C compiler respect the standard by default, they add "compiler extensions", and require special compiler options to respect the standard.

For example, unnamed unions are supposed by all C compilers used by Python (GCC, clang, MSC) in C99 mode.

So the question is if we care about "strict standard" or "what's being used in practice by C compilers supported by Python" (so including compiler extensions).

In practice, the question is if we want to support -Werror=pedantic or not.

I proposed to not support -Werror=pedantic nor -Werror=cast-qual options. It's too much work, Python has more and more static inline functions, and it's tricky to respect the "pedantic" mode.

encukou commented 1 week ago

So, you're saying that we should only try to be compatible with specific compilers (and compiler options), rather than C standards?

Python has more and more static inline functions

I don't think supporting C89 is on the table, so static inline functions aren't really relevant.

vstinner commented 1 week ago

So, you're saying that we should only try to be compatible with specific compilers (and compiler options), rather than C standards?

Yes.

I don't think supporting C89 is on the table, so static inline functions aren't really relevant.

I was referring to the fact that static inline function bodies expose a lot of code and usually compiler problems come from such code. For example, of pyatomic functions would be regular opaque functions, we wouldn't have this discussion about -Werror=cast-qual. In the past, we also got many cast issues with C++. I also had to introduce _Py_NULL to use nullptr in C++ in our Python C API :-)

encukou commented 1 week ago

Well, I don't see why you brought up -Werror=cast-qual here. If we support specific compiler options, adding that one should be a separate conversation.

Anyway, I think I understand your opinion now; I just disagree with it. I guess it's time for others to chime in.

gvanrossum commented 1 week ago

I am confused though. Can you cleanly summarize the two opinions on the table?

eli-schwartz commented 1 week ago
$ cat /tmp/foo.c
#include <Python.h>

With c99:

$ gcc -std=c99 -Werror=pedantic -I/usr/include/python3.13 -c /tmp/foo.c -o /tmp/foo.o
In file included from /usr/include/python3.13/Python.h:92,
                 from /tmp/foo.c:1:
/usr/include/python3.13/cpython/code.h:32:10: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   32 |         };
      |          ^
/usr/include/python3.13/cpython/code.h:34:6: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   34 |     };
      |      ^
/usr/include/python3.13/cpython/code.h:27:9: error: struct has no named members [-Werror=pedantic]
   27 | typedef struct {
      |         ^~~~~~
In file included from /usr/include/python3.13/Python.h:128:
/usr/include/python3.13/cpython/optimizer.h:60:14: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   60 |             };
      |              ^
/usr/include/python3.13/cpython/optimizer.h:62:10: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   62 |         };
      |          ^
/usr/include/python3.13/cpython/optimizer.h:63:6: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   63 |     };
      |      ^
cc1: some warnings being treated as errors

With gnu99:

$ gcc -std=gnu99 -Werror=pedantic -I/usr/include/python3.13 -c /tmp/foo.c -o /tmp/foo.o
In file included from /usr/include/python3.13/Python.h:92,
                 from /tmp/foo.c:1:
/usr/include/python3.13/cpython/code.h:32:10: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   32 |         };
      |          ^
/usr/include/python3.13/cpython/code.h:34:6: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   34 |     };
      |      ^
/usr/include/python3.13/cpython/code.h:27:9: error: struct has no named members [-Werror=pedantic]
   27 | typedef struct {
      |         ^~~~~~
In file included from /usr/include/python3.13/Python.h:128:
/usr/include/python3.13/cpython/optimizer.h:60:14: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   60 |             };
      |              ^
/usr/include/python3.13/cpython/optimizer.h:62:10: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   62 |         };
      |          ^
/usr/include/python3.13/cpython/optimizer.h:63:6: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]
   63 |     };
      |      ^
cc1: some warnings being treated as errors

With c11:

$ gcc -std=c11 -Werror=pedantic -I/usr/include/python3.13 -c /tmp/foo.c -o /tmp/foo.o
$

@vstinner,

Well, "standard" is one thing, "what's being used in practice" is something else. No C compiler respect the standard by default, they add "compiler extensions", and require special compiler options to respect the standard.

This has nothing to do with "compiler extensions". It is about the fact that some compilers, like GCC, will allow any code that is legal by a later standard to also compile with a newer standard if the alternative is an error. The theory is that "well you know, your meaning is obvious, you wrote code that is legal in c23 but not c17, so clearly you meant to build with c17". This doesn't actually mean it's safe to ignore the fact that it compiles by default.

You can get the same issue if you upgrade to future gcc 15, and start using c23 features. As long as there isn't an incompatible alternative meaning in earlier standards, the compiler typically accepts it. Hopefully with a warning that you're doing something which isn't ISO C99.

... which brings us back to using -Werror=pedantic, since that offers a chance to catch cases where you're using code that isn't legal for a std but the compiler is accepting it anyways "to be nice".

If it was a compiler extension it would be allowed when compiling with "GNU C", the collection of GCC vendor extensions on top of standard C. But it is not.

eli-schwartz commented 1 week ago

I am confused though. Can you cleanly summarize the two opinions on the table?

If I understand correctly, the options are:

Option 2 requires mandating specific compilers, and also specific compiler versions (e.g. require that GCC is at least "whichever version of GCC first implemented c11 unnamed unions").

vstinner commented 1 week ago

code.h:32:10: error: ISO C99 doesn’t support unnamed structs/unions [-Werror=pedantic]

The problem is not specific to code.h. I added __extension__ (GCC/clang) and __pragma(warning(disable: 4201)) (MSC) to object.h to make a similar warning quiet in the Free Threading mode, on the unnamed union:

#ifndef Py_GIL_DISABLED
struct _object {
#if (defined(__GNUC__) || defined(__clang__)) \
        && !(defined __STDC_VERSION__ && __STDC_VERSION__ >= 201112L)
    // On C99 and older, anonymous union is a GCC and clang extension
    __extension__
#endif
#ifdef _MSC_VER
    // Ignore MSC warning C4201: "nonstandard extension used:
    // nameless struct/union"
    __pragma(warning(push))
    __pragma(warning(disable: 4201))
#endif
    union {
       Py_ssize_t ob_refcnt;
#if SIZEOF_VOID_P > 4
       PY_UINT32_T ob_refcnt_split[2];
#endif
    };
#ifdef _MSC_VER
    __pragma(warning(pop))
#endif

    PyTypeObject *ob_type;
};
#else
(...)
#endif

So it seems like C11 is what we need/want here.

vstinner commented 1 week ago

Now that there's a https://github.com/python/cpython/issues/120293 about the 3.13 headers containing a C11 feature (anonymous structs/unions), I guess it's time to make an official guideline.

I suggest to require C11 and C++03 at minimum to use the Python C API.

encukou commented 1 week ago

@guido, sorry for the delay.

I am confused though. Can you cleanly summarize the two opinions on the table?

1. Document the standards we support.

The C API guidelines will list the C/C++ standards that #include <Python.h> is meant to comply with. The list is open for nitpicking; I propose:

We'd welcome PRs to improve conformance, and to make the tests better in verifying conformance. (Within reason of course -- the guidelines can always be changed.)

2. Don't.

We support specific compilers, as listed in PEP 11. The supported compiler options are defined in the tests/CI.

(That's my understanding; I'll let Victor clarify.)

vstinner commented 1 week ago
  1. Don't. We support specific compilers, as listed in PEP 11. The supported compiler options are defined in the tests/CI. (That's my understanding; I'll let Victor clarify.)

That's not my intent. I mean that in practice, we can support C99 with compiler extensions, such as __extension__.

vstinner commented 1 week ago

I mean that in practice, we can support C99 with compiler extensions, such as extension.

Anyway, as I wrote previously, I agree with Petr and I suggest to require C11 to get unnamed unions and other nice C11 features.

zooba commented 1 week ago

I suggest to require C11 to get unnamed unions and other nice C11 features

I don't think we can reasonably require it in our headers (any of the headers that we distribute, "internal" or not), until the build backends used by package publishers have released stable versions that automatically add in the compiler options necessary to enable these modes.

This isn't a decision that affects us - it affects our users, who will suddenly find that they cannot compile with our newest releases due to code that they can't change.

I'd prefer to jump through as many hoops as possible to avoid breaking our users like that. I think it's worth the effort to not use these features in public header files (again, whether "internal" or not) so that we don't put the burden on our users. As far as I'm aware, nothing is broken for us, we just have to do a little bit more typing in our own code.

vstinner commented 1 week ago

I'd prefer to jump through as many hoops as possible to avoid breaking our users like that.

My attempt to fix the issue for PyCode and PyOptimizer APIs: https://github.com/python/cpython/pull/120643