Hello Edmund, and everyone else following this, Here are my thoughts, not as a gate-keeper, but rather in the spirit of peer review.
In my own tcc hacking, I have looked closely at the cstring handling in token streams and always found this sort of code troubling: nb_words = (sizeof(CString) + cv->cstr->size + 3) >> 2; Doesn't this assume that ints are 4 bytes? Of course, if ints are actually 8 bytes, then the ensuing malloc simply allocates more room than we need, so it's not been a problem, but it nonetheless seemed a bit to loose for my taste. Your patch touches the offending lines, and it looks like it handles them correctly. What I'm not sure about, and would appreciate if somebody could check, is whether changing the contents of the union might lead to substantial increases in memory consumption. How many CValue-s are typically allocated and used during a regular compilation? The proposed change will alter a union the largest member of which used to be a pointer or 64-bit integer, and replaced it with a struct that contains an int and two pointers. On the other hand, allocating no more room than necessary for cstrings should reduce the consumed memory on 64-bit architectures. I believe Edmund that this fixes alignment issues, and this may also lead to better memory consumption, at least on 64-bit. Edmund, can you comment on any measurements of memory consumption during compilation due to this change? Any information on both 32- and 64-bit architectures would be great! David On Sat, Nov 21, 2015 at 7:06 AM, Edmund Grimley Evans < [email protected]> wrote: > I have removed from TCC all the cases of illegal/undefined C that I > know about, apart from one, which is particularly horrible: the way a > CString is copied into a token string, which is an int array: see > tok_str_add2. On a 64-bit architecture the pointers end up misaligned, > so ASan gives lots of warnings. On a 64-bit architecture that required > memory accesses to be correctly aligned it would not work at all. > > Here is a patch to cure the problem by putting the struct CString into > struct CValue instead. According to Valgrind, TCC does not leak memory > with this patch, though that's almost a miracle because it's very > unclear who is responsible for freeing the "data" in a string. Still, > unless someone has a better idea of how to do this, and is willing to > implement that better idea, I think this patch should go in. Opinions? > > Edmund > > _______________________________________________ > Tinycc-devel mailing list > [email protected] > https://lists.nongnu.org/mailman/listinfo/tinycc-devel > > -- "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." -- Brian Kernighan
_______________________________________________ Tinycc-devel mailing list [email protected] https://lists.nongnu.org/mailman/listinfo/tinycc-devel
