Skip to content

Conversation

@ilayn
Copy link
Contributor

@ilayn ilayn commented May 3, 2025

I am aware that this file went through many iterations and complex support is a constant headache, and if there is any more appetite left, I'd like to offer another minor change.

We have been having some warnings coming from non-C99-compliant compilers and C++ runs over SciPy since NumPy 2.0. This is mainly due to the npy_crealX/npy_cimagX functions, casting the input struct first and then dereferencing it in the new type, causing the strict-aliasing (type punning) rule to fire.

Here is a recent example how this manifests itself in the build process https://github.com/scipy/scipy/actions/runs/14770722795/job/41470198162#step:6:300

[snip]
 In file included from ..\scipy\_build_utils\src/npy_2_complexcompat.h:4,
                 from ../scipy/special/xsf_wrappers.h:13,
                 from ../scipy/special/xsf_wrappers.cpp:1:
C:\hostedtoolcache\windows\Python\3.12.10\x64\Lib\site-packages\numpy\_core\include/numpy/npy_math.h: In function 'npy_longdouble npy_creall(npy_clongdouble)':
C:\hostedtoolcache\windows\Python\3.12.10\x64\Lib\site-packages\numpy\_core\include/numpy/npy_math.h:422:13: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  422 |     return ((longdouble_t *) &z)[0];
      |            ~^~~~~~~~~~~~~~~~~~~~ 
[snip]

I don't know if the SciPy code is also commiting some C++ sins but the NumPy code can do better to avoid strange bugs. I don't claim to have the right solution but seems like the most obvious one to me to let the compiler handle the possible padding/endianness issues with a straight struct field access. However I don't have a strong opinion since I'm not a C++ expert.

This is also somewhat related to our ambition to be warning free at some point.

@ilayn ilayn force-pushed the unbark_npymath branch from 1767925 to 18cc962 Compare May 3, 2025 11:15
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label May 4, 2025
@ngoldbaum
Copy link
Member

Ping @lysnikolaou - do you happen to remember why you spelled these accesses this way originally?

Copy link
Member

@lysnikolaou lysnikolaou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Initially the #ifdef wasn't there, so casting to the corresponding type worked across all different complex types. Now that the #ifdef is there, using the struct field makes more sense.

@ngoldbaum
Copy link
Member

OK, in it goes. Thanks @ilayn!

@ngoldbaum ngoldbaum merged commit 83a4e6a into numpy:main May 6, 2025
73 checks passed
charris pushed a commit to charris/numpy that referenced this pull request May 6, 2025
…sts in `npy_math.h` for C++ runs (numpy#28892)

* MAINT: Avoid type-punning complex casts in npy_math.h

* MAINT: Add missing typecast in npy_cimagl
@charris charris changed the title MAINT: Avoid dereferencing/strict aliasing warnings during complex casts in npy_math.h for C++ runs MAINT: Avoid dereferencing/strict aliasing warnings May 6, 2025
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label May 6, 2025
@ilayn ilayn deleted the unbark_npymath branch May 6, 2025 23:29
MaanasArora pushed a commit to MaanasArora/numpy that referenced this pull request May 8, 2025
…sts in `npy_math.h` for C++ runs (numpy#28892)

* MAINT: Avoid type-punning complex casts in npy_math.h

* MAINT: Add missing typecast in npy_cimagl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants