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

Force inlining of the inline functions #222

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@xen0n

xen0n commented May 30, 2016

inline is not a requirement but only a suggestion, hence the compiler
has the freedom to not really compile the marked functions as inline.
Under some situations the affected functions could end up undefined in
the compiled library, causing its loading to fail!

For GCC at least, adding an always_inline attribute is enough to force
inlining. Not sure if this breaks other compilers but I cannot test, so
the definition is guarded by a check for GCC.

Really fixes #180, which is also a problem on, at least, Gentoo with GCC
5.3.0.

Evidence:

(before)
$ nm ujson.gcc-4.9.3.so | grep 'strreverse\|Buffer_AppendShortHex'
0000000000007557 T Buffer_AppendShortHexUnchecked
0000000000007557 t Buffer_AppendShortHexUnchecked.localalias.1
000000000000805d T strreverse
000000000000805d t strreverse.localalias.0

$ nm ujson.gcc-5.3.0.so | grep 'strreverse\|Buffer_AppendShortHex'
                 U Buffer_AppendShortHexUnchecked
                 U strreverse

(after)
$ nm ujson.gcc-5.3.0-always_inline.so | grep 'strreverse\|Buffer_AppendShortHex'
(no output)
Also mark the inline functions as always_inline
`inline` is not a requirement but only a suggestion, hence the compiler
has the freedom to not really compile the marked functions as inline.
Under some situations the affected functions could end up undefined in
the compiled library, causing its loading to fail!

For GCC at least, adding an `always_inline` attribute is enough to force
inlining. Not sure if this breaks other compilers but I cannot test, so
the definition is guarded by a check for GCC.

Really fixes #180, which is also a problem on, at least, Gentoo with GCC
5.3.0.

WGH- added a commit to WGH-/ultrajson that referenced this pull request Aug 27, 2016

added "static" to C functions, where possible
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (esnme#237, esnme#180, esnme#222), which manifests
   when compiled with GCC>=5.
@Jahaja

This comment has been minimized.

Show comment
Hide comment
@Jahaja

Jahaja Oct 10, 2016

Member

I haven't been able to repro this when trying on FreeBSD but let us know if this still is an issue after #238.

Member

Jahaja commented Oct 10, 2016

I haven't been able to repro this when trying on FreeBSD but let us know if this still is an issue after #238.

@Jahaja Jahaja closed this Oct 10, 2016

@sdlarsen

This comment has been minimized.

Show comment
Hide comment
@sdlarsen

sdlarsen Dec 20, 2016

I still get this error.

import ujson ImportError: /home/.../venv/np_service/lib/python3.5/site-packages/ujson.cpython-35m-x86_64-linux-gnu.so: undefined symbol: Buffer_AppendShortHexUnchecked
I've tried with --no-binary all as well to no avail

My gcc: gcc (Gentoo 5.4.0 p1.0, pie-0.6.5) 5.4.0

sdlarsen commented Dec 20, 2016

I still get this error.

import ujson ImportError: /home/.../venv/np_service/lib/python3.5/site-packages/ujson.cpython-35m-x86_64-linux-gnu.so: undefined symbol: Buffer_AppendShortHexUnchecked
I've tried with --no-binary all as well to no avail

My gcc: gcc (Gentoo 5.4.0 p1.0, pie-0.6.5) 5.4.0

cordalace added a commit to cordalace/ultrajson that referenced this pull request Nov 13, 2017

added "static" to C functions, where possible
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (esnme#237, esnme#180, esnme#222), which manifests
   when compiled with GCC>=5.

Patch info:
    Author: WGH <wgh@torlan.ru>
    Date:   Sat Aug 27 17:34:22 2016 +0300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment