Skip to content

[clang][PAC][darwin] Set correct default ptrauth features for arm64e-darwin #153722

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Aug 20, 2025
Merged

[clang][PAC][darwin] Set correct default ptrauth features for arm64e-darwin #153722

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0cbb084
[clang][PAC][darwin] Set correct default ptrauth features for arm64e-…
ojhunt Aug 15, 2025
4f7927f
addressng feedback
ojhunt Aug 17, 2025
e47f40f
moving test content around and adding a -arch specific set
ojhunt Aug 17, 2025
f464c22
Restore some tests that I removed in the various moving and editing test
ojhunt Aug 17, 2025
9f57f5d
sigh formatting
ojhunt Aug 17, 2025
c7e3353
Remove use of lambda from default argument logic
ojhunt Aug 18, 2025
1e7d828
Merge remote-tracking branch 'origin/main' into users/ojhunt/default-…
ojhunt Aug 19, 2025
6e869fa
remove the separate -arch test file
ojhunt Aug 19, 2025
0381cce
Merge remote-tracking branch 'origin/main' into users/ojhunt/default-…
ojhunt Aug 19, 2025
82b588f
simplify the tests
ojhunt Aug 19, 2025
4d80f7a
add a test for the weird ptrauth_objc_signable_class feature
ojhunt Aug 19, 2025
c98f3b0
Merge branch 'main' into users/ojhunt/default-arm64e-darwin-ptrauth-f…
ojhunt Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/Features.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ FEATURE(ptrauth_elf_got, LangOpts.PointerAuthELFGOT)

FEATURE(ptrauth_objc_isa, LangOpts.PointerAuthObjcIsa)
FEATURE(ptrauth_objc_interface_sel, LangOpts.PointerAuthObjcInterfaceSel)
FEATURE(ptrauth_objc_signable_class, true)
FEATURE(ptrauth_objc_signable_class, LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics)
FEATURE(ptrauth_objc_method_list_pointer, LangOpts.PointerAuthCalls)

