[clang][PAC] add support for options parameter to __ptrauth #136828
Conversation
ojhunt
requested review from
erichkeane,
ahatanak,
cor3ntin and
AaronBallman
and removed request for
erichkeane
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,c,h -- clang/test/CodeGen/ptrauth-stripping.c clang/test/Sema/ptrauth-qualifier-options.c clang/test/SemaCXX/ptrauth-qualifier-constexpr-options.cpp clang/include/clang/Basic/LangOptions.h clang/lib/AST/TypePrinter.cpp clang/lib/CodeGen/CGExprConstant.cpp clang/lib/Parse/ParseDecl.cpp clang/lib/Sema/SemaType.cpp clang/test/Parser/ptrauth-qualifier.c clang/test/Sema/ptrauth-qualifier.cView the diff from clang-format here.diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index d1fdedc76..4f7c9f167 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8399,7 +8399,8 @@ static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
};
auto DiagnoseInvalidOptionsParameter = [&](llvm::StringRef Reason) {
S.Diag(AuthenticationOptionsRange.getBegin(),
- diag::err_ptrauth_invalid_option) << Reason;
+ diag::err_ptrauth_invalid_option)
+ << Reason;
Attr.setInvalid();
IsInvalid = true;
ReportEvaluationOfExpressionIfNeeded();
@@ -8494,7 +8495,7 @@ static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
StringRef LeadingOption = Option.slice(0, WhitespaceIndex);
S.Diag(AuthenticationOptionsRange.getBegin(),
diag::err_ptrauth_option_missing_comma)
- << LeadingOption;
+ << LeadingOption;
} else {
S.Diag(AuthenticationOptionsRange.getBegin(),
diag::err_ptrauth_unknown_authentication_option)
|
|
The tests in this PR are dependent on #136828, and the included tests have non-sequential numbers as the tests include other options that are not supported by this PR, to try and limit the complexity of this PR to just the parsing and existing functionality. |
clang/lib/Sema/SemaType.cpp
Outdated
| return; | ||
| } | ||
| OptionsString = OptionsStringLiteral->getString(); | ||
| } else if (S.EvaluateAsString(AuthenticationOptionsArg, OptionsString, S.Context, Sema::StringEvaluationContext::PtrauthOptions, /*ErrorOnInvalidMessage=*/false)) { |
There was a problem hiding this comment.
The implementation I was upstreaming had a very very restricted version of @cor3ntin's constant evaluated string, so I have adopted that.
clang/lib/Sema/SemaType.cpp
Outdated
| bool HasSeenOption = false; | ||
| unsigned CurrentIdx = 0; | ||
|
|
||
| auto OptionStringIdxLocation = [&](unsigned Idx) { |
There was a problem hiding this comment.
This code is moderately concise parser of the grammar:
option :- [a-z-]+
options :- option (',' option)*
The reason for the complexity - vs. just a split(',').map(trim) - is just error messaging. We could simplify if people think it's warranted, but the error message tests are already reduced from what it could be.
There was a problem hiding this comment.
We could, if there is an error in the nth item, recompute the location by finding the nth comma and point to that. I feel like more granularity than that adds more complexity than it is worth.
clang/lib/Sema/SemaType.cpp
Outdated
| return std::make_pair(OptionStartIdx, OptionEndIdx); | ||
| }; | ||
|
|
||
| auto OptionHandler = [&](StringRef TokenStr, SourceRange TokenRange, |
There was a problem hiding this comment.
This is more generic than is strictly needed at this point, because future PRs add additional option types and flags
There was a problem hiding this comment.
I would much prefer a 3 if/else :)
clang/lib/Sema/SemaType.cpp
Outdated
| "__ptrauth qualifier takes between 1 and 3 arguments"); | ||
| assert((Attr.getNumArgs() > 0 && Attr.getNumArgs() <= 4) && | ||
| "__ptrauth qualifier takes between 1 and 4 arguments"); | ||
| StringRef AttrName = Attr.getAttrName()->getName(); |
There was a problem hiding this comment.
AttrName is used because the diagnostics need the actual attribute name, and there's a future PR to add the __ptrauth_restricted_intptr qualifier which means it is not a constant. We can remove AttrName now, but it is immediately needed in the next PR.
There was a problem hiding this comment.
following the __ptrauth for ints PR __ptrauth_restricted_intptr is gone, I thought I had resolved all of these
|
This PR is a draft as it will need to be updated to merged correctly, and verify test output (conflicts, unintentional mismatches coming from splitting out the PR) after #136783 |
clang/lib/Sema/SemaType.cpp
Outdated
| for (char Ch : OptionsString) { | ||
| if (!Ch || !isascii(Ch)) { | ||
| DiagnoseInvalidOptionsParameter( | ||
| "contains invalid characters", Ch, | ||
| AuthenticationOptionsArg->getSourceRange()); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we really need that? we can just say "\0签名" is not a valid option
There was a problem hiding this comment.
Can you add tests with a string-view-like type? I would not be surprised if we need more changes in the attribute parsing code to be able to have string_view as StringArgument argument
There was a problem hiding this comment.
@cor3ntin re: string_view: I'm trying not to add anything beyond the bare minimum here, I thought the existing behavior of EvaluateString is a superset of what we know existing code requires so I didn't want to trying to add more complexity to a pile of already complex PRs, that said it looks like I haven't pulled those specific tests across so I could be wrong.
Re: non-ascii: I'm not sure I understand the semantics of StringLiteral::toString and EvaluateString(...) if presented w.r.t utf8 or wide chars so I wanted consistent behavior between them, and the easiest way to ensure that was an early out on non-ascii. StringLiteral::getString() specifically has an assertion specifically about expecting char sized characters which was worrying to me. If that assertion is actually trying to say sizeof(string code point)==sizeof(char) then that's much simpler.
clang/lib/Sema/SemaType.cpp
Outdated
| bool HasSeenOption = false; | ||
| unsigned CurrentIdx = 0; | ||
|
|
||
| auto OptionStringIdxLocation = [&](unsigned Idx) { |
There was a problem hiding this comment.
We could, if there is an error in the nth item, recompute the location by finding the nth comma and point to that. I feel like more granularity than that adds more complexity than it is worth.
clang/lib/Sema/SemaType.cpp
Outdated
| if (OptionsStringLiteral->containsNonAsciiOrNull()) { | ||
| DiagnoseInvalidOptionsParameter( | ||
| "contains invalid characters", std::nullopt, | ||
| AuthenticationOptionsArg->getSourceRange()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same here as below, if we accept exactly 3 strings, we don;t need to check anything else
There was a problem hiding this comment.
There are more than just those three strings, but adding the additional ones implies the ability to test them, which means pulling in the implementation of those options.
I went for this specific subset because PointerAuthQualifier already has the required functionality, and so this PR could be very close to just the parsing.
clang/lib/Sema/SemaType.cpp
Outdated
| if (AuthenticationOptionsArg->isValueDependent() || | ||
| AuthenticationOptionsArg->isTypeDependent()) { | ||
| DiagnoseInvalidOptionsParameter( | ||
| "is dependent", std::nullopt, | ||
| AuthenticationOptionsArg->getSourceRange()); | ||
| return; |
There was a problem hiding this comment.
I wonder if that can just be delayed to later? Maybe a separate patch though
There was a problem hiding this comment.
I added this somewhat ad hoc, the behavior I was upstreaming basically just did if (StringLiteral) { ... } else if (const eval produces a char* + length) { ... }, which just happens to exclude dependent values, but EvaluateString has an assertion that the values are not dependent. That might be something we want to fix in future, but it seemed that the sensible thing at the moment was just adding a dependent value/type check as we have no support for dependent ptrauth qualifiers :-/
There was a problem hiding this comment.
@cor3ntin oh, when you say delayed until later do you mean template instantiation? That would be nice but currently the ptrauth qualifier does not support dependent parameters at all - it's something I've wanted to add support for (because it would be very useful) but is never high enough on the priority list, and seems to require an inordinate amount of seemingly mechanical work to achieve :-/
clang/lib/Sema/SemaType.cpp
Outdated
| return std::make_pair(OptionStartIdx, OptionEndIdx); | ||
| }; | ||
|
|
||
| auto OptionHandler = [&](StringRef TokenStr, SourceRange TokenRange, |
There was a problem hiding this comment.
I would much prefer a 3 if/else :)
|
@cor3ntin the option selection down stream is a StringSwitch on the authentication mode, and then a sequence of (not large, I think just two) Previous PRs have suggested maps, etc for such conditions, and a map of option=>handler seemed consistent with that in this case, but I'm really on the fence about which. Absent the map approach (just typing directly into the comment w/o checking code) I think the alternative is that the option handler looks like: switchswitch(option)
.Case("strip", ...)
.Case("sign-and-strip", ...)
.Case("sign-and-auth", ...);
// 'else' here I think is handled by a flag
else if (option == thing1) { .... }
else if (option == thing2) { ... }Any thoughts on which you consider preferable? I think either works as the gross portion of this code is already just the tokenisation. |
ojhunt
force-pushed
the
users/ojhunt/ptrauth-options
branch
4 times, most recently
from
d32a1bd to
d51ed1b
Compare
ojhunt
changed the title
ojhunt
force-pushed
the
users/ojhunt/ptrauth-options
branch
4 times, most recently
from
d28aa40 to
6f161d3
Compare
| @@ -65,6 +65,17 @@ enum class PointerAuthenticationMode : unsigned { | |||
| SignAndAuth | |||
| }; | |||
|
|
|||
| static constexpr llvm::StringLiteral PointerAuthenticationOptionStrip = "strip"; | |||
There was a problem hiding this comment.
@cor3ntin @AaronBallman is there a better/more idiomatic way of doing these?
There was a problem hiding this comment.
At the language option level, I think it's more idiomatic to use an enumeration instead of string literals and doing string processing.
There was a problem hiding this comment.
I'd suggest doing a string-switch on it, then storing an enum in the AST. We can then print it/whatever on the way out. But a pair of conversion functions is typically all you should need. Though, it doesn't seem that these are leaving the handle function anyway, so IDK.
Either way, LangOpts is a little bit of an odd place for this to live.
There was a problem hiding this comment.
It's not stored as a string (or even an attribute), they're stored as flags and enums in the pointer auth qualifier on the type.
There was a problem hiding this comment.
I was meaning "is there a more idiomatic way to have these string constants specified?" :D
There was a problem hiding this comment.
Oh, I hadn't included the updated to stringifying the qualifier so these were only used in one place.
| } else { | ||
| Expr::EvalResult Eval; | ||
| bool Result = AuthenticationOptionsArg->EvaluateAsRValue(Eval, Ctx); | ||
| if (Result && Eval.Val.isLValue()) { |
There was a problem hiding this comment.
This section can be replaced once #135864 or similar is landed as it supports const char* constant expressions
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) Changes[clang][PAC] add support for options parameter to __ptrauth This PR adds support for an 'options' parameter for the __ptrauth The initial version only exposes the authehntication modes:
We also support parsing the options but not yet the implementation
The initial support for authentication mode controls exist to support Patch is 44.28 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/136828.diff 12 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 37c80ac90182c..1941bb2d1febc 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3595,9 +3595,9 @@ def ObjCRequiresPropertyDefs : InheritableAttr {
def PointerAuth : TypeAttr {
let Spellings = [CustomKeyword<"__ptrauth">];
- let Args = [IntArgument<"Key">,
- BoolArgument<"AddressDiscriminated", 1>,
- IntArgument<"ExtraDiscriminator", 1>];
+ let Args = [IntArgument<"Key">, BoolArgument<"AddressDiscriminated", 1>,
+ IntArgument<"ExtraDiscriminator", 1>,
+ StringArgument<"Options", 1>];
let Documentation = [PtrAuthDocs];
}
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 3bbdc49946dac..8e0f818714c42 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1735,8 +1735,8 @@ def warn_pragma_unroll_cuda_value_in_parens : Warning<
"argument to '#pragma unroll' should not be in parentheses in CUDA C/C++">,
InGroup<CudaCompat>;
-def err_ptrauth_qualifier_bad_arg_count : Error<
- "'__ptrauth' qualifier must take between 1 and 3 arguments">;
+def err_ptrauth_qualifier_bad_arg_count
+ : Error<"'__ptrauth' qualifier must take between 1 and 4 arguments">;
def warn_cuda_attr_lambda_position : Warning<
"nvcc does not allow '__%0__' to appear after the parameter list in lambdas">,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 89b2d664d66a0..0ae2c09b1e4fb 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1045,6 +1045,23 @@ def err_ptrauth_extra_discriminator_invalid : Error<
"invalid extra discriminator flag '%0'; '__ptrauth' requires a value between "
"'0' and '%1'">;
+// __ptrauth qualifier options string
+def note_ptrauth_evaluating_options
+ : Note<"options parameter evaluated to '%0'">;
+def err_ptrauth_invalid_option : Error<"'%0' options parameter %1">;
+def err_ptrauth_unknown_authentication_option
+ : Error<"unknown '%0' authentication option '%1'">;
+def err_ptrauth_repeated_authentication_option
+ : Error<"repeated '%0' authentication %select{mode|option}1%select{, prior mode was '%3'| '%2'}1">;
+def note_ptrauth_previous_authentication_option
+ : Note<"previous '%0' authentication %select{mode|option}1">;
+def err_ptrauth_unexpected_option_end
+ : Error<"unexpected end of options parameter for %0">;
+def err_ptrauth_option_unexpected_token
+ : Error<"unexpected character '%0' in '%1' options">;
+def err_ptrauth_option_missing_comma
+ : Error<"missing comma after '%1' option in '%0' qualifier">;
+
/// main()
// static main() is not an error in C, just in C++.
def warn_static_main : Warning<"'main' should not be declared static">,
@@ -1735,7 +1752,6 @@ def err_static_assert_requirement_failed : Error<
def note_expr_evaluates_to : Note<
"expression evaluates to '%0 %1 %2'">;
-
def subst_user_defined_msg : TextSubstitution<
"%select{the message|the expression}0 in "
"%select{a static assertion|this asm operand}0">;
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 491e8bee9fd5c..3944946374d30 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -65,6 +65,17 @@ enum class PointerAuthenticationMode : unsigned {
SignAndAuth
};
+static constexpr llvm::StringLiteral PointerAuthenticationOptionStrip = "strip";
+static constexpr llvm::StringLiteral PointerAuthenticationOptionSignAndStrip =
+ "sign-and-strip";
+static constexpr llvm::StringLiteral PointerAuthenticationOptionSignAndAuth =
+ "sign-and-auth";
+static constexpr llvm::StringLiteral PointerAuthenticationOptionIsaPointer =
+ "isa-pointer";
+static constexpr llvm::StringLiteral
+ PointerAuthenticationOptionAuthenticatesNullValues =
+ "authenticates-null-values";
+
/// Bitfields of LangOptions, split out from LangOptions in order to ensure that
/// this large collection of bitfields is a trivial class type.
class LangOptionsBase {
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 6ba45f42db4d1..1de3d8a181343 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -2129,6 +2129,13 @@ class ConstantLValueEmitter : public ConstStmtVisitor<ConstantLValueEmitter,
}
+static bool shouldSignPointer(const PointerAuthQualifier &PointerAuth) {
+ PointerAuthenticationMode AuthenticationMode =
+ PointerAuth.getAuthenticationMode();
+ return AuthenticationMode == PointerAuthenticationMode::SignAndStrip ||
+ AuthenticationMode == PointerAuthenticationMode::SignAndAuth;
+}
+
llvm::Constant *ConstantLValueEmitter::tryEmit() {
const APValue::LValueBase &base = Value.getLValueBase();
@@ -2162,7 +2169,8 @@ llvm::Constant *ConstantLValueEmitter::tryEmit() {
// Apply pointer-auth signing from the destination type.
if (PointerAuthQualifier PointerAuth = DestType.getPointerAuth();
- PointerAuth && !result.HasDestPointerAuth) {
+ PointerAuth && !result.HasDestPointerAuth &&
+ shouldSignPointer(PointerAuth)) {
value = Emitter.tryEmitConstantSignedPointer(value, PointerAuth);
if (!value)
return nullptr;
@@ -2210,8 +2218,9 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
if (D->hasAttr<WeakRefAttr>())
return CGM.GetWeakRefReference(D).getPointer();
- auto PtrAuthSign = [&](llvm::Constant *C) {
- if (PointerAuthQualifier PointerAuth = DestType.getPointerAuth()) {
+ auto PtrAuthSign = [&](llvm::Constant *C, bool IsFunction) {
+ if (PointerAuthQualifier PointerAuth = DestType.getPointerAuth();
+ PointerAuth && shouldSignPointer(PointerAuth)) {
C = applyOffset(C);
C = Emitter.tryEmitConstantSignedPointer(C, PointerAuth);
return ConstantLValue(C, /*applied offset*/ true, /*signed*/ true);
@@ -2219,7 +2228,7 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
CGPointerAuthInfo AuthInfo;
- if (EnablePtrAuthFunctionTypeDiscrimination)
+ if (IsFunction && EnablePtrAuthFunctionTypeDiscrimination)
AuthInfo = CGM.getFunctionPointerAuthInfo(DestType);
if (AuthInfo) {
@@ -2237,17 +2246,18 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
};
if (const auto *FD = dyn_cast<FunctionDecl>(D))
- return PtrAuthSign(CGM.getRawFunctionPointer(FD));
+ return PtrAuthSign(CGM.getRawFunctionPointer(FD), /*IsFunction=*/true);
if (const auto *VD = dyn_cast<VarDecl>(D)) {
// We can never refer to a variable with local storage.
if (!VD->hasLocalStorage()) {
if (VD->isFileVarDecl() || VD->hasExternalStorage())
- return CGM.GetAddrOfGlobalVar(VD);
+ return PtrAuthSign(CGM.GetAddrOfGlobalVar(VD), /*IsFunction=*/false);
if (VD->isLocalVarDecl()) {
- return CGM.getOrCreateStaticVarDecl(
+ llvm::Constant *C = CGM.getOrCreateStaticVarDecl(
*VD, CGM.getLLVMLinkageVarDefinition(VD));
+ return PtrAuthSign(C, /*IsFunction=*/false);
}
}
}
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 4fe3565687905..259ce5029271e 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -3428,7 +3428,7 @@ void Parser::ParsePtrauthQualifier(ParsedAttributes &Attrs) {
T.consumeClose();
SourceLocation EndLoc = T.getCloseLocation();
- if (ArgExprs.empty() || ArgExprs.size() > 3) {
+ if (ArgExprs.empty() || ArgExprs.size() > 4) {
Diag(KwLoc, diag::err_ptrauth_qualifier_bad_arg_count);
return;
}
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index a8e85c885069e..654ac4d903f51 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8350,14 +8350,16 @@ static void HandleNeonVectorTypeAttr(QualType &CurType, const ParsedAttr &Attr,
/// Handle the __ptrauth qualifier.
static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
const ParsedAttr &Attr, Sema &S) {
-
- assert((Attr.getNumArgs() > 0 && Attr.getNumArgs() <= 3) &&
- "__ptrauth qualifier takes between 1 and 3 arguments");
+ assert((Attr.getNumArgs() > 0 && Attr.getNumArgs() <= 4) &&
+ "__ptrauth qualifier takes between 1 and 4 arguments");
+ StringRef AttrName = Attr.getAttrName()->getName();
Expr *KeyArg = Attr.getArgAsExpr(0);
Expr *IsAddressDiscriminatedArg =
Attr.getNumArgs() >= 2 ? Attr.getArgAsExpr(1) : nullptr;
Expr *ExtraDiscriminatorArg =
Attr.getNumArgs() >= 3 ? Attr.getArgAsExpr(2) : nullptr;
+ Expr *AuthenticationOptionsArg =
+ Attr.getNumArgs() >= 4 ? Attr.getArgAsExpr(3) : nullptr;
unsigned Key;
if (S.checkConstantPointerAuthKey(KeyArg, Key)) {
@@ -8373,10 +8375,140 @@ static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
IsAddressDiscriminated);
IsInvalid |= !S.checkPointerAuthDiscriminatorArg(
ExtraDiscriminatorArg, PointerAuthDiscArgKind::Extra, ExtraDiscriminator);
+ std::string LastAuthenticationMode;
+ std::optional<PointerAuthenticationMode> AuthenticationMode = std::nullopt;
+ bool IsIsaPointer = false;
+ bool AuthenticatesNullValues = false;
+
+ if (AuthenticationOptionsArg && !AuthenticationOptionsArg->containsErrors()) {
+ StringRef OptionsString;
+ std::string EvaluatedString;
+ bool HasEvaluatedOptionsString = false;
+ const StringLiteral *OptionsStringLiteral =
+ dyn_cast<StringLiteral>(AuthenticationOptionsArg);
+ SourceRange AuthenticationOptionsRange =
+ AuthenticationOptionsArg->getSourceRange();
+ bool ReportedEvaluation = false;
+ auto ReportEvaluationOfExpressionIfNeeded = [&]() {
+ if (OptionsStringLiteral || !HasEvaluatedOptionsString ||
+ ReportedEvaluation)
+ return;
+ ReportedEvaluation = true;
+ S.Diag(AuthenticationOptionsRange.getBegin(),
+ diag::note_ptrauth_evaluating_options)
+ << OptionsString << AuthenticationOptionsRange;
+ };
+ auto DiagnoseInvalidOptionsParameter = [&](llvm::StringRef Reason) {
+ S.Diag(AuthenticationOptionsRange.getBegin(),
+ diag::err_ptrauth_invalid_option)
+ << AttrName << Reason;
+ Attr.setInvalid();
+ IsInvalid = true;
+ ReportEvaluationOfExpressionIfNeeded();
+ };
+ if (AuthenticationOptionsArg->isValueDependent() ||
+ AuthenticationOptionsArg->isTypeDependent()) {
+ DiagnoseInvalidOptionsParameter("is dependent");
+ return;
+ }
+ if (OptionsStringLiteral) {
+ OptionsString = OptionsStringLiteral->getString();
+ HasEvaluatedOptionsString = true;
+ } else {
+ Expr::EvalResult Eval;
+ bool Result = AuthenticationOptionsArg->EvaluateAsRValue(Eval, Ctx);
+ if (Result && Eval.Val.isLValue()) {
+ auto *BaseExpr = Eval.Val.getLValueBase().dyn_cast<const Expr *>();
+ const StringLiteral *EvaluatedStringLiteral =
+ dyn_cast<StringLiteral>(const_cast<Expr *>(BaseExpr));
+ if (EvaluatedStringLiteral) {
+ CharUnits StartOffset = Eval.Val.getLValueOffset();
+ EvaluatedString = EvaluatedStringLiteral->getString().drop_front(
+ StartOffset.getQuantity());
+ OptionsString = EvaluatedString;
+ HasEvaluatedOptionsString = true;
+ }
+ }
+ }
+ if (!HasEvaluatedOptionsString) {
+ DiagnoseInvalidOptionsParameter(
+ "must be a string of comma separated flags");
+ return;
+ }
+ for (char Ch : OptionsString) {
+ if (Ch != '-' && Ch != ',' && !isWhitespace(Ch) && !isalpha(Ch)) {
+ DiagnoseInvalidOptionsParameter("contains invalid characters");
+ return;
+ }
+ }
+ HasEvaluatedOptionsString = true;
+ OptionsString = OptionsString.trim();
+ llvm::SmallVector<StringRef> Options;
+ if (!OptionsString.empty())
+ OptionsString.split(Options, ',');
+
+ auto OptionHandler = [&](auto Value, auto *Option,
+ std::string *LastOption = nullptr) {
+ return [&, Value, Option, LastOption](StringRef OptionString) {
+ if (!*Option) {
+ *Option = Value;
+ if (LastOption)
+ *LastOption = OptionString;
+ return true;
+ }
+ bool IsAuthenticationMode =
+ std::is_same_v<decltype(Value), PointerAuthenticationMode>;
+ S.Diag(AuthenticationOptionsRange.getBegin(),
+ diag::err_ptrauth_repeated_authentication_option)
+ << AttrName << !IsAuthenticationMode << OptionString
+ << (LastOption ? *LastOption : "");
+ return false;
+ };
+ };
- if (IsInvalid) {
- Attr.setInvalid();
- return;
+ for (unsigned Idx = 0; Idx < Options.size(); ++Idx) {
+ StringRef Option = Options[Idx].trim();
+ if (Option.empty()) {
+ bool IsLastOption = Idx == (Options.size() - 1);
+ DiagnoseInvalidOptionsParameter(
+ IsLastOption ? "has a trailing comma" : "contains an empty option");
+ continue;
+ }
+ auto SelectedHandler =
+ llvm::StringSwitch<std::function<bool(StringRef)>>(Option)
+ .Case(PointerAuthenticationOptionStrip,
+ OptionHandler(PointerAuthenticationMode::Strip,
+ &AuthenticationMode, &LastAuthenticationMode))
+ .Case(PointerAuthenticationOptionSignAndStrip,
+ OptionHandler(PointerAuthenticationMode::SignAndStrip,
+ &AuthenticationMode, &LastAuthenticationMode))
+ .Case(PointerAuthenticationOptionSignAndAuth,
+ OptionHandler(PointerAuthenticationMode::SignAndAuth,
+ &AuthenticationMode, &LastAuthenticationMode))
+ .Case(PointerAuthenticationOptionIsaPointer,
+ OptionHandler(true,
+ &IsIsaPointer))
+ .Case(PointerAuthenticationOptionAuthenticatesNullValues,
+ OptionHandler(true,
+ &AuthenticatesNullValues))
+ .Default([&](StringRef Option) {
+ if (size_t WhitespaceIndex =
+ Option.find_first_of(" \t\n\v\f\r");
+ WhitespaceIndex != Option.npos) {
+ StringRef LeadingOption = Option.slice(0, WhitespaceIndex);
+ S.Diag(AuthenticationOptionsRange.getBegin(),
+ diag::err_ptrauth_option_missing_comma)
+ << AttrName << LeadingOption;
+ } else {
+ S.Diag(AuthenticationOptionsRange.getBegin(),
+ diag::err_ptrauth_unknown_authentication_option)
+ << AttrName << Option;
+ }
+ return false;
+ });
+ if (!SelectedHandler(Option))
+ IsInvalid = true;
+ }
}
if (!T->isSignableType(Ctx) && !T->isDependentType()) {
@@ -8385,6 +8517,9 @@ static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
return;
}
+ if (!AuthenticationMode)
+ AuthenticationMode = PointerAuthenticationMode::SignAndAuth;
+
if (T.getPointerAuth()) {
S.Diag(Attr.getLoc(), diag::err_ptrauth_qualifier_redundant) << T;
Attr.setInvalid();
@@ -8397,13 +8532,19 @@ static void HandlePtrAuthQualifier(ASTContext &Ctx, QualType &T,
return;
}
+ if (IsInvalid) {
+ Attr.setInvalid();
+ return;
+ }
+
assert((!IsAddressDiscriminatedArg || IsAddressDiscriminated <= 1) &&
"address discriminator arg should be either 0 or 1");
PointerAuthQualifier Qual = PointerAuthQualifier::Create(
- Key, IsAddressDiscriminated, ExtraDiscriminator,
- PointerAuthenticationMode::SignAndAuth, /*IsIsaPointer=*/false,
- /*AuthenticatesNullValues=*/false);
+ Key, IsAddressDiscriminated, ExtraDiscriminator, *AuthenticationMode,
+ IsIsaPointer, AuthenticatesNullValues);
+ assert(Qual.getAuthenticationMode() == *AuthenticationMode);
T = S.Context.getPointerAuthType(T, Qual);
+ assert(T.getPointerAuth().getAuthenticationMode() == *AuthenticationMode);
}
/// HandleArmSveVectorBitsTypeAttr - The "arm_sve_vector_bits" attribute is
diff --git a/clang/test/CodeGen/ptrauth-stripping.c b/clang/test/CodeGen/ptrauth-stripping.c
new file mode 100644
index 0000000000000..4e187d8debdc7
--- /dev/null
+++ b/clang/test/CodeGen/ptrauth-stripping.c
@@ -0,0 +1,327 @@
+// RUN: %clang_cc1 -triple arm64-apple-ios -fptrauth-calls -fptrauth-intrinsics -emit-llvm %s -o - | FileCheck %s
+
+typedef void *NonePointer;
+typedef void *__ptrauth(1, 1, 101, "strip") StripPointer;
+typedef void *__ptrauth(1, 1, 102, "sign-and-strip") SignAndStripPointer;
+typedef void *__ptrauth(1, 1, 103, "sign-and-auth") SignAndAuthPointer;
+typedef __UINT64_TYPE__ NoneIntptr;
+typedef __UINT64_TYPE__ __ptrauth(1, 0, 105, "strip") StripIntptr;
+typedef __UINT64_TYPE__ __ptrauth(1, 0, 106, "sign-and-strip") SignAndStripIntptr;
+typedef __UINT64_TYPE__ __ptrauth(1, 0, 107, "sign-and-auth") SignAndAuthIntptr;
+
+NonePointer globalNonePointer = "foo0";
+StripPointer globalStripPointer = "foo1";
+SignAndStripPointer globalSignAndStripPointer = "foo2";
+SignAndAuthPointer globalSignAndAuthPointer = "foo3";
+NoneIntptr globalNoneIntptr = (__UINT64_TYPE__)&globalNonePointer;
+StripIntptr globalStripIntptr = (__UINT64_TYPE__)&globalStripPointer;
+SignAndStripIntptr globalSignAndStripIntptr = (__UINT64_TYPE__)&globalSignAndStripPointer;
+SignAndAuthIntptr globalSignAndAuthIntptr = (__UINT64_TYPE__)&globalSignAndAuthPointer;
+
+// CHECK: @.str = private unnamed_addr constant [5 x i8] c"foo0\00", align 1
+// CHECK: @globalNonePointer = global ptr @.str, align 8
+// CHECK: @.str.1 = private unnamed_addr constant [5 x i8] c"foo1\00", align 1
+// CHECK: @globalStripPointer = global ptr @.str.1, align 8
+// CHECK: @.str.2 = private unnamed_addr constant [5 x i8] c"foo2\00", align 1
+// CHECK: @globalSignAndStripPointer = global ptr ptrauth (ptr @.str.2, i32 1, i64 102, ptr @globalSignAndStripPointer), align 8
+// CHECK: @.str.3 = private unnamed_addr constant [5 x i8] c"foo3\00", align 1
+// CHECK: @globalSignAndAuthPointer = global ptr ptrauth (ptr @.str.3, i32 1, i64 103, ptr @globalSignAndAuthPointer), align 8
+// CHECK: @globalNoneIntptr = global i64 ptrtoint (ptr @globalNonePointer to i64), align 8
+// CHECK: @globalStripIntptr = global i64 ptrtoint (ptr @globalStripPointer to i64), align 8
+// CHECK: @globalSignAndStripIntptr = global i64 ptrtoint (ptr ptrauth (ptr @globalSignAndStripPointer, i32 1, i64 106) to i64), align 8
+// CHECK: @globalSignAndAuthIntptr = global i64 ptrtoint (ptr ptrauth (ptr @globalSignAndAuthPointer, i32 1, i64 107) to i64), align 8
+
+typedef struct {
+ NonePointer ptr;
+ NoneIntptr i;
+} NoneStruct;
+typedef struct {
+ StripPointer ptr;
+ StripIntptr i;
+} StripStruct;
+typedef struct {
+ SignAndStripPointer ptr;
+ SignAndStripIntptr i;
+} SignAndStripStruct;
+typedef struct {
+ SignAndAuthPointer ptr;
+ SignAndAuthIntptr i;
+} SignAndAuthStruct;
+
+// CHECK-LABEL: @testNone
+NoneStruct testNone(NoneStruct *a, NoneStruct *b, NoneStruct c) {
+ globalNonePointer += 1;
+ // CHECK: [[GLOBALP:%.*]] = load ptr, ptr @globalNonePointer
+ // CHECK: [[GLOBALPP:%.*]] = getelementptr inbounds i8, ptr [[GLOBALP]], i64 1
+ // CHECK: store ptr [[GLOBALPP]], ptr @globalNonePointer
+ globalNoneIntptr += 1;
+ // CHECK: [[GLOBALI:%.*]] = load i64, ptr @globalNoneIntptr
+ // CHECK: [[GLOBALIP:%.*]] = add i64 [[GLOBALI]], 1
+ // CHECK: store i64 [[GLOBALIP]], ptr @globalNoneIntptr
+ a->ptr += 1;
+ // CHECK: [[PTR:%.*]] = load ptr, ptr %a.addr, align 8
+ // CHECK: [[PTR_PTR:%.*]] = getelementptr inbounds nuw %struct.NoneStruct, ptr [[PTR]], i32 0, i32 0
+ // CHECK: [[PTR:%.*]] = load ptr, ptr [[PTR_PTR]], align 8
+ // CHECK: [[AP:%.*]] = getelementptr inbounds i8, ptr [[PTR]], i64 1
+ // CHECK: stor...
[truncated]
|
ojhunt
force-pushed
the
users/ojhunt/ptrauth-options
branch
from
6f161d3 to
8d42eca
Compare
| @@ -1735,8 +1735,8 @@ def warn_pragma_unroll_cuda_value_in_parens : Warning< | |||
| "argument to '#pragma unroll' should not be in parentheses in CUDA C/C++">, | |||
| InGroup<CudaCompat>; | |||
|
|
|||
| def err_ptrauth_qualifier_bad_arg_count : Error< | |||
| "'__ptrauth' qualifier must take between 1 and 3 arguments">; | |||
| def err_ptrauth_qualifier_bad_arg_count | |||
There was a problem hiding this comment.
This is clang-format induced (aside from the 3 vs 4 change obviously) I can more the Error< back but it might be worth trying to work out how to stop clang-format from making this choice
There was a problem hiding this comment.
I'd recommend manually formatting diagnostic files rather than using clang-format, at least until the tool learns (or can be configured for) our conventions.
There was a problem hiding this comment.
yeah, I wonder if we can make clang-format exclude the diagnostic files
| IntArgument<"ExtraDiscriminator", 1>]; | ||
| let Args = [IntArgument<"Key">, BoolArgument<"AddressDiscriminated", 1>, | ||
| IntArgument<"ExtraDiscriminator", 1>, | ||
| StringArgument<"Options", 1>]; |
There was a problem hiding this comment.
Should there be an update to AttrDocs.td for the new argument?
There was a problem hiding this comment.
I'll see if there's any existing documentation as the attribute can't be specified by attribute syntax
There was a problem hiding this comment.
Updated the PointerAuthentication document
| @@ -1735,8 +1735,8 @@ def warn_pragma_unroll_cuda_value_in_parens : Warning< | |||
| "argument to '#pragma unroll' should not be in parentheses in CUDA C/C++">, | |||
| InGroup<CudaCompat>; | |||
|
|
|||
| def err_ptrauth_qualifier_bad_arg_count : Error< | |||
| "'__ptrauth' qualifier must take between 1 and 3 arguments">; | |||
| def err_ptrauth_qualifier_bad_arg_count | |||
There was a problem hiding this comment.
I'd recommend manually formatting diagnostic files rather than using clang-format, at least until the tool learns (or can be configured for) our conventions.
| @@ -65,6 +65,17 @@ enum class PointerAuthenticationMode : unsigned { | |||
| SignAndAuth | |||
| }; | |||
|
|
|||
| static constexpr llvm::StringLiteral PointerAuthenticationOptionStrip = "strip"; | |||
There was a problem hiding this comment.
At the language option level, I think it's more idiomatic to use an enumeration instead of string literals and doing string processing.
| @@ -65,6 +65,17 @@ enum class PointerAuthenticationMode : unsigned { | |||
| SignAndAuth | |||
| }; | |||
|
|
|||
| static constexpr llvm::StringLiteral PointerAuthenticationOptionStrip = "strip"; | |||
There was a problem hiding this comment.
I'd suggest doing a string-switch on it, then storing an enum in the AST. We can then print it/whatever on the way out. But a pair of conversion functions is typically all you should need. Though, it doesn't seem that these are leaving the handle function anyway, so IDK.
Either way, LangOpts is a little bit of an odd place for this to live.
This PR adds support for an 'options' parameter for the __ptrauth qualifier. The initial version only exposes the authehntication modes: * "strip" * "sign-and-strip" * "sign-and-auth" We also support parsing the options but not yet the implementation * "isa-pointer" * "authenticates-null-values" The initial support for authentication mode controls exist to support ABI changes over time, and as a byproduct support basic initial tests for option parsing.
ojhunt
force-pushed
the
users/ojhunt/ptrauth-options
branch
from
f6f11a5 to
927380b
Compare
| separated modifiers to the normal authentication behavior. Currently supported | ||
| options are | ||
|
|
||
| - Authentication mode: This is one of ``strip``, ``sign-and-strip``, and |
There was a problem hiding this comment.
This PR does not actually provide the implementation of this option, as it touches a number of places unrelated to parsing the option.
[clang][PAC] add support for options parameter to __ptrauth
This PR adds support for an 'options' parameter for the __ptrauth
qualifier.
The initial version only exposes the authehntication modes:
We also support parsing the options but not yet the implementation
The initial support for authentication mode controls exist to support
ABI changes over time, and as a byproduct support basic initial tests
for option parsing.