After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 557517 - Please make byteswapping utilities behave consistently through static inlines
Please make byteswapping utilities behave consistently through static inlines
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: general
2.18.x
Other All
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2008-10-22 23:18 UTC by Diego Elio Pettenò
Modified: 2018-05-24 11:36 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Proof of concept patch (1.15 KB, patch)
2008-10-22 23:19 UTC, Diego Elio Pettenò
none Details | Review
Testcase for static inline and __builtin_constant_p (561 bytes, text/plain)
2008-10-23 00:00 UTC, Diego Elio Pettenò
  Details
Complete patch (12.00 KB, patch)
2008-10-23 12:30 UTC, Diego Elio Pettenò
none Details | Review

Description Diego Elio Pettenò 2008-10-22 23:18:35 UTC
Right now the behaviour of the byteswapping utilities in glib is not consistent between them and with other similar interfaces in other libraries.

In particular, there is no consistency on whether the arguments to byteswapping macros are evaluated once or twice (which does matter for side-effects; while the documentation does point this out, it is buried under some general information that most programmers wouldn't feel the need to read, and "experimental knowledge" may prove fatal (on x86 with optimisation turned on evaluation happen once; if a developer wrote a software without noticing the one-line warning, and got it to work for him, might ship something that breaks on, say, AMD64 where the 16-bit macros evaluate the argument twice).

Comparable interfaces from C libraries (bswap_*) and FFmpeg/libavutil only evaluate the parameter once.

Since the inline assembly versions of the macros already use GNU extensions (__extension__), an alternative could be to use static inline functions to provide the same interface, and yet evaluate the arguments just once.

Empirical tests between glibc's and FFmpeg's bswap macros show that at least on GCC 4.3 whether the code is emitted through a macro or a static inline, the resulting assembly code is 100% identical. On the other hand, it makes debugging easier since the code is no more just "glued" on where the macro is expanded, and an user might opt in to disable inlining altogether to step through those functions, if needed.

I'll attach a proof-of-concept patch changing one macro into static inline, I'll be happy to convert the rest of the code to this form if it's to be accepted.
Comment 1 Diego Elio Pettenò 2008-10-22 23:19:24 UTC
Created attachment 121166 [details] [review]
Proof of concept patch
Comment 2 Manish Singh 2008-10-22 23:36:42 UTC
Does __builtin_constant_p carry through in the inline? I suspect not, since the Linux kernel byteswap functions still use a macro for that case, e.g.:

#  define __swab16(x) \
(__builtin_constant_p((__u16)(x)) ? \
 ___constant_swab16((x)) : \
 __fswab16((x)))

__fswabXX is a static inline though.

The sentiment is reasonable though, so how about making the code like the kernel?
Comment 3 Diego Elio Pettenò 2008-10-23 00:00:13 UTC
Created attachment 121168 [details]
Testcase for static inline and __builtin_constant_p

This is a testcase using the static inline approach. Yes the __builtin_constant_p is still expanded by the compiler, if you build it with gcc -O2 -S you'll see that the _constant case returns a numeric literal instead of executing rorw on that constant.

I also tried ICC (which works as expected) and Sun Studio Express 11 on Linux (which does not expand inlines, I'd expect that to be a compiler bug, and I'd have to check it on Solaris, since there has been a few inconsistencies between the two even at the same version).
Comment 4 Diego Elio Pettenò 2008-10-23 12:30:52 UTC
Created attachment 121194 [details] [review]
Complete patch

Okay this should do it, the patch is written over trunk but it should apply fine to 2.18 branch as well.
Comment 5 Diego Elio Pettenò 2009-03-12 22:22:38 UTC
Any hope to get this applied? :)
Comment 6 GNOME Infrastructure Team 2018-05-24 11:36:45 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/167.