EXTENSION(swiftcc,
Expand Down
33 changes: 33 additions & 0 deletions clang/lib/Driver/ToolChains/Darwin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3291,6 +3291,39 @@ void Darwin::addClangTargetOptions(
if (!RequiresSubdirectorySearch)
CC1Args.push_back("-fno-modulemap-allow-subdirectory-search");
}

if (getTriple().isArm64e()) {
Copy link
Member

Choose a reason for hiding this comment

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

There's already a block up in MachO::addClangTargetOptions. But it's been committed separately from our upstreaming effort, so is definitely outdated; can you update it instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this - the out of date-ness was not bad, it was just lacking the new features that this PR was adding - but I've moved it to there as well.

I was concerned about pushing it down to MachO before, but I've realized that there is unlikely to be another platform built on MachOs targeting arm64e specifically :D

Copy link
Member

Choose a reason for hiding this comment

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

Heh, I think it was moved from darwin to macho specifically for the myriad non-userland targets, which are usually obscure and not too interesting here

auto EnsureDefaultPtrauthFlag = [&](OptSpecifier Pos, OptSpecifier Neg) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why not the more idiomatic hasFlag/hasArg and push_back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does hasFlag(..) handle -ffoo -fno-foo correctly? I've assumed that it does not?

The reason for the weird push_back setup is because the verbose push_back("-ffoo") has always seemed error prone and difficult to maintain, and given these are a series of identically structured args it seemed doing the look up programmatically was better anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Does hasFlag(..) handle -ffoo -fno-foo correctly? I've assumed that it does not?

It does; per ArgList.h:

If both the option and its negation are present, the last one wins.

and if it didn't, given how idiomatic it is in the driver, we should fix it

The reason for the weird push_back setup is because the verbose push_back("-ffoo") has always seemed error prone and difficult to maintain, and given these are a series of identically structured args it seemed doing the look up programmatically was better anyway?

In general I don't disagree, but I'd argue that would belong as a general utility we can recognize, same as hasFlag. This being a special-purpose lambda just here makes it need a double-take to figure out that, yes, it does indeed behave the way you'd expect.

Note that hasFlag (or the lambda in the patch) vs. hasArg may subtly change the behavior here. The hasArg version tries to only set a target default when neither -f/-fno- flag is passed, letting the earlier generic driver logic handle the forwarding of driver flags to cc1 (and this part could be generalized to all flags that follow the pattern). With hasFlag I think you'd end up with the same flag twice in the cc1 command, which isn't wrong, just dumb. Even after reading it, I'm not quite sure what this patch does; I guess that's my point above ;)

In general, we should consider having the cc1 target machinery set these defaults, and only have the driver propagate overrides. Some of the weirder darwin targets would need some thought (mkernel and whatnot), but that's a problem for another day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Righto, I'll use string literals for now (and will move it to the macho place)

Copy link
Member

Choose a reason for hiding this comment

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

The same applies to the lambda, what it's for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making the code nicer? but will change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it repeat the if (!hasArg)pattern that we really need to remove :-/

assert(Pos != Neg);
if (const Arg *Opt = DriverArgs.getLastArg(Pos, Neg);
Opt && Opt->getOption().matches(Neg))
return;
Option PosOpt = getDriverOptTable().getOption(Pos);
CC1Args.push_back(PosOpt.getPrefixedName().data());
};
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_calls,
options::OPT_fno_ptrauth_calls);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_returns,
options::OPT_fno_ptrauth_returns);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_intrinsics,
options::OPT_fno_ptrauth_intrinsics);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_indirect_gotos,
options::OPT_fno_ptrauth_indirect_gotos);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_auth_traps,
options::OPT_fno_ptrauth_auth_traps);
EnsureDefaultPtrauthFlag(
options::OPT_fptrauth_vtable_pointer_address_discrimination,
options::OPT_fno_ptrauth_vtable_pointer_address_discrimination);
EnsureDefaultPtrauthFlag(
options::OPT_fptrauth_vtable_pointer_type_discrimination,
options::OPT_fno_ptrauth_vtable_pointer_type_discrimination);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_objc_isa,
options::OPT_fno_ptrauth_objc_isa);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_objc_class_ro,
options::OPT_fno_ptrauth_objc_class_ro);
EnsureDefaultPtrauthFlag(options::OPT_fptrauth_objc_interface_sel,
options::OPT_fno_ptrauth_objc_interface_sel);
}
}

