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

Force inlining of the inline functions #222

Closed
wants to merge 1 commit into from

Conversation

@xen0n
Copy link

@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)
`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
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (ultrajson#237, ultrajson#180, ultrajson#222), which manifests
   when compiled with GCC>=5.
@Jahaja
Copy link
Contributor

@Jahaja 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
Copy link

@sdlarsen 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
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (ultrajson#237, ultrajson#180, ultrajson#222), which manifests
   when compiled with GCC>=5.

Patch info:
    Author: WGH <wgh@torlan.ru>
    Date:   Sat Aug 27 17:34:22 2016 +0300
paul-theorem pushed a commit to pdlending/ultrajson that referenced this pull request Jan 9, 2020
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (ultrajson#237, ultrajson#180, ultrajson#222), which manifests
   when compiled with GCC>=5.
brettmason-tracmap added a commit to tracmap/ultrajson that referenced this pull request Jan 17, 2020
1. It reduces clutter in symbol table.
2. It fixes issues with C99 inline semantics for functions
   marked as inline (ultrajson#237, ultrajson#180, ultrajson#222), which manifests
   when compiled with GCC>=5.

(cherry picked from commit 6cf6c7f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.