Skip to content

refactor svh #26629

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

Closed
wants to merge 5 commits into from
Closed
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
4 changes: 2 additions & 2 deletions src/librustc/metadata/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1998,12 +1998,12 @@ fn encode_crate_dep(rbml_w: &mut Encoder,
dep: decoder::CrateDep) {
rbml_w.start_tag(tag_crate_dep);
rbml_w.wr_tagged_str(tag_crate_dep_crate_name, &dep.name);
rbml_w.wr_tagged_str(tag_crate_dep_hash, dep.hash.as_str());
rbml_w.wr_tagged_str(tag_crate_dep_hash, &dep.hash.to_string());
rbml_w.end_tag();
}

fn encode_hash(rbml_w: &mut Encoder, hash: &Svh) {
rbml_w.wr_tagged_str(tag_crate_hash, hash.as_str());
rbml_w.wr_tagged_str(tag_crate_hash, &hash.to_string());
}

fn encode_crate_name(rbml_w: &mut Encoder, crate_name: &str) {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/metadata/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ impl<'a> Context<'a> {
info!("Rejecting via hash: expected {} got {}", *myhash, hash);
self.rejected_via_hash.push(CrateMismatch {
path: libpath.to_path_buf(),
got: myhash.as_str().to_string()
got: myhash.to_string()
});
false
} else {
Expand Down
43 changes: 24 additions & 19 deletions src/librustc_back/svh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,26 @@ use std::hash::{Hash, SipHasher, Hasher};
use syntax::ast;
use syntax::visit;

#[derive(Clone, PartialEq, Debug)]
#[derive(Clone, PartialEq, Eq, Debug)]
pub struct Svh {
hash: String,
raw: u64,
}

impl Svh {
pub fn new(hash: &str) -> Svh {
assert!(hash.len() == 16);
Svh { hash: hash.to_string() }
}
// Ideally we'd just reverse the nibbles on LE machines during to_string, unfortunately
// this would break the abi so I guess we're just doing this now.

let s = if cfg!(target_endian = "big") {
hash.to_string()
} else {
hash.chars().rev().collect()
};
Copy link
Member

Choose a reason for hiding this comment

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

This seems somewhat odd, and I don't understand how endianness helps here, so could you clarify what this is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the to_string() impl, it walks the low nibble to the high nibble constructing the string, the net result being that without this, Svh::new("deadbeefdeadbeef").to_string() returns feebdaedfeebdaed

Unfortunately, because it's flipped nibble wise not byte wise I can't just flip the bytes in the resulting u64, but on a big endian architecture this reversing shouldn't be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I understand that this is reversing the string, but what I don't understand is why it matters to have the same ordering across different architectures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right. It needs to maintain compatibility with what's emitted today to avoid breaking the abi (without flipping it on LE arches it will refuse to build a stage1, because when it goes to consume the libc and core crates it'll believe they're out of date). I believe it's safe to nop out on BE architectures since they're already in the right order.

Copy link
Member

Choose a reason for hiding this comment

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

We don't currently maintain ABI compatibility today, so that's not too much of a worry, and I feel like if we cross endianness platforms that this is likely to be the least of our problems (I'm also not 100% sure that this is a problem). Altogether I think it's fine to omit this.


pub fn as_str<'a>(&'a self) -> &'a str {
&self.hash
Svh {
raw: u64::from_str_radix(&s, 16).unwrap(),
}
}

pub fn calculate(metadata: &Vec<String>, krate: &ast::Crate) -> Svh {
Expand Down Expand Up @@ -100,25 +107,23 @@ impl Svh {
attr.node.value.hash(&mut state);
}

let hash = state.finish();
return Svh {
hash: (0..64).step_by(4).map(|i| hex(hash >> i)).collect()
};

fn hex(b: u64) -> char {
let b = (b & 0xf) as u8;
let b = match b {
0 ... 9 => '0' as u8 + b,
_ => 'a' as u8 + b - 10,
};
b as char
Svh {
raw: state.finish(),
}
}
}

impl Hash for Svh {
fn hash<H>(&self, state: &mut H) where H: Hasher {
// We have to hash a &str since that's what the old implementations did, and otherwise we
// break the abi
&self.to_string()[..].hash(state);
Copy link
Member

Choose a reason for hiding this comment

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

Why go through a string to implement a hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually possible this is not necessary any more, although I believe that the u64 and the String will hash differently. I believe this was a first guess at what was happening before when I hit the endianness bug above (My stage1 would refuse to compile anything that depended on libc or core because it believed they were out of date)

}
}

impl fmt::Display for Svh {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.pad(self.as_str())
f.pad(&self.to_string())
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is an infinitely recursive function now?

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 think it is, I have a patch on a local branch but it needs a rebase

}
}

Expand Down
3 changes: 1 addition & 2 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,7 @@ pub fn commit_date_str() -> Option<&'static str> {
option_env!("CFG_VER_DATE")
}

/// Prints version information and returns None on success or an error
/// message on panic.
/// Prints version information
pub fn version(binary: &str, matches: &getopts::Matches) {
let verbose = matches.opt_present("verbose");

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ fn symbol_hash<'tcx>(tcx: &ty::ctxt<'tcx>,
symbol_hasher.reset();
symbol_hasher.input_str(&link_meta.crate_name);
symbol_hasher.input_str("-");
symbol_hasher.input_str(link_meta.crate_hash.as_str());
symbol_hasher.input_str(&link_meta.crate_hash.to_string());
for meta in tcx.sess.crate_metadata.borrow().iter() {
symbol_hasher.input_str(&meta[..]);
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ impl<'tcx> TypeMap<'tcx> {
cx.sess().cstore.get_crate_hash(source_def_id.krate)
};

output.push_str(crate_hash.as_str());
output.push_str(&crate_hash.to_string());
output.push_str("/");
output.push_str(&format!("{:x}", def_id.node));

Expand Down
2 changes: 1 addition & 1 deletion src/rt/rust_try.ll
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
; When f(...) returns normally, the return value is null.
; When f(...) throws, the return value is a pointer to the caught exception object.

; See also: libstd/rt/unwind.rs
; See also: libstd/rt/unwind/mod.rs

define i8* @rust_try(void (i8*)* %f, i8* %env) {

Expand Down