Skip to content

Add TargetOptions::min_global_align, with s390x at 16-bit #44440

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

Merged
merged 2 commits into from
Sep 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions src/librustc_back/target/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,9 @@ pub struct TargetOptions {

/// Whether or not stack probes (__rust_probestack) are enabled
pub stack_probes: bool,

/// The minimum alignment for global symbols.
pub min_global_align: Option<u64>,
}

impl Default for TargetOptions {
Expand Down Expand Up @@ -486,6 +489,7 @@ impl Default for TargetOptions {
crt_static_default: false,
crt_static_respected: false,
stack_probes: false,
min_global_align: None,
}
}
}
Expand Down Expand Up @@ -724,6 +728,7 @@ impl Target {
key!(crt_static_default, bool);
key!(crt_static_respected, bool);
key!(stack_probes, bool);
key!(min_global_align, Option<u64>);

if let Some(array) = obj.find("abi-blacklist").and_then(Json::as_array) {
for name in array.iter().filter_map(|abi| abi.as_string()) {
Expand Down Expand Up @@ -914,6 +919,7 @@ impl ToJson for Target {
target_option_val!(crt_static_default);
target_option_val!(crt_static_respected);
target_option_val!(stack_probes);
target_option_val!(min_global_align);

if default.abi_blacklist != self.options.abi_blacklist {
d.insert("abi-blacklist".to_string(), self.options.abi_blacklist.iter()
Expand Down
1 change: 1 addition & 0 deletions src/librustc_back/target/s390x_unknown_linux_gnu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub fn target() -> TargetResult {
base.max_atomic_width = Some(64);
// see #36994
base.exe_allocation_crate = None;
base.min_global_align = Some(16);

Ok(Target {
llvm_target: "s390x-unknown-linux-gnu".to_string(),
Expand Down
24 changes: 22 additions & 2 deletions src/librustc_trans/consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use rustc::ty;

use rustc::hir;

use std::cmp;
use std::ffi::{CStr, CString};
use syntax::ast;
use syntax::attr;
Expand All @@ -42,6 +43,25 @@ pub fn bitcast(val: ValueRef, ty: Type) -> ValueRef {
}
}

fn set_global_alignment(ccx: &CrateContext,
gv: ValueRef,
mut align: machine::llalign) {
// The target may require greater alignment for globals than the type does.
// Note: GCC and Clang also allow `__attribute__((aligned))` on variables,
// which can force it to be smaller. Rust doesn't support this yet.
if let Some(min) = ccx.sess().target.target.options.min_global_align {
match ty::layout::Align::from_bits(min, min) {
Ok(min) => align = cmp::max(align, min.abi() as machine::llalign),
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 a .min method on Align, you should use that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want the greater of the two, so it would be Align::max. But this would require converting the incoming machine::llalign to Align, comparing, then back to llalign for the LLVM call. I don't see the point of that.

Copy link
Member

Choose a reason for hiding this comment

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

Well, llalign should have never existed etc. etc. I'll just have to remember fixing it on my branch where it's gone, but the rebase should catch it because of type mismatches and whatnot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it's also pointless to convert min_global_align to Align. I just figured a little sanity checking of that value would be good.

Copy link
Member

@eddyb eddyb Sep 9, 2017

Choose a reason for hiding this comment

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

That conversion is fine, since it will be in there sooner or later! In the future I plan to have librustc_back and a lot of librustc/ty/layout.rs be in the same crate so even the target specification would use it.

Err(err) => {
ccx.sess().err(&format!("invalid minimum global alignment: {}", err));
}
}
}
unsafe {
llvm::LLVMSetAlignment(gv, align);
}
}

pub fn addr_of_mut(ccx: &CrateContext,
cv: ValueRef,
align: machine::llalign,
Expand All @@ -53,7 +73,7 @@ pub fn addr_of_mut(ccx: &CrateContext,
bug!("symbol `{}` is already defined", name);
});
llvm::LLVMSetInitializer(gv, cv);
llvm::LLVMSetAlignment(gv, align);
set_global_alignment(ccx, gv, align);
llvm::LLVMRustSetLinkage(gv, llvm::Linkage::InternalLinkage);
SetUnnamedAddr(gv, true);
gv
Expand Down Expand Up @@ -276,7 +296,7 @@ pub fn trans_static<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
ccx.statics_to_rauw().borrow_mut().push((g, new_g));
new_g
};
llvm::LLVMSetAlignment(g, ccx.align_of(ty));
set_global_alignment(ccx, g, ccx.align_of(ty));
llvm::LLVMSetInitializer(g, v);

// As an optimization, all shared statics which do not have interior
Expand Down
22 changes: 22 additions & 0 deletions src/test/run-make/min-global-align/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
-include ../tools.mk

# This tests ensure that global variables respect the target minimum alignment.
# The three bools `STATIC_BOOL`, `STATIC_MUT_BOOL`, and `CONST_BOOL` all have
# type-alignment of 1, but some targets require greater global alignment.

SRC = min_global_align.rs
LL = $(TMPDIR)/min_global_align.ll

all:
ifeq ($(UNAME),Linux)
# Most targets are happy with default alignment -- take i686 for example.
ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86)
$(RUSTC) --target=i686-unknown-linux-gnu --emit=llvm-ir $(SRC)
[ "$$(grep -c 'align 1' "$(LL)")" -eq "3" ]
endif
# SystemZ requires even alignment for PC-relative addressing.
ifeq ($(filter systemz,$(LLVM_COMPONENTS)),systemz)
$(RUSTC) --target=s390x-unknown-linux-gnu --emit=llvm-ir $(SRC)
[ "$$(grep -c 'align 2' "$(LL)")" -eq "3" ]
endif
endif
38 changes: 38 additions & 0 deletions src/test/run-make/min-global-align/min_global_align.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(no_core, lang_items)]
#![crate_type="rlib"]
#![no_core]

pub static STATIC_BOOL: bool = true;

pub static mut STATIC_MUT_BOOL: bool = true;

const CONST_BOOL: bool = true;
pub static CONST_BOOL_REF: &'static bool = &CONST_BOOL;


#[lang = "sized"]
trait Sized {}

#[lang = "copy"]
trait Copy {}

#[lang = "freeze"]
trait Freeze {}

#[lang = "sync"]
trait Sync {}
impl Sync for bool {}
impl Sync for &'static bool {}

#[lang="drop_in_place"]
pub unsafe fn drop_in_place<T: ?Sized>(_: *mut T) { }