void Darwin::addClangCC1ASTargetOptions(
Expand Down
85 changes: 85 additions & 0 deletions clang/test/Driver/ptrauth-platform-defaults.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// RUN: %clang -target arm64 -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64-apple-macosx -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64-darwin -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64-apple-darwin -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64-apple-ios-macabi -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64-linux-gnu -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64e-linux-gnu -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we really need to test 75% of these, should we? I wouldn't mind if they were free, but they're not ;) Some are really triple-parsing tests that ought to be covered elsewhere
But I'd especially remove arm64e-linux-gnu, because it normalizes something that we should straight up reject.

Copy link
Contributor Author

@ojhunt ojhunt Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  • will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly had these here as a hint for linux folk to add their defaults into here as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave it to them to add this if needed, but in general I don't think any other target really has as much emphasis on platform defaults as we can afford for darwin; in this case I believe it's all gated by the pauthabi machinery and whatnot, which is thoroughly tested elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'd expect -triple arm64e to boil down to something like arm64e-unknown-linux-elf, via arm64e-unknown-unknown, so it's still doing linux-elf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, but -arch does imply darwin so there's a new test for that (for the platform restriction in the test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmedbougacha I was not arguing, and I did remove the linux triples - I was just saying why I thought they would be useful originally :D

// RUN: %clang -target aarch64-linux-gnu -DNO_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=no_default_ptrauth
// RUN: %clang -target arm64e-apple-macosx -DDARWIN_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=darwin_ptrauth_defaults
// RUN: %clang -target arm64e-apple-ios -DDARWIN_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=darwin_ptrauth_defaults
// RUN: %clang -target arm64e-darwin -DDARWIN_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=darwin_ptrauth_defaults
// RUN: %clang -target arm64e-apple-darwin -DDARWIN_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=darwin_ptrauth_defaults
// RUN: %clang -target arm64e-apple-ios-macabi -DDARWIN_DEFAULT_PTRAUTH %s -fsyntax-only -Xclang -verify=darwin_ptrauth_defaults

// A simple test case to test basic override logic
// RUN: %clang -target arm64e-apple-macosx -DDARWIN_DEFAULT_PTRAUTH_OVERRIDE -fno-ptrauth-calls %s -fsyntax-only -Xclang -verify=darwin_ptrauth_override

#define ASSERT_MODE_AND_KIND(feature, enabled, kind) \
_Static_assert(enabled == __has_##kind(feature), \
Copy link
Member

Choose a reason for hiding this comment

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

ISTM this conflates testing the features and the driver defaults. We already do both in

clang/test/Driver/arch-arm64e.c
clang/test/Preprocessor/ptrauth_feature.c

They both could always benefit from some cleaning up, but I'd rather have those updated, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I wasn't sure how to robustly test the argument list when I'm trying to test for multiple flags? I could test based on hope (this is a reference literally no one will recognize) with something like

// CHECK: flag1{{.*}}flag2{{.*}}flag3 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also rather keep both the arg and feature flags because I've encountered problems previously where one place screwed up propagation of flags -- I think it did something like if (OPT_foo) push_back("-fbar") or LangOpts.Bar = hasArg(OPT_foo), testing the features captures the full clang->clang_cc->feature path as a backup paranoia test, and doesn't have any real cost.

Copy link
Member

Choose a reason for hiding this comment

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

So I wasn't sure how to robustly test the argument list when I'm trying to test for multiple flags? I could test based on hope (this is a reference literally no one will recognize) with something like

// CHECK: flag1{{.*}}flag2{{.*}}flag3 ...

Yeah, I guess a bunch of driver test do something like that, or CHECK-SAME, or splitting the output into lines, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmedbougacha the feature check based test was literally because I couldn't work out how to deal with the variable ordering of parameters, but having moved this down to the macho target arg generation all of the flags end up in a single sequence.

I am going to keep a feature check version of this just as a text for the default driver flags correctly propagating to the cc1 layer.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, isn't the propagating to cc1 tested with the cc1 command check lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then we make sure that is propagated to the actual compilation state because I'm paranoid :D

"Expected to have the " #feature " " #kind " enabled");

#define ASSERT_FEATURE_ENABLED(feature_name) \
ASSERT_MODE_AND_KIND(feature_name, 1, feature)
#define ASSERT_FEATURE_DISABLED(feature_name) \
ASSERT_MODE_AND_KIND(feature_name, 0, feature)
#define ASSERT_EXTENSION_ENABLED(extension_name) \
ASSERT_MODE_AND_KIND(extension_name, 1, extension)
#define ASSERT_EXTENSION_DISABLED(extension_name) \
ASSERT_MODE_AND_KIND(extension_name, 0, extension)

#if defined(DARWIN_DEFAULT_PTRAUTH) || defined(DARWIN_DEFAULT_PTRAUTH_OVERRIDE)
ASSERT_FEATURE_ENABLED(ptrauth_intrinsics)
ASSERT_EXTENSION_ENABLED(ptrauth_qualifier)

#if defined(DARWIN_DEFAULT_PTRAUTH_OVERRIDE)
ASSERT_FEATURE_DISABLED(ptrauth_calls)
// These flags directly reflect the state of ptrauth_calls, but exist
// for backward compatibility reasons
ASSERT_FEATURE_DISABLED(ptrauth_member_function_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_objc_method_list_pointer)
#else
ASSERT_FEATURE_ENABLED(ptrauth_calls)
ASSERT_FEATURE_ENABLED(ptrauth_member_function_pointer_type_discrimination)
ASSERT_FEATURE_ENABLED(ptrauth_objc_method_list_pointer)
#endif

ASSERT_FEATURE_ENABLED(ptrauth_returns)
ASSERT_FEATURE_ENABLED(ptrauth_vtable_pointer_address_discrimination)
ASSERT_FEATURE_ENABLED(ptrauth_vtable_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_type_info_vtable_pointer_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_signed_block_descriptors)
ASSERT_FEATURE_DISABLED(ptrauth_function_pointer_type_discrimination)
ASSERT_FEATURE_ENABLED(ptrauth_indirect_gotos)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini_address_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_elf_got)
ASSERT_FEATURE_ENABLED(ptrauth_objc_isa)
ASSERT_FEATURE_ENABLED(ptrauth_objc_interface_sel)
ASSERT_FEATURE_ENABLED(ptrauth_objc_signable_class)
#endif

#ifdef NO_DEFAULT_PTRAUTH
ASSERT_FEATURE_DISABLED(ptrauth_intrinsics)
ASSERT_EXTENSION_DISABLED(ptrauth_qualifier)
ASSERT_FEATURE_DISABLED(ptrauth_calls)
ASSERT_FEATURE_DISABLED(ptrauth_returns)
ASSERT_FEATURE_DISABLED(ptrauth_vtable_pointer_address_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_vtable_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_type_info_vtable_pointer_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_member_function_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_signed_block_descriptors)
ASSERT_FEATURE_DISABLED(ptrauth_function_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_indirect_gotos)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini_address_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_elf_got)
ASSERT_FEATURE_DISABLED(ptrauth_objc_isa)
ASSERT_FEATURE_DISABLED(ptrauth_objc_interface_sel)
ASSERT_FEATURE_DISABLED(ptrauth_objc_signable_class)
ASSERT_FEATURE_DISABLED(ptrauth_objc_method_list_pointer)
#endif

