-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[IRGen] Upstream pointer auth for value witness tables #74625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@swift-ci please test |
b16d6d5
to
162ff58
Compare
@swift-ci please test |
@swift-ci please test Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, but some small requests.
lib/IRGen/GenMeta.cpp
Outdated
|
||
const auto &ptrAuthOpts = IGM.getOptions().PointerAuth; | ||
if (ptrAuthOpts.FunctionPointers && ptrAuthOpts.FunctionPointers.getKind() == | ||
PointerAuthSchema::Kind::ARM8_3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should really just be if (auto schema = IGM.getOptions().PointerAuth.ValueWitnessTable) {
. We don't need to test function pointers or the kind of ptrauth we're doing in general; knowing that we have a request to sign vwtables is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this check is already performed in IRGen's setPointerAuthOptions
. Removed it here.
lib/IRGen/GenMeta.cpp
Outdated
auto *Clang = | ||
static_cast<ClangImporter *>(IGM.Context.getClangModuleLoader()); | ||
auto &targetInfo = Clang->getTargetInfo(); | ||
auto intSize = Size::forBits(targetInfo.getIntWidth()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the patch seems to be trying to set a more accurate bound on the dereferenceable range of the vwtable. That's fine, but it should be a separate patch (and probably extracted into a nice helper function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I removed this change from the patch.
162ff58
to
30cd90c
Compare
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last tweak, but otherwise LGTM
This makes Swift emit a signed pointer to the value witness table in type metadata. The original change was done by Varun Gandhi.
30cd90c
to
b22057e
Compare
@swift-ci please test |
This makes Swift emit a signed pointer to the value witness table in type metadata.
The original change was done by Varun Gandhi.