Skip to content
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

rustc_codegen_llvm: give names to non-alloca variable values. #64149

Merged
merged 1 commit into from
Sep 7, 2019
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
35 changes: 32 additions & 3 deletions src/librustc_codegen_llvm/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use rustc_codegen_ssa::debuginfo::{FunctionDebugContext, MirDebugScope, Variable

use libc::c_uint;
use std::cell::RefCell;
use std::ffi::CString;
use std::ffi::{CStr, CString};

use syntax_pos::{self, Span, Pos};
use syntax::ast;
Expand Down Expand Up @@ -224,8 +224,37 @@ impl DebugInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> {
gdb::insert_reference_to_gdb_debug_scripts_section_global(self)
}

fn set_value_name(&mut self, value: &'ll Value, name: &str) {
let cname = SmallCStr::new(name);
fn set_var_name(&mut self, value: &'ll Value, name: impl ToString) {
// Avoid wasting time if LLVM value names aren't even enabled.
if self.sess().fewer_names() {
return;
}

// Only function parameters and instructions are local to a function,
// don't change the name of anything else (e.g. globals).
let param_or_inst = unsafe {
llvm::LLVMIsAArgument(value).is_some() ||
llvm::LLVMIsAInstruction(value).is_some()
};
if !param_or_inst {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

My first instinct would be to make this a bug!(), not a silent nop. Are there any callers (now or after more debuginfo work) that might good reason need to pass in something other than an instruction or argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yeah, Constants (which would be a nop), or, worse, GlobalValues (functions/statics), which would get their mangled names replaced.

That is, I'm pretty sure let x = foo as fn(); and let y = &STATIC; would both allow accidental renaming of foo's or STATIC's symbol names without this return.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense. Well, silent nop it is then.

}

let old_name = unsafe {
CStr::from_ptr(llvm::LLVMGetValueName(value))
};
match old_name.to_str() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was gonna suggest writing this match as if old_name.is_empty() for readability (+ to avoid having to care about UTF-8) but actually CStr doesn't have that 🤷‍♀

FYI while annoyed about this I also noticed that we can use better APIs for touching LLVM value names (see #64223) but that's unrelated to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes I forgot to bring that up! I also noticed those functions.

Ok("") => {}
Ok(_) => {
// Avoid replacing the name if it already exists.
// While we could combine the names somehow, it'd
// get noisy quick, and the usefulness is dubious.
return;
}
Err(_) => return,
}

let cname = CString::new(name.to_string()).unwrap();
unsafe {
llvm::LLVMSetValueName(value, cname.as_ptr());
}
Expand Down
2 changes: 2 additions & 0 deletions src/librustc_codegen_llvm/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,7 @@ extern "C" {
pub fn LLVMRustRemoveFunctionAttributes(Fn: &Value, index: c_uint, attr: Attribute);

// Operations on parameters
pub fn LLVMIsAArgument(Val: &Value) -> Option<&Value>;
pub fn LLVMCountParams(Fn: &Value) -> c_uint;
pub fn LLVMGetParam(Fn: &Value, Index: c_uint) -> &Value;

Expand All @@ -818,6 +819,7 @@ extern "C" {
pub fn LLVMDeleteBasicBlock(BB: &BasicBlock);

// Operations on instructions
pub fn LLVMIsAInstruction(Val: &Value) -> Option<&Value>;
pub fn LLVMGetFirstBasicBlock(Fn: &Value) -> &BasicBlock;

// Operations on call sites
Expand Down
16 changes: 8 additions & 8 deletions src/librustc_codegen_ssa/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,19 +518,19 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
PassMode::Ignore(IgnoreMode::CVarArgs) => {}
PassMode::Direct(_) => {
let llarg = bx.get_param(llarg_idx);
bx.set_value_name(llarg, &name);
bx.set_var_name(llarg, &name);
llarg_idx += 1;
return local(
OperandRef::from_immediate_or_packed_pair(bx, llarg, arg.layout));
}
PassMode::Pair(..) => {
let a = bx.get_param(llarg_idx);
bx.set_value_name(a, &(name.clone() + ".0"));
llarg_idx += 1;
let (a, b) = (bx.get_param(llarg_idx), bx.get_param(llarg_idx + 1));
llarg_idx += 2;

let b = bx.get_param(llarg_idx);
bx.set_value_name(b, &(name + ".1"));
llarg_idx += 1;
// FIXME(eddyb) these are scalar components,
// maybe extract the high-level fields?
bx.set_var_name(a, format_args!("{}.0", name));
bx.set_var_name(b, format_args!("{}.1", name));

return local(OperandRef {
val: OperandValue::Pair(a, b),
Expand All @@ -546,7 +546,7 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
// already put it in a temporary alloca and gave it up.
// FIXME: lifetimes
let llarg = bx.get_param(llarg_idx);
bx.set_value_name(llarg, &name);
bx.set_var_name(llarg, &name);
llarg_idx += 1;
PlaceRef::new_sized(llarg, arg.layout)
} else if arg.is_unsized_indirect() {
Expand Down
16 changes: 15 additions & 1 deletion src/librustc_codegen_ssa/mir/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,21 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
self.codegen_rvalue_unsized(bx, cg_indirect_dest, rvalue)
}
LocalRef::Operand(None) => {
let (bx, operand) = self.codegen_rvalue_operand(bx, rvalue);
let (mut bx, operand) = self.codegen_rvalue_operand(bx, rvalue);
if let Some(name) = self.mir.local_decls[index].name {
match operand.val {
OperandValue::Ref(x, ..) |
OperandValue::Immediate(x) => {
bx.set_var_name(x, name);
}
OperandValue::Pair(a, b) => {
// FIXME(eddyb) these are scalar components,
// maybe extract the high-level fields?
bx.set_var_name(a, format_args!("{}.0", name));
bx.set_var_name(b, format_args!("{}.1", name));
}
}
}
self.locals[index] = LocalRef::Operand(Some(operand));
bx
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_ssa/traits/debuginfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,5 @@ pub trait DebugInfoBuilderMethods<'tcx>: BackendTypes {
span: Span,
);
fn insert_reference_to_gdb_debug_scripts_section_global(&mut self);
fn set_value_name(&mut self, value: Self::Value, name: &str);
fn set_var_name(&mut self, value: Self::Value, name: impl ToString);
}
15 changes: 15 additions & 0 deletions src/test/codegen/var-names.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// compile-flags: -O -C no-prepopulate-passes

#![crate_type = "lib"]

// CHECK-LABEL: define i32 @test(i32 %a, i32 %b)
#[no_mangle]
pub fn test(a: u32, b: u32) -> u32 {
let c = a + b;
// CHECK: %c = add i32 %a, %b
let d = c;
let e = d * a;
// CHECK-NEXT: %e = mul i32 %c, %a
e
// CHECK-NEXT: ret i32 %e
}