Review comments

- Changed the separator from '+' to ','. - Moved the branch protection options from -C to -Z. - Additional test for incorrect branch-protection option. - Remove LLVM < 12 code. - Style fixes. Co-authored-by: James McGregor <james.mcgregor2@arm.com>
ehuss · Dec 1, 2021 · 984ca46 · 984ca46
1 parent 837cc16
commit 984ca46
Show file tree

Hide file tree

Showing 12 changed files with 57 additions and 120 deletions.
diff --git a/compiler/rustc_codegen_llvm/src/attributes.rs b/compiler/rustc_codegen_llvm/src/attributes.rs
@@ -9,7 +9,7 @@ use rustc_hir::def_id::DefId;
 use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
 use rustc_middle::ty::layout::HasTyCtxt;
 use rustc_middle::ty::{self, TyCtxt};
-use rustc_session::config::{BranchProtection, OptLevel, PAuthKey};
+use rustc_session::config::OptLevel;
 use rustc_session::Session;
 use rustc_target::spec::abi::Abi;
 use rustc_target::spec::{FramePointer, SanitizerSet, StackProbeType, StackProtector};
@@ -203,58 +203,6 @@ pub fn non_lazy_bind(sess: &Session, llfn: &'ll Value) {
     }
 }
 
-pub fn set_branch_protection(sess: &Session, llfn: &'ll Value) {
-    // Setting PAC/BTI function attributes is only necessary for LLVM 11 and earlier.
-    // For LLVM 12 and greater, module-level metadata attributes are set in
-    // `compiler/rustc_codegen_llvm/src/context.rs`.
-    if llvm_util::get_version() >= (12, 0, 0) {
-        return;
-    }
-
-    let BranchProtection { bti, pac_ret: pac } = sess.opts.cg.branch_protection;
-
-    if bti {
-        llvm::AddFunctionAttrString(
-            llfn,
-            llvm::AttributePlace::Function,
-            cstr!("branch-target-enforcement"),
-        );
-    }
-
-    if let Some(pac_opts) = pac {
-        if pac_opts.leaf {
-            llvm::AddFunctionAttrStringValue(
-                llfn,
-                llvm::AttributePlace::Function,
-                cstr!("sign-return-address"),
-                cstr!("non-leaf"),
-            );
-        } else {
-            llvm::AddFunctionAttrStringValue(
-                llfn,
-                llvm::AttributePlace::Function,
-                cstr!("sign-return-address"),
-                cstr!("all"),
-            );
-        }
-
-        match pac_opts.key {
-            PAuthKey::A => llvm::AddFunctionAttrStringValue(
-                llfn,
-                llvm::AttributePlace::Function,
-                cstr!("sign-return-address-key"),
-                cstr!("a_key"),
-            ),
-            PAuthKey::B => llvm::AddFunctionAttrStringValue(
-                llfn,
-                llvm::AttributePlace::Function,
-                cstr!("sign-return-address-key"),
-                cstr!("b_key"),
-            ),
-        }
-    }
-}
-
 pub(crate) fn default_optimisation_attrs(sess: &Session, llfn: &'ll Value) {
     match sess.opts.optimize {
         OptLevel::Size => {

diff --git a/compiler/rustc_codegen_llvm/src/context.rs b/compiler/rustc_codegen_llvm/src/context.rs
@@ -21,7 +21,7 @@ use rustc_middle::ty::layout::{
 };
 use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
 use rustc_middle::{bug, span_bug};
-use rustc_session::config::{BranchProtection, CFGuard, CrateType, DebugInfo, PAuthKey};
+use rustc_session::config::{BranchProtection, CFGuard, CrateType, DebugInfo, PAuthKey, PacRet};
 use rustc_session::Session;
 use rustc_span::source_map::Span;
 use rustc_span::symbol::Symbol;
@@ -243,35 +243,31 @@ pub unsafe fn create_module(
     }
 
     if sess.target.arch == "aarch64" {
-        let BranchProtection { bti, pac_ret: pac } = sess.opts.cg.branch_protection;
+        let BranchProtection { bti, pac_ret: pac } = sess.opts.debugging_opts.branch_protection;
 
         llvm::LLVMRustAddModuleFlag(
             llmod,
             "branch-target-enforcement\0".as_ptr().cast(),
             bti.into(),
         );
 
-        if let Some(pac_opts) = pac {
-            llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address\0".as_ptr().cast(), 1);
-            llvm::LLVMRustAddModuleFlag(
-                llmod,
-                "sign-return-address-all\0".as_ptr().cast(),
-                pac_opts.leaf.into(),
-            );
-            llvm::LLVMRustAddModuleFlag(
-                llmod,
-                "sign-return-address-with-bkey\0".as_ptr().cast(),
-                if pac_opts.key == PAuthKey::A { 0 } else { 1 },
-            );
-        } else {
-            llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address\0".as_ptr().cast(), 0);
-            llvm::LLVMRustAddModuleFlag(llmod, "sign-return-address-all\0".as_ptr().cast(), 0);
-            llvm::LLVMRustAddModuleFlag(
-                llmod,
-                "sign-return-address-with-bkey\0".as_ptr().cast(),
-                0,
-            );
-        }
+        llvm::LLVMRustAddModuleFlag(
+            llmod,
+            "sign-return-address\0".as_ptr().cast(),
+            pac.is_some().into(),
+        );
+        let pac_opts = pac.unwrap_or(PacRet { leaf: false, key: PAuthKey::A });
+        llvm::LLVMRustAddModuleFlag(
+            llmod,
+            "sign-return-address-all\0".as_ptr().cast(),
+            pac_opts.leaf.into(),
+        );
+        let is_bkey = if pac_opts.key == PAuthKey::A { false } else { true };
+        llvm::LLVMRustAddModuleFlag(
+            llmod,
+            "sign-return-address-with-bkey\0".as_ptr().cast(),
+            is_bkey.into(),
+        );
     }
 
     llmod

diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs
@@ -45,10 +45,6 @@ fn declare_raw_fn(
         llvm::Attribute::NoRedZone.apply_llfn(Function, llfn);
     }
 
-    if cx.tcx.sess.target.arch == "aarch64" {
-        attributes::set_branch_protection(cx.tcx.sess, llfn);
-    }
-
     attributes::default_optimisation_attrs(cx.tcx.sess, llfn);
     attributes::non_lazy_bind(cx.sess(), llfn);
 

diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs
@@ -567,10 +567,6 @@ fn test_codegen_options_tracking_hash() {
 
     // Make sure that changing a [TRACKED] option changes the hash.
     // This list is in alphabetical order.
-    tracked!(
-        branch_protection,
-        BranchProtection { bti: true, pac_ret: Some(PacRet { leaf: true, key: PAuthKey::B }) }
-    );
     tracked!(code_model, Some(CodeModel::Large));
     tracked!(control_flow_guard, CFGuard::Checks);
     tracked!(debug_assertions, Some(true));
@@ -723,6 +719,10 @@ fn test_debugging_options_tracking_hash() {
     tracked!(asm_comments, true);
     tracked!(assume_incomplete_release, true);
     tracked!(binary_dep_depinfo, true);
+    tracked!(
+        branch_protection,
+        BranchProtection { bti: true, pac_ret: Some(PacRet { leaf: true, key: PAuthKey::B }) }
+    );
     tracked!(chalk, true);
     tracked!(codegen_backend, Some("abc".to_string()));
     tracked!(crate_attr, vec!["abc".to_string()]);

diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs
@@ -390,7 +390,7 @@ mod desc {
     pub const parse_stack_protector: &str =
         "one of (`none` (default), `basic`, `strong`, or `all`)";
     pub const parse_branch_protection: &str =
-        "a `+` separated combination of `bti`, `b-key`, `pac-ret`, or `leaf`";
+        "a `,` separated combination of `bti`, `b-key`, `pac-ret`, or `leaf`";
 }
 
 mod parse {
@@ -935,7 +935,7 @@ mod parse {
     crate fn parse_branch_protection(slot: &mut BranchProtection, v: Option<&str>) -> bool {
         match v {
             Some(s) => {
-                for opt in s.split('+') {
+                for opt in s.split(',') {
                     match opt {
                         "bti" => slot.bti = true,
                         "pac-ret" if slot.pac_ret.is_none() => {
@@ -953,7 +953,6 @@ mod parse {
                     };
                 }
             }
-
             _ => return false,
         }
         true
@@ -971,8 +970,6 @@ options! {
 
     ar: String = (String::new(), parse_string, [UNTRACKED],
         "this option is deprecated and does nothing"),
-    branch_protection: BranchProtection = (BranchProtection::default(), parse_branch_protection, [TRACKED],
-        "set options for branch target identification and pointer authentication on AArch64"),
     code_model: Option<CodeModel> = (None, parse_code_model, [TRACKED],
         "choose the code model to use (`rustc --print code-models` for details)"),
     codegen_units: Option<usize> = (None, parse_opt_number, [UNTRACKED],
@@ -1101,6 +1098,8 @@ options! {
         (default: no)"),
     borrowck: String = ("migrate".to_string(), parse_string, [UNTRACKED],
         "select which borrowck is used (`mir` or `migrate`) (default: `migrate`)"),
+    branch_protection: BranchProtection = (BranchProtection::default(), parse_branch_protection, [TRACKED],
+        "set options for branch target identification and pointer authentication on AArch64"),
     cgu_partitioning_strategy: Option<String> = (None, parse_opt_string, [TRACKED],
         "the codegen unit partitioning strategy to use"),
     chalk: bool = (false, parse_bool, [TRACKED],

diff --git a/src/doc/rustc/src/codegen-options/index.md b/src/doc/rustc/src/codegen-options/index.md
@@ -7,29 +7,6 @@ a version of this list for your exact compiler by running `rustc -C help`.
 
 This option is deprecated and does nothing.
 
-## branch-protection
-
-This option lets you enable branch authentication instructions on AArch64.
-This option is ignored for non-AArch64 architectures.
-It takes some combination of the following values, separated by a `+`.
-
-- `pac-ret` - Enable pointer authentication for non-leaf functions.
-- `leaf` - Enable pointer authentication for all functions, including leaf functions.
-- `b-key` - Sign return addresses with key B, instead of the default key A.
-- `bti` - Enable branch target identification.
-
-`leaf` and `b-key` are only valid if `pac-ret` was previously specified.
-For example, `-C branch-protection=bti+pac-ret+leaf` is valid, but
-`-C branch-protection=bti+leaf+pac-ret` is not.
-
-Repeated values are ignored.
-For example, `-C branch-protection=pac-ret+leaf+pac-ret` is equivalent to
-`-C branch-protection=pac-ret+leaf`.
-
-Rust's standard library does not ship with BTI or pointer authentication enabled by default. \
-In Cargo projects the standard library can be recompiled with pointer authentication using the nightly
-[build-std](https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-std) feature.
-
 ## code-model
 
 This option lets you choose which code model to use. \

diff --git a/src/doc/unstable-book/src/compiler-flags/branch-protection.md b/src/doc/unstable-book/src/compiler-flags/branch-protection.md
@@ -0,0 +1,18 @@
+# `branch-protection`
+
+This option lets you enable branch authentication instructions on AArch64.
+This option is ignored for non-AArch64 architectures.
+It takes some combination of the following values, separated by a `,`.
+
+- `pac-ret` - Enable pointer authentication for non-leaf functions.
+- `leaf` - Enable pointer authentication for all functions, including leaf functions.
+- `b-key` - Sign return addresses with key B, instead of the default key A.
+- `bti` - Enable branch target identification.
+
+`leaf` and `b-key` are only valid if `pac-ret` was previously specified.
+For example, `-Z branch-protection=bti,pac-ret,leaf` is valid, but
+`-Z branch-protection=bti,leaf,pac-ret` is not.
+
+Rust's standard library does not ship with BTI or pointer authentication enabled by default.
+In Cargo projects the standard library can be recompiled with pointer authentication using the nightly
+[build-std](https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#build-std) feature.
diff --git a/src/test/assembly/aarch64-pointer-auth.rs b/src/test/assembly/aarch64-pointer-auth.rs
@@ -3,7 +3,7 @@
 // min-llvm-version: 10.0.1
 // assembly-output: emit-asm
 // compile-flags: --target aarch64-unknown-linux-gnu
-// compile-flags: -C branch-protection=pac-ret+leaf
+// compile-flags: -Z branch-protection=pac-ret,leaf
 // needs-llvm-components: aarch64
 
 #![feature(no_core, lang_items)]

diff --git a/src/test/codegen/branch-protection.rs b/src/test/codegen/branch-protection.rs
@@ -3,10 +3,10 @@
 // revisions: bti pac-ret leaf b-key
 // min-llvm-version: 12.0.0
 // needs-llvm-components: aarch64
-// [bti] compile-flags: -C branch-protection=bti
-// [pac-ret] compile-flags: -C branch-protection=pac-ret
-// [leaf] compile-flags: -C branch-protection=pac-ret+leaf
-// [b-key] compile-flags: -C branch-protection=pac-ret+b-key
+// [bti] compile-flags: -Z branch-protection=bti
+// [pac-ret] compile-flags: -Z branch-protection=pac-ret
+// [leaf] compile-flags: -Z branch-protection=pac-ret,leaf
+// [b-key] compile-flags: -Z branch-protection=pac-ret,b-key
 // compile-flags: --target aarch64-unknown-linux-gnu
 
 #![crate_type = "lib"]

diff --git a/src/test/run-make-fulldeps/pointer-auth-link-with-c/Makefile b/src/test/run-make-fulldeps/pointer-auth-link-with-c/Makefile
@@ -5,10 +5,10 @@
 all:
 	$(COMPILE_OBJ) $(TMPDIR)/test.o test.c
 	$(AR) rcs $(TMPDIR)/libtest.a $(TMPDIR)/test.o
-	$(RUSTC) -C branch-protection=bti+pac-ret+leaf test.rs
+	$(RUSTC) -Z branch-protection=bti,pac-ret,leaf test.rs
 	$(call RUN,test)
 
 	$(COMPILE_OBJ) $(TMPDIR)/test.o test.c -mbranch-protection=bti+pac-ret+leaf
 	$(AR) rcs $(TMPDIR)/libtest.a $(TMPDIR)/test.o
-	$(RUSTC) -C branch-protection=bti+pac-ret+leaf test.rs
-	$(call RUN,test)
+	$(RUSTC) -Z branch-protection=bti,pac-ret,leaf test.rs
+	$(call RUN,test)
diff --git a/src/test/ui/invalid-compile-flags/branch-protection-missing-pac-ret.rs b/src/test/ui/invalid-compile-flags/branch-protection-missing-pac-ret.rs
@@ -0,0 +1 @@
+// compile-flags: -Z branch-protection=leaf
diff --git a/src/test/ui/invalid-compile-flags/branch-protection-missing-pac-ret.stderr b/src/test/ui/invalid-compile-flags/branch-protection-missing-pac-ret.stderr
@@ -0,0 +1,2 @@
+error: incorrect value `leaf` for debugging option `branch-protection` - a `,` separated combination of `bti`, `b-key`, `pac-ret`, or `leaf` was expected
+
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		error: incorrect value `leaf` for debugging option `branch-protection` - a `,` separated combination of `bti`, `b-key`, `pac-ret`, or `leaf` was expected