Hi Vincent, On Sun, Mar 27, 2016 at 05:41:08PM +0200, Vincent Bernat wrote: > I am trying to look if there are other cases of alignment problem. Some > time ago, gcc was emitting a warning if this was the case (-Wcast-align) > but there was too many false positives and this is not the case anymore. > > It would be easier to spot the cast problems if there was less of > them.
Oh I totally agree with you, I even wrote it in the coding style guide that most of the time if there's a cast, there's a dormant bug. A cast is a way to tell the compiler "shut up I know what I'm doing" and when you're seeing casts from/to void*, you know the person knows less than the compiler, which makes the whole line very suspicious. > Willy, you always cast for functions returning void*. This is not > needed. For example: > > l = (struct listener *)calloc(1, sizeof(struct listener)); > > Could be just: > > l = calloc(1, sizeof(struct listener)); I definitely agree. This code is quite old (2005) and we don't do that much cleanup passes unfortunately. Oh and yes by the way, in case people have doubts about this, I wrote it and used to proceed like this in the past, it offered a good protection against blind copy-paste and/or incorrect backports... But after that I preferred to change for : l = calloc(1, sizeof(*l)); which makes the backport work easier and is even more robust (eg: when "l" is struct stream in 1.6 and struct session in 1.5, the old form would have compiled and done the wrong thing while the new one would have automatically adapted). > C always do the casting from void* to the appropriate type used on > LHS. I don't have reference, but this is C89, so even very old compilers > will do the casting just fine. Yes, even on gcc 1.37 (the oldest one I've used) it was already OK. > I can propose a (large) patch to remove all those casts (using a > semantic patch to not miss anything). Here is a preview (for master, > only src/): > > https://gist.github.com/dc61d8b035545dc24efd > > After this patch, this leaves 52 casts to be examined for alignment (git > grep '= (struct.*\*)', not rocket science). Of course, no need to really > apply the patch to examine those, but in the future, this may be easier > without those casts. I'm fine with a huge cleanup patch at once. We should just synchronize to limit the rejects. I currently have a few pending patches from Thierry and Baptiste I guess. > Patch is incomplete, I can also remove SSL_get_app_data casts, > > I see another problematic cast in src/shctx.c: > > cur = (struct shared_block *)((char *)prev + sizeof(struct shared_block)); > > But prev is already a "struct shared_block *". So it's OK. It would be > easier to read as: > > cur = prev + 1; Looks like a copy-paste of things which were mechanically changed. Or maybe the design evolved a few times from something unaligned to this one in the end. > I am pretty sure that in src/connection.c, make_tlv() and other TLV > handling stuff are problematic, unless there is some guarantee that TLV > are memory-aligned but as I suppose this is a wire format, this can't be > the case. We should check the proxy proto spec, I don't remember well. I *think* the fields were 32-bit aligned in PPv2. > I don't see any other problematic cases. To sum up: > > 1. can we remove uneeded void* cast? As much as possible yes! > 2. correct dns.c I don't know the exact bug but I'll trust you :-) > 3. correct TLV handling in connection.c I just checked and the TLV is 32-bit aligned so I guess we don't need any change there. > 4. simplify the occurrence in shctx.c (or not worth it) Yes, it's a good opportunity for it as well. thanks! Willy