// darwin_ptrauth_defaults-no-diagnostics
// no_default_ptrauth-no-diagnostics
// darwin_ptrauth_override-no-diagnostics
45 changes: 45 additions & 0 deletions clang/test/Sema/ptrauth-platform-defaults.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// RUN: %clang_cc1 -triple arm64 %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64-apple-macosx %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64-darwin %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64-apple-darwin %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64-apple-ios-macabi %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64-linux-gnu %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-linux-gnu %s -fsyntax-only
// RUN: %clang_cc1 -triple aarch64-linux-gnu %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-apple-macosx %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-apple-ios %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-darwin %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-apple-darwin %s -fsyntax-only
// RUN: %clang_cc1 -triple arm64e-apple-ios-macabi %s -fsyntax-only

// The -cc1 mode should not insert default ptrauth flags

#define ASSERT_MODE_AND_KIND(feature, enabled, kind) \
_Static_assert(enabled == __has_##kind(feature), \
"Expected to have the " #feature " " #kind " enabled");

#define ASSERT_FEATURE_DISABLED(feature_name) \
ASSERT_MODE_AND_KIND(feature_name, 0, feature)
#define ASSERT_EXTENSION_DISABLED(extension_name) \
ASSERT_MODE_AND_KIND(extension_name, 0, extension)

ASSERT_FEATURE_DISABLED(ptrauth_intrinsics)
ASSERT_EXTENSION_DISABLED(ptrauth_qualifier)
ASSERT_FEATURE_DISABLED(ptrauth_calls)
ASSERT_FEATURE_DISABLED(ptrauth_returns)
ASSERT_FEATURE_DISABLED(ptrauth_vtable_pointer_address_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_vtable_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_type_info_vtable_pointer_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_member_function_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_signed_block_descriptors)
ASSERT_FEATURE_DISABLED(ptrauth_function_pointer_type_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_indirect_gotos)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini)
ASSERT_FEATURE_DISABLED(ptrauth_init_fini_address_discrimination)
ASSERT_FEATURE_DISABLED(ptrauth_elf_got)
ASSERT_FEATURE_DISABLED(ptrauth_objc_isa)
ASSERT_FEATURE_DISABLED(ptrauth_objc_interface_sel)
ASSERT_FEATURE_DISABLED(ptrauth_objc_signable_class)
ASSERT_FEATURE_DISABLED(ptrauth_objc_method_list_pointer)

// expected-no-diagnostics