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

Should not use __attribute__((optimize(…))) #2494

Closed
jwilk opened this issue Jul 13, 2018 · 9 comments
Closed

Should not use __attribute__((optimize(…))) #2494

jwilk opened this issue Jul 13, 2018 · 9 comments

Comments

@jwilk
Copy link
Contributor

jwilk commented Jul 13, 2018

GCC documentation says this about the optimize attribute:

This attribute should be used for debugging purposes only. It is not suitable in production code.

In practice, __attribute__((optimize("Os"))) seems to re-enable strict aliasing, despite -fno-strict-aliasing specified on command-line:

i686-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fdebug-prefix-map=/build/python2.7-5qX40H/python2.7-2.7.15=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/usr/include/python2.7 -c build/temp.linux-i386-2.7/src/decode.c -o build/temp.linux-i386-2.7/build/temp.linux-i386-2.7/src/decode.o -pthread
build/temp.linux-i386-2.7/src/decode.c: In function 'initdecode':
build/temp.linux-i386-2.7/src/decode.c:51244:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   __Pyx_INCREF(((PyObject *)(&PyString_Type)));
   ^~~~~~~~~~~~

(full build log)

I was going to file a bug against GCC, but then I realized the attribute shouldn't be used in the first place, and there's apparently plenty of bugs about it in GCC Bugzilla already.

@scoder
Copy link
Contributor

scoder commented Jul 22, 2018

Thanks for finding that out. The cold attribute seems to have a similar effect, though:
https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/Common-Function-Attributes.html#index-cold-function-attribute

@scoder scoder closed this as completed in 9ddac71 Jul 22, 2018
@charris
Copy link

charris commented Aug 2, 2018

@scoder Will Cython be changing the use of the attribute?

@charris
Copy link

charris commented Aug 2, 2018

I ask because if it is true that __attribute__((optimize("Os"))) re-enables strict aliasing, that would be a problem for NumPy.

@scoder
Copy link
Contributor

scoder commented Aug 2, 2018

Yes, this will change in 0.29. See 9ddac71.

@charris
Copy link

charris commented Aug 2, 2018

Do you have an estimate of the 0.29 timeline?

@scoder
Copy link
Contributor

scoder commented Aug 2, 2018

You can always define the C macro CYTHON_SMALL_CODE to nothing to disable this unconditionally.

I'd like to release it rather earlier than later, but there is no clear release timeframe. #2499 is the most important open ticket. Once that's solved, and it'll take a couple of person days to get right, I think I'd look into a release. So, expect a couple of weeks.

I'll also consider a 0.28.5 release in the meantime that would have this fix.

@mattip
Copy link
Contributor

mattip commented Aug 3, 2018

Just a FYI that numpy/numpy#11665 adopted defining CYTHON_SMALL_CODE= was merged for an upcoming NumPy 1.15.1, thanks for helping work through this.

scoder added a commit that referenced this issue Aug 3, 2018
@scoder scoder modified the milestones: 0.29, 0.28.5 Aug 3, 2018
@scoder
Copy link
Contributor

scoder commented Aug 3, 2018

Seeing that this causes actual trouble to projects, I decided to release 0.28.5 with (almost) only this fix. It's uploaded to PyPI.

@charris
Copy link

charris commented Aug 3, 2018

Thanks @scoder .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants