6 ms·
The Byte Order Fiasco (2021)
- deleted 2y ago[deleted]
- throw0101b 2y ago> Clang and GCC are reaching for any optimization they can get. Undefined Behavior may be hostile and dangerous, but it's legal. So don't let your code become a casualty. Perhaps we need a -Wundefined-behaviour so that compilers print out messages when they use those type of 'tricks'. If you see them you can then choose to adjust your code in a way that it follows defined path(s) of the standard(s) in question.
- Uvix 2y agoIsn't that what -fsanitize=undefined does?
- MaulingMonkey 2y ago`-fsanitize=undefined` enables some runtime warnings/checks, `-Wundefined-behaviour` would presumably enable some kind of compile time warnings/checks.
- saagarjha 2y agoYou’ll have far too many messages.
- tedunangst 2y agoDo you want a warning on every for (int i = 0; i < n; i++) loop?
- saurik 2y agoYes: that loop is miserable and we should upgrade it to use size_t (and preferably ++i), at which point the undefined behavior required to make that abomination fast wouldn't matter anymore.
- JonChesterfield 2y agoWhy would you prefer to preincrement an integer? Too much time around C++ iterators?
- saurik 2y agoI mean, why would you ever prefer to post-increment an integer whose prior value you didn't need? Not enough time studying iterators to make that distinction obvious, perhaps? Regardless, the context here is that we should stop relying so much on the compiler to paper over differences in code that don't by the user anything except a form of quaint familiarity with old code: the i++ relies on the optimizer knowing it doesn't have to keep the prior value, while ++i doesn't, and yet somehow I guess you are enamored with i++ despite it being exactly the same characters to type, merely in a different order. Stop using int, stop using post-increment, and sure: maybe even stop using < when you semantically want != (though I explicitly didn't go there because that isn't--as far as I know--going to affect the performance with a dumber compiler).
- JonChesterfield 2y agoPattern matching is a big thing in programming. Not in C++, as ++i executes arbitrary code found an unbounded distance from the programmer (so ++i vs i++ might be a very important distinction) but in other languages it helps a lot. for (int i=0, j=0; i < N && j < M; i+=2, j++) i++ looks like i+=1 so fits better in loops iterating over multiple things. Also that int says "these values don't overflow" very concisely and the more modern cleverer C++ using iterator facades probably fails to communicate that idea to anyone. edit: amused to note on checking for typos that I wrote that with < instead of != which violates your other recommendation, which is one I'm in favour of in theory but apparently don't write out by default.
- karatinversion 2y agoI always thought the problem with this was that the compilers do loads of these optimisations in very mundane ways. Eg if I have a #define FOO 17 void bar(int x, int y) { if (x + y >= FOO) { //do stuff } } void baz(int x) { bar(x, FOO); } the compiler can inline the call to bar in baz, and then optimise the condition to (x>=0)… because signed integer overflow is undefined, so can’t happen, so the two conditions are equivalent. The countless messages about optimisations like that would swamp ones about real dangerous optimisations.
- im3w1l 2y agoHow much is actually gained by removing checks like these, and is it worth the cost? Can we find a safer approach to making the code fast?
- somat 2y agoI have to admit I don't really understand the logic behind how compiler writers got to the point where they were able to interpret "undefined behavior" as "will not happen" My understanding is that undefined behavior is in the spec because the various physical machines that C could compile to would each do something different. So to avoid stepping on toes the spec basically punted and said we are not going to define what happens. Originally this was fine. the C compiler would dutifully generate the machine instructions and the machine would execute them returning the result. The wrench was thrown into the works when we started getting optimizing compilers. I would argue the correct interpretation of "undefined behavior" in the face of a optimizing compiler is "unknown" but then it would sort of behave like the sql NULL(each unknown is different from every other unknown) and act as an optimization fence. When you have to ask the machine what the result of this operation is, it is hard to optimize around it ahead of time. So my best guess as to why they decided to read "undefined" as "can not happen" is that this is the interpretation they can best optimize for. And nobody really pushed back because what the hell does "undefined" mean anyway? My read is that if the spec wanted to say "can not happen" the spec would have said "can not happen"
- karatinversion 2y ago
- userbinator 2y agoOr better yet, perhaps they should just do the sane thing instead of being a hostile pedantic smartass. Undefined shouldn't mean "whatever", it should be an opportunity to consider what makes the most sense.
- amelius 2y agoBut then you have to write a standard.
- lmm 2y agoYou actually don't. Plenty of languages and compilers manage to be less programmer-hostile without a written standard. Unfortunately GCC/Clang have prioritised getting higher numbers on benchmarks.
- favorited 2y ago> Unfortunately GCC/Clang have prioritised getting higher numbers on benchmarks. Because that's what people actually want. There's no untapped market for a C compiler that doesn't aggressively optimize. The closest you'll get is projects selectively disabling certain optimizations, like how the Linux kernel builds with `-fno-strict-aliasing`.
- lmm 2y ago> There's no untapped market for a C compiler that doesn't aggressively optimize. There probably isn't now, because people who want sanity have moved on from C.I do think that years ago a different path was possible. > The closest you'll get is projects selectively disabling certain optimizations, like how the Linux kernel builds with `-fno-strict-aliasing`. Linux also does `-fno-delete-null-pointer-checks` and probably others. I don't think they have a principled way of deciding, rather whenever an aggressive optimization causes a terrible bug they disable that particular one and leave the rest on.
- userbinator 2y ago
- circuit10 2y agoIt’s not always possible to predict in advance when undefined behaviour can happen
- pjc50 2y agoLast time I suggested this it was unpopular. https://news.ycombinator.com/item?id=40787276 https://news.ycombinator.com/item?id=40787276 (even for the relatively high-profile and easy-sounding "detect if the compiler has deleted a line of code in a way which alters observable behavior of the program"!)
- throwaway2037 2y agoDo any static analysis tools (commercial or open source) already do this now?
- IshKebab 2y agoThese days I think the sane option is to just add a static assert that the machine is little endian and move on with your life. Unless you're writing glibc or something do you really care about supporting ancient IBM mainframes? Also recommending octal is sadistic!
- bvrmn 2y ago> add a static assert that the machine is little endian and move on with your life It's not clear how it would free you from interpreting BE data from incoming streams/blobs.
- forrestthewoods 2y agoI feel like we're at a point where you should assume little endian serialization and treat anything big endian as a slow path you don't care about. There's no real reason for any blob, stream, or socket to use big endian for anything afaict. If some legacy system still serializes big endian data then call bswap and call it a day.
- bvrmn 2y agoAFAIK quite a number of protocols and file formats use BE without any sign to become a legacy even in a distant future.
- saagarjha 2y agoYou do realize that most of the networking stack is big-endian, right?
- syncsynchalt 2y agoThe internet is big-endian, and generally data sent over the wire is converted to/from BE. For example the numbers in IP or TCP headers are big-endian, and any RFC that defines a protocol including binary data will generally go with big-endian numbers. I believe this dates from Bolt Baranek and Newman basing the IMP on a BE architecture. Similarly computers tend to be LE these days because that's what the "winning" PC architecture (x86) uses.
- o11c 2y agoWhen I dealt with this, there were a couple major gotchas: * Compilers seem to reliably detect byteswap, but are(were) very hit-or-miss with the shift patterns for reading/writing to memory directly, so you still need(ed) an ifdef. I know compilers have improved but there are so many patterns that I'm still paranoid. * There are a lot of "optimized" headers that actually pessimize by inserting inline assembly that the compiler can't optimize through (in particularly, the compiler can't inline constants and can't choose `movbe` instead of `bswap`), so do not trust any standard API; write your own with memcpy + ifdef'd C-only swapping. * For speaking wire protocols, generating (struct-based?) code is far better than writing code that mentions offsets directly, which in turn is far better than the `mempcpy`-like code which the link suggests.
- mjevans 2y agoI can't remember the source, but AFAIK packed structs aren't mandated in the specification. It's terrible because a programmer will OFTEN see that result, on any architecture they're familiar with; however it isn't guaranteed to be portable. One of the top google hits when I searched for 'c packed structs' https://stackoverflow.com/questions/4306186/structure-padding-and-packing https://stackoverflow.com/questions/4306186/structure-paddin...
- o11c 2y agoWow, that SO post is full of very bad answers. Regardless ... Even ignoring compiler extensions, you don't have to memcpy the struct to the network directly (though this necessarily must be doable safely due to the fact that <net/> headers use it). Instead, define an API that takes loose structs, and have generated code do all the swapping and packing from the struct form to the bytes form. If you're careful, you might make it such that on common platforms this optimizes into just a memcpy. But if the struct can get inlined this doesn't actually matter.
- pjc50 2y agoC programmers say things like "C is close to the machine"(+) and then have to write a huge elaborate macro in order to fool the compiler into generating a single machine instruction which is the one they actually want. One of these days I'll write my "actually typesafe portable macro assembler" project. It will be .. opinionated. (+) which machine, though
- akira2501 2y ago> Now you don't need to use those APIs because you know the secret. Was that a desired outcome? The endian.3 and byteorder.3 manual pages make it easy.
- pizlonator 2y ago> Modern compiler policies don't even permit code that looks like like that anymore. Your compiler might see that and emit assembly that formats your hard drive with btrfs. This is total FUD. Some sanitizers might be unhappy with that code, but that's just sanitizers creating problems where there need not be any. The llvm policy here is that must alias trumps TBAA, so clang will reliably compile the cast of char* to uint32_t* and do what systems programmers expect. If it didn't then too much stuff would break.
- trealira 2y ago> Mask, and then shift. > Repeat it like a mantra. You mask first, to define away potential concerns about signedness. Then you cast if needed. And finally, you can shift. Now we can create the fool-proof version for machines with at least 32-bit ints: #define READ32BE(p) \ (uint32_t)(255 & p[0]) << 24 | (255 & p[1]) << 16 | (255 & p[2]) << 8 | (255 & p[3]) I don't think this works just because she's masking it. I'm pretty sure it's working because she cast (255 & p[0]) to uint32_t, and so all the other operands get promoted to uint32_t as well. I have this working with just casting to unsigned char first: #include <stdio.h> #include <stdint.h> char b[4] = {0x02,0x03,0x04,0x80}; #define UC(x) ((unsigned char) (x)) #define READ32BE(p) (uint32_t)UC(p[3]) | UC(p[2]) << 8 | UC(p[1]) << 16 | UC(p[0]) << 24 int main(void) { printf("%08x\n", READ32BE(b)); } $ cc -fsanitize=undefined -g -Os -o /tmp/o endian.c && /tmp/o 02030480 Edit: actually, it works for me even without casting to uint32_t, without UBSan causing a runtime error, like in the article, so I don't know what's going on.
- moefh 2y ago> I don't think this works just because she's masking it. I'm pretty sure it's working because she cast (255 & p[0]) to uint32_t You're right, masking does nothing to solve the problem, what does it is the cast to uint32_t. Masking only helps if you're working with (signed) char, which is a bit silly. Your example works without the cast because the example in the article doesn't show the problem. That is, the following code doesn't have undefined behavior: char b[4] = {0x02,0x03,0x04,0x80}; #define READ32BE(p) p[3] | p[2]<<8 | p[1]<<16 | p[0]<<24 int main(int argc, char *argv[]) { printf("%08x\n", READ32BE(b)); } Note that b[0] (which is being shifted by 24) is 0x02, so everything works. To show the problem you'd need something like this (b[0] must be >= 128): char b[4] = {0x82,0x03,0x04,0x80};
- trealira 2y agoThanks for the explanation! That makes sense. I knew from reading K&R that, in converting a char to an int without sign extension, casting to unsigned char first is just as good as masking by 0xFF after, and I guess I was just not sufficiently familiar with the type promotion rules of C. This now causes a runtime error: #include <stdio.h> #include <stdint.h> char b[4] = {0x82,0x03,0x04,0x80}; #define UC(x) ((unsigned char) (x)) #define READ32BE(p) (uint32_t)UC(p[3]) | UC(p[2]) << 8 | UC(p[1]) << 16 | UC(p[0]) << 24 int main(void) { printf("%08x\n", READ32BE(b)); } ---- endian.c:8:22: runtime error: left shift of 130 by 24 places cannot be represented in type 'int' 82030480 But this works: #include <stdio.h> #include <stdint.h> char b[4] = {0x82,0x03,0x04,0x80}; #define CH_to_U32(x) ((uint32_t) (unsigned char) (x)) #define READ32BE(p) (CH_to_U32(p[3]) \ | CH_to_U32(p[2]) << 8 \ | CH_to_U32(p[1]) << 16 \ | CH_to_U32(p[0]) << 24) int main(void) { printf("%08x\n", READ32BE(b)); } ---- 82030480
- zzo38computer 2y agoI always just write the code with shifts in any program that cares about endianness (e.g. when reading/writing files), except for dealing with internet (which is big-endian) (in which case the functions specifically for dealing with endianness with internet, will be used). However, in addition to big-endian and small-endian, sometimes PDP-endian is used, such as Hamster archive file, which some of my programs use. (The same way of using shifts can be used, like above.) MMIX is big-endian, and has MOR and MXOR instructions, either of which can be used to deal with endianness (including PDP-endian; these instructions have other uses too). (However, my opinion is that small-endian is better than big-endian, but any one will work.)
- jwrallie 2y agoI never thought about doing bit shifts using octal numbers.. I definitely will be doing 010, 020, 030, ... instead of 8, 16, 24, ... from now on!
- orlp 2y agoPlease don't.
- 1vuio0pswjnm7 2y agoWas the author allowed to fix the Tensorflow example?
- dang 2y agoDiscussed at the time: The Byte Order Fiasco - https://news.ycombinator.com/item?id=27085952 https://news.ycombinator.com/item?id=27085952 - May 2021 (364 comments)
- kazinator 2y agoPlease, do not write shift amounts in octal. Or anything else in octal. Mask after shifting, so you can use the same mask. It's less verbose and probably easier to optimize, based on the simple pattern that an expression of the form: EXPR & MOD is being converted to a type which is MOD + 1 bits wide. From this: #define WRITE64BE(P, V) \ ((P)[0] = (0xFF00000000000000 & (V)) >> 070, \ (P)[1] = (0x00FF000000000000 & (V)) >> 060, \ (P)[2] = (0x0000FF0000000000 & (V)) >> 050, \ (P)[3] = (0x000000FF00000000 & (V)) >> 040, \ (P)[4] = (0x00000000FF000000 & (V)) >> 030, \ (P)[5] = (0x0000000000FF0000 & (V)) >> 020, \ (P)[6] = (0x000000000000FF00 & (V)) >> 010, \ (P)[7] = (0x00000000000000FF & (V)) >> 000, (P) + 8) To this: #define WRITE64BE(P, V) \ ((P)[0] = ((V) >> 56) & 0xFF, \ (P)[1] = ((V) >> 48) & 0xFF, \ (P)[2] = ((V) >> 40) & 0xFF, \ (P)[3] = ((V) >> 32) & 0xFF, \ (P)[4] = ((V) >> 24) & 0xFF, \ (P)[5] = ((V) >> 16) & 0xFF, \ (P)[6] = ((V) >> 8) & 0xFF, \ (P)[7] = (V) & 0xFF, (P) + 8) It doesn't help anyone that the multiples of 8 look nice in octal, because these decimals are burned into hackers' brains. And are you gonna use base 13 for when multiples of 13 look good? Save the octal for chmod 0777 public-dir. Anyone who doesn't know this leading zero stupidity perpetrated by C reads 070 as "seventy". Even 78, 68, 58, 48, ... would be better, and also take 3 characters: #define WRITE64BE(P, V) \ ((P)[0] = ((V) >> 7*8) & 0xFF, \ (P)[1] = ((V) >> 6*8) & 0xFF, \ (P)[2] = ((V) >> 5*8) & 0xFF, \ (P)[3] = ((V) >> 4*8) & 0xFF, \ (P)[4] = ((V) >> 3*8) & 0xFF, \ (P)[5] = ((V) >> 2*8) & 0xFF, \ (P)[6] = ((V) >> 8) & 0xFF, \ (P)[7] = (V) & 0xFF, (P) + 8) though we are relying on knowing that * has higher precedence than >>. Supplementary note: when we have the 0xFF in this position, and P has type "unsigned char", we can remove it. If P is unsigned char, the & 0xFF does nothing, except on machines where chars are wider than 8 bits. These fall into two categories: historic museum hardware your code will never run on and DSP chips. On DSP chips, bytes wider than 8 doesn't mean 9 or 10, but 16 or more. Simply doing & 0xFF may not make the code work. You may have to pack the code into 16 bit bytes in the right byte order. (Been there and done that. E.g. I worked on an ARM platform that had a TeakLite III DSP, where that sort of thing was done in communicating data between the host and the DSP). So we are actually okay with: #define WRITE64BE(P, V) \ ((P)[0] = (V) >> 56), \ (P)[1] = (V) >> 48), \ (P)[2] = (V) >> 40), \ (P)[3] = (V) >> 32), \ (P)[4] = (V) >> 24), \ (P)[5] = (V) >> 16), \ (P)[6] = (V) >> 8), \ (P)[7] = (V) , (P) + 8)
- el_pollo_diablo 2y agoLook, it's not that difficult: static inline uint32_t load_le32(void const *p) { unsigned char const *p_ = p; _Static_assert(sizeof(uint32_t) == 4, "uint32_t is not 4 bytes long"); return (uint32_t)p_[0] << 0 | (uint32_t)p_[1] << 8 | (uint32_t)p_[2] << 16 | (uint32_t)p_[3] << 24; } static inline void store_le32(void *p, uint32_t v) { unsigned char *p_ = p; _Static_assert(sizeof(uint32_t) == 4, "uint32_t is not 4 bytes long"); p_[0] = (unsigned char)(v >> 0); p_[1] = (unsigned char)(v >> 8); p_[2] = (unsigned char)(v >> 16); p_[3] = (unsigned char)(v >> 24); } All that nonsense about masking with an int must stop. You want a result in uint32_t? Convert to that type before shifting. Done. Now, the C standard could still get in the way and assign a lower conversion rank to uint32_t than to int, so uint32_t operands would get promoted to int (or unsigned int) before shifting, but those left shifts would still be defined as the result would be representable in the promoted type (as promotion only makes sense if it preserves values, which implies that all values of the type before promotion are representable in the type after promotion).
- matheusmoreira 2y agoThe Linux user space API byte swap functions: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/swab.h https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/byteorder/little_endian.h https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/byteorder/big_endian.h https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... https://github.com/torvalds/linux/blob/master/include/uapi/linux/swab.h https://github.com/torvalds/linux/blob/master/include/uapi/l... https://github.com/torvalds/linux/blob/master/include/uapi/linux/byteorder/little_endian.h https://github.com/torvalds/linux/blob/master/include/uapi/l... https://github.com/torvalds/linux/blob/master/include/uapi/linux/byteorder/big_endian.h https://github.com/torvalds/linux/blob/master/include/uapi/l... They are quite similar if not equivalent to the code presented in this article. I assume people far smarter than me have run these things under sanitizers and found no issues. I mean, it's Linux. #include <asm/byteorder.h>
- jowea 2y agoCan't we just declare __alignof__(uint32_t) on the 'char b[4]' and then freely use bswap without all this madness? Or memcpy the chars into a uint32 array? https://gcc.gnu.org/onlinedocs/gcc/Alignment.html https://gcc.gnu.org/onlinedocs/gcc/Alignment.html