memcpy object with no trivial copy-assignment

[tnicolson]

Hi,

I'm encountering the following error with gcc 8.1:

cxx_function.hpp:626:24: warning: 'void memcpy(void, const void*, size_t)' writing to an object of type 'struct cxx_function::impl::erasure_base' with no trivial copy-assignment; use copy-initialization instead [-Wclass-memaccess]
std::memcpy( & this->erasure(), & s.erasure(), sizeof (storage) );

Is this spurious?

[potswa]

There's some value in that warning, but it's a little off. The problem is that memcpy is not doing copy-assignment there, but it's actually replacing an object of derived type with another of different derived type.

If the library isn't careful, the compiler will assume that the type of the object doesn't change, and it may generate a non-virtual call to the wrong target operator().

Since this library was last revised, std::launder was standardized and hopefully it has been used to fix the corresponding issues in the vendors' standard libraries. So I need to dig into those and apply a similar fix here… but also not breaking backward compatibility as it already tiptoes around older compilers.

[graphicsMan]

I encountered this as well, and in this case, the fix is trivial: The memory backing the erasure() accessor is just something like std::aligned_storage<...> storage; In this case, just modify the memcpy to:

storage = s.storage;

I have tried creating a push for this, but I'm getting permission denied.

[potswa]

@graphicsMan You would need to create a merge request after clicking the Fork button at top-right of GitHub and pushing to your own repo. This repo isn't open for contributors to push directly.

The issue here is actually bit different from my first comment. Thanks for bringing my attention back to this.

The problem is that erasure_base has a reference member, and references cannot be reassigned, so when memcpy is applied the compiler is free to ignore the new referent (the "vtable" of the new derived type).

The reason for using memcpy instead of assignment on the data blob is that the compiler likewise may overlook that the erasure_base value is changing, when it only observes that an aligned_storage value at that address is changing. It depends on the compiler's alias analysis but I have already seen problems like this.

Given that I'm already working around some alias problems by memcpy instead of assignment, we should improve the workaround by changing the reference to a pointer. I'm open to a merge request like this… Otherwise I'll try to push an update sometime soon, but I'm pretty busy these days.

[graphicsMan]

I think also just changing the arguments to be reinterpret_cast to chat* should fix the warning.

Thanks!

On Fri, Oct 26, 2018, 15:52 David Krauss notifications@github.com wrote:

@graphicsMan https://github.com/graphicsMan You would need to create a merge request after clicking the Fork button at top-right of GitHub and pushing to your own repo. This repo isn't open for contributors to push directly.

However, the issue here is actually bit different. Thanks for bringing my attention back to this.

The problem is that erasure_base has a reference member, and references cannot be reassigned, so when memcpy is applied the compiler is free to ignore the new referent (the "vtable" of the new derived type).

The reason for using memcpy instead of assignment on the data blob is that the compiler likewise may overlook that the erasure_base value is changing, when it only observes that an aligned_storage value at that address is changing. It depends on the compiler's alias analysis but I have already seen problems like this.

Given that I'm already working around some alias problems by memcpy instead of assignment, we should improve the workaround by changing the reference to a pointer. I'm open to a merge request like this… Otherwise I'll try to push an update sometime soon, but I'm pretty busy these days.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/potswa/cxx_function/issues/12#issuecomment-433563801, or mute the thread https://github.com/notifications/unsubscribe-auth/AlEcfeGr7OjQojUzoe0p0obBeFZ1v-KMks5uo5I0gaJpZM4UCjlt .

[graphicsMan]

Autocorrect... That should be char*.

On Fri, Oct 26, 2018, 17:57 Brian Budge brian.budge@gmail.com wrote:

I think also just changing the arguments to be reinterpret_cast to chat* should fix the warning.

Thanks!

On Fri, Oct 26, 2018, 15:52 David Krauss notifications@github.com wrote:

@graphicsMan https://github.com/graphicsMan You would need to create a merge request after clicking the Fork button at top-right of GitHub and pushing to your own repo. This repo isn't open for contributors to push directly.

However, the issue here is actually bit different. Thanks for bringing my attention back to this.

The problem is that erasure_base has a reference member, and references cannot be reassigned, so when memcpy is applied the compiler is free to ignore the new referent (the "vtable" of the new derived type).

The reason for using memcpy instead of assignment on the data blob is that the compiler likewise may overlook that the erasure_base value is changing, when it only observes that an aligned_storage value at that address is changing. It depends on the compiler's alias analysis but I have already seen problems like this.

Given that I'm already working around some alias problems by memcpy instead of assignment, we should improve the workaround by changing the reference to a pointer. I'm open to a merge request like this… Otherwise I'll try to push an update sometime soon, but I'm pretty busy these days.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/potswa/cxx_function/issues/12#issuecomment-433563801, or mute the thread https://github.com/notifications/unsubscribe-auth/AlEcfeGr7OjQojUzoe0p0obBeFZ1v-KMks5uo5I0gaJpZM4UCjlt .

[graphicsMan]

Bah. I just re-read your email. I admit, I only looked shallowly at this previously. I will try to have another look at the code for erasure_base, and if I get some time this weekend, I can try your proposed change.

On Fri, Oct 26, 2018, 18:07 Brian Budge brian.budge@gmail.com wrote:

Autocorrect... That should be char*.

On Fri, Oct 26, 2018, 17:57 Brian Budge brian.budge@gmail.com wrote:

I think also just changing the arguments to be reinterpret_cast to chat* should fix the warning.

Thanks!

On Fri, Oct 26, 2018, 15:52 David Krauss notifications@github.com wrote:

@graphicsMan https://github.com/graphicsMan You would need to create a merge request after clicking the Fork button at top-right of GitHub and pushing to your own repo. This repo isn't open for contributors to push directly.

However, the issue here is actually bit different. Thanks for bringing my attention back to this.

The problem is that erasure_base has a reference member, and references cannot be reassigned, so when memcpy is applied the compiler is free to ignore the new referent (the "vtable" of the new derived type).

The reason for using memcpy instead of assignment on the data blob is that the compiler likewise may overlook that the erasure_base value is changing, when it only observes that an aligned_storage value at that address is changing. It depends on the compiler's alias analysis but I have already seen problems like this.

Given that I'm already working around some alias problems by memcpy instead of assignment, we should improve the workaround by changing the reference to a pointer. I'm open to a merge request like this… Otherwise I'll try to push an update sometime soon, but I'm pretty busy these days.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/potswa/cxx_function/issues/12#issuecomment-433563801, or mute the thread https://github.com/notifications/unsubscribe-auth/AlEcfeGr7OjQojUzoe0p0obBeFZ1v-KMks5uo5I0gaJpZM4UCjlt .

[potswa]

Fixed by 987f7dc16.