Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Use of #undef complex in npy_common.h breaks extension modules that use #include <complex.h> #24344

Closed
WarrenWeckesser opened this issue Aug 5, 2023 · 11 comments · Fixed by #24364

Comments

@WarrenWeckesser
Copy link
Member

WarrenWeckesser commented Aug 5, 2023

Describe the issue:

The line #undef complex in the file npy_common.h that was added in #24085 breaks the compilation of any extension module that uses both npy_common.h and the standard header <complex.h>. (See, for example, the extension module cross_gufunc.c.src in ufunclab.)

A fix would be to remove that line from npy_common.h, but that will break any code that the includes npy_common.h that also uses the name complex to define a variable or struct. This is the case for several files in NumPy, including the f2c-related files in the lapack_lite code (see my naive attempt in #24343).

Transitive includes and the global namespace in C... such fun.

Runtime information:

In [1]: import sys, numpy; print(numpy.__version__); print(sys.version)
2.0.0.dev0+766.g8627dc962
3.11.1 (v3.11.1:a7a450f84a, Dec  6 2022, 15:24:06) [Clang 13.0.0 (clang-1300.0.29.30)]

In [2]: print(numpy.show_runtime())
[{'numpy_version': '2.0.0.dev0+766.g8627dc962',
  'python': '3.11.1 (v3.11.1:a7a450f84a, Dec  6 2022, 15:24:06) [Clang 13.0.0 '
            '(clang-1300.0.29.30)]',
  'uname': uname_result(system='Darwin', node='Warrens-MacBook-Air.local', release='22.5.0', version='Darwin Kernel Version 22.5.0: Thu Jun  8 22:21:34 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T8112', machine='arm64')},
 {'simd_extensions': {'baseline': [], 'found': [], 'not_found': []}}]
None

Context for the issue:

No response

@WarrenWeckesser
Copy link
Member Author

In theory, the line #undef complex could be removed from npy_common.h, and all existing uses of the name complex in NumPy could be fixed (e.g. just change the names to, say, complex_). That would fix the NumPy build, but the change to npy_common.h will also break third party extension modules that happen to use the name complex in their code. On the other hand, we are currently breaking third party extension modules that use #include <complex.h>.

@rgommers
Copy link
Member

rgommers commented Aug 6, 2023

In theory, the line #undef complex could be removed from npy_common.h, and all existing uses of the name complex in NumPy could be fixed (e.g. just change the names to, say, complex_).

That sounds like the right fix at first sight.

That would fix the NumPy build, but the change to npy_common.h will also break third party extension modules that happen to use the name complex in their code.

We're requiring C99 and complex is a keyword there, so using complex as a variable name seems bad practice to me. I think it's fine to require those users to fix up their code.

On the other hand, we are currently breaking third party extension modules that use #include <complex.h>.

That seems like something we should not do indeed, arguably that's a bug in NumPy.

@WarrenWeckesser WarrenWeckesser added this to the 2.0.0 release milestone Aug 7, 2023
@WarrenWeckesser
Copy link
Member Author

Now I see that @mattip also pointed out this problem in #24198.

@seberg
Copy link
Member

seberg commented Aug 7, 2023

Ping @lysnikolaou for awareness.

@lysnikolaou
Copy link
Contributor

Agreed that we should leave the #undef out and fix the rest of the issues. The problem with fixing the ones in #24198 is that it's in code we vendor in from f2c, so it's unclear to me how we would go about it. I can try to have a look either way.

@rgommers
Copy link
Member

rgommers commented Aug 7, 2023

I think post-processing the f2c-generated code inside make_lite.py probably?

@lysnikolaou
Copy link
Contributor

lysnikolaou commented Aug 7, 2023

The problem with that is that it's not only in f2c-generated files, but in the version of f2c itself that we vendor as well (in the files f2c.h and f2c.c).

@mattip
Copy link
Member

mattip commented Aug 7, 2023

There is an issue about f2c maintenance on a (the?) github mirror, where it seems the way to contact the netlib maintainer is via email (see the bottom of the README).

@mattip
Copy link
Member

mattip commented Aug 7, 2023

with that, I don't think we have to solve all of the problems. We can do what we need to do, and point users to the potential pitfalls of using f2c.

@rgommers
Copy link
Member

rgommers commented Aug 7, 2023

We're not exposing f2c for anyone else to use, so I'm not sure that's all that relevant. We should be able to add an f2c.c.patch like for some of the other .patch files under linalg/lapack_lite/ I'd think? Either that or post-processing via Python code, a sed script, or whatever works.

@lysnikolaou
Copy link
Contributor

lysnikolaou commented Aug 7, 2023

We should be able to add an f2c.c.patch like for some of the other .patch files under linalg/lapack_lite/ I'd think? Either that or post-processing via Python code, a sed script, or whatever works.

Not sure whether that's necessary. As far as I understand, the f2c.[ch] files are just copy-pasted from the libf2c source, so that they can be used in case f2c is not installed. Just adding/modifying the patches for f2c-generated files and changing f2c.c and f2c.h directly (maybe together with a note in the README in that directory) seems good enough for me.

lysnikolaou added a commit to lysnikolaou/numpy that referenced this issue Aug 8, 2023
- Change complex to is_complex in multiarray/buffer.c
- Change all complex occurences in vendored f2c to singlecomplex
- Modify patches for f2c-generated code and regenerate

Closes numpy#24344.
FFY00 pushed a commit to FFY00/numpy that referenced this issue Aug 14, 2023
- Change complex to is_complex in multiarray/buffer.c
- Change all complex occurences in vendored f2c to singlecomplex
- Modify patches for f2c-generated code and regenerate

Closes numpy#24344.

Signed-off-by: Filipe Laíns <lains@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants