access-softek / llvm-project

Other
0 stars 0 forks source link

Error if PAuth language features are used on unsupported CPU #66

Closed atrosinenko closed 8 months ago

atrosinenko commented 10 months ago

Check for several -fptrauth-* language features that require CPU with FEAT_PAuth support. If such unsupported feature is requested, make Clang frontend print error and exit early.

atrosinenko commented 10 months ago

Looks like HasPAuth and a few other flags should be conditionally set in AArch64TargetInfo::setArchFeatures function, but they don't: https://github.com/access-softek/llvm-project/commit/9e3a02b1661a17f8f6815eec5fc783fbd92ff00f Strictly speaking, these flags do not directly correspond to FEAT_* identifiers (example: HasCCPP), so this should be checked a bit more...

atrosinenko commented 10 months ago

As far as I can see, it is Clang driver that sets the per-triple default CPU and features, but clang -cc1 enables several pauth-related features based on arm64e-* triple. Thus, I have a number of options:

I plan implementing the second variant (add target-feature / target-cpu to existing tests). This seems safer than the first option (less probability of accidentally disabling pointer authentication).

asl commented 10 months ago

@atrosinenko I think everything should be done in driver. Tests are likely to be fixed.

atrosinenko commented 10 months ago

As far as I can see, it is Clang driver that sets the per-triple default CPU and features, but clang -cc1 enables several pauth-related features based on arm64e-* triple.

Sorry, this seems to be incorrect: both choosing the default CPU/features and -fptrauth-... options takes place in the driver, as expected. The only change in ~70 tests is to insert -target-feature +pauth options in invocations like this:

// RUN: %clang_cc1 -target arm64e-apple-ios -fptrauth-calls ... -emit-llvm ...

(i.e. those enabling ptrauth and not involving actual code generation either in assembler or object file form). Several tests still have to be fixed.

Maybe it is better to check for unsupported options in the driver instead. This should eliminate clang::PointerAuthOptions::getAllUsedSchemas() but may require more complicated checking for presence of +pauth feature. I will check this tomorrow.

atrosinenko commented 9 months ago

A few notes:

atrosinenko commented 9 months ago

llvm/llvm-project#78027 is the mainline PR that should replace 24048e11b427958315687a330e2ea359e04de9be commit in this PR.

atrosinenko commented 9 months ago

Addressed the comments, except for using a more generic triple in the test. A few weeks ago I saw a Discourse thread on "generic vs. precise triples" - obviously "the more triples tested the better", but I cannot remember whether that was an argument for using generic triples or for picking an arbitrary triple that is the most familiar for the particular developer... I guess that it is better to have as precise triple as possible (even though arbitrarily chosen at commit time) for better reproducibility. Links to coding standards and Discourse threads are welcome.

Note: after all review comments would be ultimately addressed, I would like to manually squash and rebase, so that the commit that sets HasPAuth flag remains a separate commit to be rolled back and then replaced by mainline one (see https://github.com/llvm/llvm-project/pull/78027).

atrosinenko commented 8 months ago

Once again, addressed everything except target triples :) Thanks for the link - I will update the test after a bit more research.

atrosinenko commented 8 months ago

Changed the cases that do not rely on features not being implicitly enabled to -target aarch64 (assuming no features are ever implicitly disabled due to target-specific defaults).

atrosinenko commented 8 months ago

Dropped the commit that sets HasPAuth is v8.3+ (replaced by #74), squashed and rebased.