Skip to content

Conversation

ojhunt
Copy link
Contributor

@ojhunt ojhunt commented Sep 3, 2025

Clang as a number of Cleanup types used in exception handling, these are presumed to be POD types that can be memmoved as needed, however this is not correct by default on platforms with pointer authentication that make vtable pointers address discriminated.

This PR mitigates this problem by introducing a LLVM_MOVABLE_POLYMORPHIC_TYPE macro that can be used to annotate polymorphic types that are required to be movable, to override the use of address discrimination of the vtable pointer.

…types

Clang has a number of Cleanup types used in exception handling, these
are presumed to be POD types that can be memmoved as needed, however
this is not correct by default on platforms with pointer authentication
that make vtable pointers address discriminated.

This PR mitigates this problem by introducing a LLVM_MOVABLE_POLYMORPHIC_TYPE
macro that can be used to annotate polymorphic types that are required to
be movable, to override the use of address discrimination of the vtable pointer.
@ojhunt ojhunt changed the title [clang] Polymorphic Cleanup type is moved despite not being true POD … [clang] Polymorphic Cleanup type is moved despite not being POD types Sep 3, 2025
@ojhunt
Copy link
Contributor Author

ojhunt commented Sep 3, 2025

Not sure who should be looking at the Compiler.h change?

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure I get the motivation/what is happening here, but I don't know if there is a better person to review? Perhaps @efriedma-quic can give it a quick double-check as well?

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed other changes here recently (#152866). The code is playing some weird games with memory allocation, using an std::vector as an arena. Probably there's some better way to write this. Strictly speaking, it's undefined behavior to memmove a polymorphic class. But this is how it works for now. Maybe worth leaving a comment explaining this.

Otherwise LGTM

@ojhunt
Copy link
Contributor Author

ojhunt commented Sep 4, 2025

Not really sure I get the motivation/what is happening here, but I don't know if there is a better person to review? Perhaps @efriedma-quic can give it a quick double-check as well?

If you build clang targeting arm64e, pointer authentication means vtable pointers are not memcopyable by default.

The fix itself is trivial, I'm more concerned about people being ok with the location of the macro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants