fix reinterpret(Char, ::UInt32) for "unnatural" values (fix #29181) #29192

StefanKarpinski · 2018-09-14T19:26:46Z

No description provided.

JeffBezanson · 2018-09-14T22:00:29Z

src/datatype.c

@@ -689,8 +689,9 @@ static jl_value_t *boxed_char_cache[128];
 JL_DLLEXPORT jl_value_t *jl_box_char(uint32_t x)
 {
    jl_ptls_t ptls = jl_get_ptls_states();
-    if (0 < (int32_t)x)
-        return boxed_char_cache[x >> 24];
+    uint32_t u = __builtin_bswap32(x);


We have bswap_32 defined for this for a few platforms.

"for a few platforms" sounds unlikely to work everywhere. @yuyichao, @Keno, @vtjnash, do any of you have advice on how to spell this operation correctly and portably?

bswap_32 is the correct version since it takes care of the difference between clang, ICC, MSVC, or not having the builtin available at all.

see

julia/src/support/utils.h

Lines 32 to 57 in 7507511

#if defined(__clang__) || (defined(__GNUC__) && (__GNUC__ > 4 || __GNUC_MINOR__ >= 8))

#define bswap_16(x) __builtin_bswap16(x)

#define bswap_32(x) __builtin_bswap32(x)

#define bswap_64(x) __builtin_bswap64(x)

#elif defined(_MSC_VER)

#define bswap_16(x) _byteswap_ushort(x)

#define bswap_32(x) _byteswap_ulong(x)

#define bswap_64(x) _byteswap_uint64(x)

#elif defined(__INTEL_COMPILER)

#define bswap_16(x) _bswap16(x)

#define bswap_32(x) _bswap(x)

#define bswap_64(x) _bswap64(x)

#else

#define bswap_16(x) (((x) & 0x00ff) << 8 | ((x) & 0xff00) >> 8)

#define bswap_32(x) \

((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >> 8) | \

(((x) & 0x0000ff00) << 8) | (((x) & 0x000000ff) << 24))

STATIC_INLINE uint64_t ByteSwap64(uint64_t x)

{

uint32_t high = (uint32_t) (x >> 32);

uint32_t low = (uint32_t) x;

return ((uint64_t) bswap_32 (high)) |

(((uint64_t) bswap_32 (low)) << 32);

}

#define bswap_64(x) ByteSwap64(x)

#endif

This code was assuming that character values only have bit-patterns that decoding a string can produce, but of course `reinterpret` can produce any bit pattern in a `Char` whatsoever. The fix doesn't use that assumption and only uses the cache for actual ASCII characters.

StefanKarpinski · 2018-09-17T12:52:45Z

We're seeing a lot of httpbin flakiness again. Time to switch, @staticfloat?

staticfloat · 2018-09-17T13:50:10Z

@StefanKarpinski your wish is my command: #29228

…29192) This code was assuming that character values only have bit-patterns that decoding a string can produce, but of course `reinterpret` can produce any bit pattern in a `Char` whatsoever. The fix doesn't use that assumption and only uses the cache for actual ASCII characters. (cherry picked from commit 88f74b7)

StefanKarpinski force-pushed the sk/char-reinterpret branch from 87083bb to 3759d23 Compare September 14, 2018 19:32

JeffBezanson reviewed Sep 14, 2018

View reviewed changes

JeffBezanson added bugfix This change fixes an existing bug strings "Strings!" labels Sep 14, 2018

StefanKarpinski force-pushed the sk/char-reinterpret branch from 3759d23 to 0a18ca9 Compare September 16, 2018 18:50

StefanKarpinski force-pushed the sk/char-reinterpret branch from 0a18ca9 to 7d3fdcd Compare September 17, 2018 06:39

StefanKarpinski merged commit 88f74b7 into master Sep 17, 2018

StefanKarpinski deleted the sk/char-reinterpret branch September 17, 2018 20:28

KristofferC added the backport pending 1.0 label Sep 17, 2018

KristofferC mentioned this pull request Sep 30, 2018

WIP: Backports for 1.0.2 #29444

Merged

KristofferC removed the backport pending 1.0 label Nov 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix reinterpret(Char, ::UInt32) for "unnatural" values (fix #29181) #29192

fix reinterpret(Char, ::UInt32) for "unnatural" values (fix #29181) #29192

StefanKarpinski commented Sep 14, 2018

JeffBezanson Sep 14, 2018

StefanKarpinski Sep 15, 2018

vchuravy Sep 16, 2018

StefanKarpinski commented Sep 17, 2018

staticfloat commented Sep 17, 2018

	#if defined(__clang__) \|\| (defined(__GNUC__) && (__GNUC__ > 4 \|\| __GNUC_MINOR__ >= 8))
	#define bswap_16(x) __builtin_bswap16(x)
	#define bswap_32(x) __builtin_bswap32(x)
	#define bswap_64(x) __builtin_bswap64(x)
	#elif defined(_MSC_VER)
	#define bswap_16(x) _byteswap_ushort(x)
	#define bswap_32(x) _byteswap_ulong(x)
	#define bswap_64(x) _byteswap_uint64(x)
	#elif defined(__INTEL_COMPILER)
	#define bswap_16(x) _bswap16(x)
	#define bswap_32(x) _bswap(x)
	#define bswap_64(x) _bswap64(x)
	#else
	#define bswap_16(x) (((x) & 0x00ff) << 8 \| ((x) & 0xff00) >> 8)
	#define bswap_32(x) \
	((((x) & 0xff000000) >> 24) \| (((x) & 0x00ff0000) >> 8) \| \
	(((x) & 0x0000ff00) << 8) \| (((x) & 0x000000ff) << 24))
	STATIC_INLINE uint64_t ByteSwap64(uint64_t x)
	{
	uint32_t high = (uint32_t) (x >> 32);
	uint32_t low = (uint32_t) x;
	return ((uint64_t) bswap_32 (high)) \|
	(((uint64_t) bswap_32 (low)) << 32);
	}
	#define bswap_64(x) ByteSwap64(x)
	#endif

fix reinterpret(Char, ::UInt32) for "unnatural" values (fix #29181) #29192

fix reinterpret(Char, ::UInt32) for "unnatural" values (fix #29181) #29192

Conversation

StefanKarpinski commented Sep 14, 2018

JeffBezanson Sep 14, 2018

Choose a reason for hiding this comment

StefanKarpinski Sep 15, 2018

Choose a reason for hiding this comment

vchuravy Sep 16, 2018

Choose a reason for hiding this comment

StefanKarpinski commented Sep 17, 2018

staticfloat commented Sep 17, 2018