--- !ditz.rubyforge.org,2008-03-06/issue title: DUK_TVAL_SET_TVAL corruption in FreeBSD + clang desc: |- To reproduce: * Compile in FreeBSD with clang 3.3: $ clang -v FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610 Target: x86_64-unknown-freebsd10.0 Thread model: posix * Additional options: -m32, DUK_PROFILE=101 * Hand tune duk_features.h to enable all log levels * The problem persists even when -fstrict-aliasing is NOT given, but disappears if -Os is changed to -O0. Incorrect behavior happens in multiple places (not sure if this is a clang/llvm bug or incorrect assumptions in Duktape aliasing related code). In duk_push_tval(), after DUK_TVAL_SET_TVAL(tv_slot, tv) has been called, the memory dump of the two regions are (first byte is from 'tv', second is from 'tv_slot'):: 60 60 32 32 81 81 28 28 00 00 00 00 f5 fd <== ff ff As can be seen, the second to last byte has been changed from F5 to FD, which causes all sorts of problems. One possible culprit may be the DUK_TVAL_SET_TVAL() macro itself, which simply uses *(dst) = *(src) for a duk_double_union - perhaps this is non-portable. The problem persist even when replaced by a memmove() or by (dst)->d = (src)->d. Copying the values byte by byte using the 'uc' part of the union fixes this particular problem:: #define DUK_TVAL_SET_TVAL(v,x) do { \ volatile int _i; \ for (_i = 0; _i < sizeof(duk_tval); _i++) { \ ((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \ } \ } while (0) Note that if you remove "volatile", the fix works in most call sites but not all! This suggests clang/llvm converts the hand-coded byte copy to some intrinsic which fails to honor the aliasing rules for char pointers. Another problem seems to happen in duk_hobject_props.c:realloc_props, at the line: new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i); The line copies a duk_propvalue to another using a plain assignment. Debug printing the results shows that the values get corrupted in the copy the same way as in DUK_TVAL_SET_TVAL. Some values copy OK (first is source, second is target): 00 00 00 00 00 00 00 00 00 00 00 00 f8 f8 7f 7f 00 00 00 00 00 00 00 00 00 00 00 00 f0 f0 7f 7f while others get corrupted again at their 7th byte (little endian memory layout). In more concrete terms, 0x08 gets OR'd to the 7th byte. Considering the double as a 64-bit big endian value, 0x0008'0000'0000'0000 gets OR'd to the value: 2c 2c cb cb ff ff ff ff 00 00 00 00 f1 f9 <== ff ff 10 10 49 49 81 81 28 28 00 00 00 00 f6 fe <== ff ff The bit in question is the highest bit of the fractional part. This can be similarly fixed by writing a manual byte-by-byte copy, again with a volatile loop counter. type: :bugfix component: duk release: reporter: sva status: :unstarted disposition: creation_time: 2013-12-10 01:28:45.479389 Z references: [] id: 4df1a3df6fcfc88b7515221dd083243df8747cb0 log_events: - - 2013-12-10 01:28:45.647113 Z - sva - created - "" - - 2013-12-10 01:30:44.318909 Z - sva - commented - |- Below is a diff of the two fixes mentioned in the description, applied to the all-in-one source, which together fix the crashes in FreeBSD + clang 3.3. The diff is not directly applicable because it is a bit hacky, but illustrates the idea of the fix. Removing the 'volatile' for either of the loop variables breaks the result. $ diff -u duktape-orig.c duktape-fixed.c --- duktape-orig.c 2013-12-10 04:28:08.000000000 +0200 +++ duktape-fixed.c 2013-12-10 04:28:15.000000000 +0200 @@ -5832,7 +5832,12 @@ #define DUK_TVAL_SET_BUFFER(v,h) DUK__TVAL_SET_TAGGEDPOINTER((v),(h),DUK_TAG_BUFFER) #define DUK_TVAL_SET_POINTER(v,p) DUK__TVAL_SET_TAGGEDPOINTER((v),(p),DUK_TAG_POINTER) -#define DUK_TVAL_SET_TVAL(v,x) do { *(v) = *(x); } while (0) +#define DUK_TVAL_SET_TVAL(v,x) do { \ + volatile int _i; \ + for (_i = 0; _i < 8; _i++) { \ + ((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \ + } \ +} while(0) /* getters */ #define DUK_TVAL_GET_BOOLEAN(v) ((int) (v)->us[DUK_DBL_IDX_US1]) @@ -31536,7 +31541,15 @@ new_e_pv != NULL && new_e_f != NULL); new_e_k[new_e_used] = key; +#if 0 new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i); +#endif + { + unsigned char *p_dst = (unsigned char *) &new_e_pv[new_e_used]; + unsigned char *p_src = (unsigned char *) DUK_HOBJECT_E_GET_VALUE_PTR(obj, i); + volatile int _i; + for (_i = 0; _i < 8; _i++) { p_dst[_i] = p_src[_i]; } + } new_e_f[new_e_used] = DUK_HOBJECT_E_GET_FLAGS(obj, i); new_e_used++; } - - 2013-12-10 10:00:44.425108 Z - sva - commented - "See: misc/clang_aliasing.c" - - 2013-12-10 18:19:25.100543 Z - sva - commented - |- The root cause is the fact the clang ends up using a floating point load and store to do the structured union assignment. A load+store sequence on X86 may convert a signaling NaN into a quiet NaN, which happens by setting the highest bit of the mantissa. See, for instance: http://caml.inria.fr/mantis/view.php?id=5038 This may be impossible to catch at compile time, so perhaps add a few self tests and/or add an assert to DUK_TVAL_SET_TVAL. There is no telling what other code might be affected though. - - 2013-12-10 21:35:38.936057 Z - sva - assigned to release v0.9 from v0.8 - "" - - 2014-01-12 14:03:11.829583 Z - sva - assigned to release v0.10 from v0.9 - "" - - 2014-02-11 22:37:38.045628 Z - sva - assigned to release v0.11 from v0.10 - "" - - 2014-02-18 13:22:36.557661 Z - sva - unassigned from release v0.11 - ""