4 ms·
I mentioned this as an afterthought in a nested comment, but it bears repeating. The real problem here is not the bug in the memcmp() call, it's that you shoul
by Stratoscope 4y ago
I mentioned this as an afterthought in a nested comment, but it bears repeating.
The real problem here is not the bug in the memcmp() call, it's that you should not use a memory compare to compare two structs in a test at all.
If the structs have any padding, you are invoking undefined behavior if the padding is not initialized.
And as we've seen, memcmp() opens the door to all kinds of errors.
Don't rely on a compiler to catch this for you. Instead write the code correctly in the first place.
The author's struct has x, y, and z fields, and the right way to compare these structs is to explicitly compare these three fields.
For example, based on the author's code (using a typedef for the struct, and naming it xyz instead of tmp):
typedef int bool;
typedef struct {
int x, y, z;
} xyz;
bool xyz_equal( xyz one, xyz two ) {
return(
one.x == two.x &&
one.y == two.y &&
one.z == two.z
);
}
Now you can use xyz_equal() in your test code and it won't have any of the problems that the misuse of memcmp() caused.
- Stracke 4y ago[dead]
- lelanthran 4y ago> I mentioned this as an afterthought in a nested comment, but it bears repeating. I agree whole-heartedly. There is no situation in which `memcmp` should be used with structs. In code review this would have been flagged as an error, no matter how much the author cried "but but but ... there's no padding!". Even if there is no padding right now (compiler flags, or struct has perfectly aligned fields), at some point in the future this code will be reused in a spot where there will be padding (compiler upgrade, someone adds a new field at the beginning of the struct, etc). It's the same with serialising structs, you have to do it field by field.
- Stratoscope 4y agoThis was an interesting lesson for me. Not only did pretty much every commenter miss this point, but I did too! We were all talking about better ways to use memcmp(), such as whether to use the "!= 0" or not, or my memeq() wrapper. Those are all good things to consider if memcmp() is the right solution in the first place. I only had the "aha" moment after I took a break for a long walk and dinner. I should do that more often before assuming I have the right answer... :-) Maybe this is an example of what they call an XY Problem: https://en.wikipedia.org/wiki/XY_problem https://en.wikipedia.org/wiki/XY_problem
- cozzyd 4y agoSerializing structs directly is probably fine if you fully control the platform on both sides and you memset to 0 ahead of time (the latter probably only important if compressing or if you are worried about leaking data, which in a closed system you would typically not be...).
- xiphias2 4y agoIt's interesting that there was a post just today about initializing padding and other stack variables to 0 decreasing bugs in C/C++ by 10% without significant change in performance, this sounds like a great example.
- gkfasdfasdf 4y agoCan you share a link to that post? I must have missed it.
- xiphias2 4y agoIt was in the C++ meetup trip report (actually yesterday): https://news.ycombinator.com/item?id=33762154 https://news.ycombinator.com/item?id=33762154 https://twitter.com/jfbastien/status/1592578174763745280?ref_src=twsrc%5Etfw%7Ctwcamp%5Etweetembed%7Ctwterm%5E1592578174763745280%7Ctwgr%5E587e1f4eb046e527bc22e817fc4e32d9e42d0f08%7Ctwcon%5Es1_c10&ref_url=https%3A%2F%2Fcor3ntin.github.io%2Fposts%2Fkona22%2F https://twitter.com/jfbastien/status/1592578174763745280?ref...
- camel-cdr 4y agoIn general, memcmp, shouldn't be used for anything other than char or (u)intN_t arrays, because all other types can have padding bits. In general everything from the string.h header should be only used on non strings with great care.
- cozzyd 4y agoWill any compilers cause trouble if the struct is always memset to 0 on initialization? I suppose there's probably nothing preventing the compiler from randomly setting padding bytes to other values but I doubt that'd ever happen?
- hyperman1 4y agoI think it can. If you have, say char a; int b; in a struct, so there is padding in between. Then do s.a= x;s.b=y; If x and y happen to have the same memory layout, the compiler can replace the 2 assignements with 1 big transfer, and copy random things in the padding.
- cozzyd 4y agoInteresting! I guess most realistic scenarios would have x and y on the stack though in which case I bet they'd get optimized out completely under normal circumstances.
- hyperman1 4y agoTrue, it is a case of severe bad luck. But these are even worse than normal bugs: Some minor change in an unrelated part of the code triggers a cascade of different inlining decisions, and now a comparison is silently returning wrong answers in a very specific edge case. Another variant is having x as an int. It is supposed to truncate the int on assignement, but instead it simply dumps the high bits in the alignement. As the saying goes: Working code > crashing code > silent corruption.