GNOME Bugzilla – Bug 557517
Please make byteswapping utilities behave consistently through static inlines
Last modified: 2018-05-24 11:36:45 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.
Created attachment 121166 [details] [review] Proof of concept patch
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?
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).
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.
Any hope to get this applied? :)
-- 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.