Re: [boost] [beast] Formal review

Subject: Re: [boost] [beast] Formal review
From: Groke, Paul (paul.groke_at_[hidden])
Date: 2017-07-12 08:19:26

> From: Gavin Lambert via Boost
> Sent: Mittwoch, 12. Juli 2017 01:33
> 
> On 11/07/2017 21:30, Andrey Semashev wrote:
>> You can cast both ways. The casted-from-char pointer can be used as
>> long as the underlying object, in the C++ object model, matches the
>> pointed type. In other words, this is fine:
>>
>>    std::size_t* p1 = new std::size_t[10];
>>    char* p2 = reinterpret_cast< char* >(p1);
>>    std::size_t* p3 = reinterpret_cast< std::size_t* >(p2);
>>    assert(p1 == p3);
>>    p3[0] = 10; // ok
>>
>> In Beast's case we (and compiler) cannot tell whether the pointers
>> refer to std::size_t objects. For compiler that means it has to assume
>> the objects exist and thus prevent any optimizations that would contradict
>> it.
> 
(snip)
> 
> (It's also important to note that the standard only permits casting to "char*" -
> - not "unsigned char*" or "uint8_t*".)

Sure that unsigned char isn't OK? I thought, out of the char family, only signed char wasn't. (And uint8_t might theoretically be a problem, because an implementation doesn't necessarily have to implement uint8_t as a typedef to unsigned char.)

> You can still run afoul of strict aliasing if you pass both pointers to other
> methods, however (but passing only one should be safe).  Hopefully the
> compiler is smart enough to notice that they're aliased as long as you stay
> within the same method.

I didn't follow the discussion close enough so I might be mistaken, but isn't this a function that takes a char* as input (from user code)?
If that is so, using size_ts to read the chars could be an aliasing violation. Namely if the user thinks that he can pass e.g. a pointer to an int array, because the function takes a char pointer, and char may alias...
Then he'd have one part of the application access a memory location with size_t, which was written by another part of the application using int.

A real example that causes a real bug here would probably be very hard to construct. But I don't think this should be ignored.

Also I wouldn't feel very good about using memcpy for this and similar optimizations - seen it come out slow (=real call to memcpy) too often in the past. Since this is a common problem with a common optimization, maybe it's time to introduce aliasing_load and aliasing_store helper functions (e.g. in Boost.Utility)? That way any improvements (speed, reliability) would benefit all users and not just one library. Or, a different idea: wouldn't a compiler fence like std::atomic_signal_fence before the aliasing read be enough? I'm almost sure that this doesn't make it OK with the standard, but I can't imagine what kind of optimization a compiler might possibly do to break things if there are fences before all aliasing loads/after all aliasing stores. (Granted: this is a bad argument. Maybe the standard should provide std::aliasing_fence xD).

Regards,
Paul