From 70f627c7ff5c52dc48fd37333ac78c0fd05f5338 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Wed, 27 Jan 2021 22:55:29 -0500 Subject: [PATCH 1/7] Avoid possible compiler crash and duplicate labels errors in asm! blocks --- compiler/rustc_codegen_llvm/src/asm.rs | 45 +++++++++++++++++++++++++- src/test/ui/asm/duplicate-labels.rs | 16 +++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/asm/duplicate-labels.rs diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 8801211d51bc3..ca75d769a0d83 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -198,8 +198,37 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } + /// Gets a LLVM label from a &str if it exists + /// Uses this as reference: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols + fn get_label(s: &str) -> Option<&str> { + if let Some(colon_idx) = s.find(':') { + let substr = &s[..colon_idx]; + let mut chars = substr.chars(); + if let Some(c) = chars.next() { + // First char is [a-zA-Z_.] + if ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c + { + // All subsequent chars are [a-zA-Z0-9_$.@] + if chars.all(|c| { + ('a'..='z').contains(&c) + || ('A'..='Z').contains(&c) + || '_' == c + || '$' == c + || '.' == c + || '@' == c + }) { + return Some(substr); + } + } + } + } + + None + } + // Build the template string let mut template_str = String::new(); + let mut labels = vec![]; for piece in template { match *piece { InlineAsmTemplatePiece::String(ref s) => { @@ -212,7 +241,21 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } } else { - template_str.push_str(s) + let mut ts = s.to_owned(); + // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels + // Fixes https://github.com/rust-lang/rust/issues/74262 + if let Some(label) = get_label(&ts) { + labels.push(label.to_owned()); + } + + for label in &labels { + ts = ts.replace( + label, + format!("${{:private}}{}${{:uid}}", label).as_ref(), + ); + } + + template_str.push_str(&ts) } } InlineAsmTemplatePiece::Placeholder { operand_idx, modifier, span: _ } => { diff --git a/src/test/ui/asm/duplicate-labels.rs b/src/test/ui/asm/duplicate-labels.rs new file mode 100644 index 0000000000000..85f156b873a81 --- /dev/null +++ b/src/test/ui/asm/duplicate-labels.rs @@ -0,0 +1,16 @@ +// compile-flags: -g +// build-pass + +#![feature(asm)] + +#[inline(always)] +fn asm() { + unsafe { + asm!("duplabel: nop",); + } +} + +fn main() { + asm(); + asm(); +} From 4bb2d5de64bb1fe94fdcb4a0dfe7d5ba151df718 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Wed, 27 Jan 2021 23:23:46 -0500 Subject: [PATCH 2/7] Move get_llvm_label_from_str out of codegen_inline_asm --- compiler/rustc_codegen_llvm/src/asm.rs | 57 +++++++++++++------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index ca75d769a0d83..67822e01bd61f 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -198,34 +198,6 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } - /// Gets a LLVM label from a &str if it exists - /// Uses this as reference: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols - fn get_label(s: &str) -> Option<&str> { - if let Some(colon_idx) = s.find(':') { - let substr = &s[..colon_idx]; - let mut chars = substr.chars(); - if let Some(c) = chars.next() { - // First char is [a-zA-Z_.] - if ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c - { - // All subsequent chars are [a-zA-Z0-9_$.@] - if chars.all(|c| { - ('a'..='z').contains(&c) - || ('A'..='Z').contains(&c) - || '_' == c - || '$' == c - || '.' == c - || '@' == c - }) { - return Some(substr); - } - } - } - } - - None - } - // Build the template string let mut template_str = String::new(); let mut labels = vec![]; @@ -244,7 +216,7 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { let mut ts = s.to_owned(); // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels // Fixes https://github.com/rust-lang/rust/issues/74262 - if let Some(label) = get_label(&ts) { + if let Some(label) = get_llvm_label_from_str(&ts) { labels.push(label.to_owned()); } @@ -917,3 +889,30 @@ fn llvm_fixup_output_type( _ => layout.llvm_type(cx), } } + +/// Gets a LLVM label from a &str if it exists +/// Uses this as reference: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols +fn get_llvm_label_from_str(s: &str) -> Option<&str> { + if let Some(colon_idx) = s.find(':') { + let substr = &s[..colon_idx]; + let mut chars = substr.chars(); + if let Some(c) = chars.next() { + // First char is [a-zA-Z_.] + if ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c { + // All subsequent chars are [a-zA-Z0-9_$.@] + if chars.all(|c| { + ('a'..='z').contains(&c) + || ('A'..='Z').contains(&c) + || '_' == c + || '$' == c + || '.' == c + || '@' == c + }) { + return Some(substr); + } + } + } + } + + None +} From bce67de5308a0a488fcaf98a12f7ff8fd7d3bf97 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Wed, 27 Jan 2021 23:27:53 -0500 Subject: [PATCH 3/7] Add explicit inline and simple duplication tests --- src/test/ui/asm/duplicate-labels-inline.rs | 16 ++++++++++++++++ src/test/ui/asm/duplicate-labels.rs | 13 +++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/asm/duplicate-labels-inline.rs diff --git a/src/test/ui/asm/duplicate-labels-inline.rs b/src/test/ui/asm/duplicate-labels-inline.rs new file mode 100644 index 0000000000000..85f156b873a81 --- /dev/null +++ b/src/test/ui/asm/duplicate-labels-inline.rs @@ -0,0 +1,16 @@ +// compile-flags: -g +// build-pass + +#![feature(asm)] + +#[inline(always)] +fn asm() { + unsafe { + asm!("duplabel: nop",); + } +} + +fn main() { + asm(); + asm(); +} diff --git a/src/test/ui/asm/duplicate-labels.rs b/src/test/ui/asm/duplicate-labels.rs index 85f156b873a81..67a6664afa6c6 100644 --- a/src/test/ui/asm/duplicate-labels.rs +++ b/src/test/ui/asm/duplicate-labels.rs @@ -3,14 +3,19 @@ #![feature(asm)] -#[inline(always)] -fn asm() { +fn asm1() { unsafe { asm!("duplabel: nop",); } } +fn asm2() { + unsafe { + asm!("nop", "duplabel: nop",); + } +} + fn main() { - asm(); - asm(); + asm1(); + asm2(); } From e78a1bf5f55cc3805d2e58a35448847a8fab4589 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Thu, 28 Jan 2021 12:15:17 -0500 Subject: [PATCH 4/7] Handle label declared after usage --- compiler/rustc_codegen_llvm/src/asm.rs | 14 ++++++-------- src/test/ui/asm/label-use-before-declaration.rs | 10 ++++++++++ 2 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 src/test/ui/asm/label-use-before-declaration.rs diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 67822e01bd61f..44a77ecf6841e 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -213,20 +213,13 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } } else { - let mut ts = s.to_owned(); + let ts = s.to_owned(); // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels // Fixes https://github.com/rust-lang/rust/issues/74262 if let Some(label) = get_llvm_label_from_str(&ts) { labels.push(label.to_owned()); } - for label in &labels { - ts = ts.replace( - label, - format!("${{:private}}{}${{:uid}}", label).as_ref(), - ); - } - template_str.push_str(&ts) } } @@ -259,6 +252,11 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } + for label in &labels { + template_str = + template_str.replace(label, format!("${{:private}}{}${{:uid}}", label).as_ref()); + } + if !options.contains(InlineAsmOptions::PRESERVES_FLAGS) { match asm_arch { InlineAsmArch::AArch64 | InlineAsmArch::Arm => { diff --git a/src/test/ui/asm/label-use-before-declaration.rs b/src/test/ui/asm/label-use-before-declaration.rs new file mode 100644 index 0000000000000..2205acec63af0 --- /dev/null +++ b/src/test/ui/asm/label-use-before-declaration.rs @@ -0,0 +1,10 @@ +// compile-flags: -g +// build-pass + +#![feature(asm)] + +fn main() { + unsafe { + asm!("jmp l", "nop", "l: nop"); + } +} From b72835f6fbfffa026ac3d8ff5af0d50fb26dc602 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Sat, 30 Jan 2021 14:55:03 -0500 Subject: [PATCH 5/7] Make it only replace independent identifiers that are labels --- compiler/rustc_codegen_llvm/src/asm.rs | 40 +++++++++++++++++---- src/test/ui/asm/label-substr-instruction.rs | 10 ++++++ src/test/ui/asm/label-substr-label.rs | 10 ++++++ 3 files changed, 54 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/asm/label-substr-instruction.rs create mode 100644 src/test/ui/asm/label-substr-label.rs diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 44a77ecf6841e..8f95b9348f40b 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -213,14 +213,13 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } } else { - let ts = s.to_owned(); // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels // Fixes https://github.com/rust-lang/rust/issues/74262 - if let Some(label) = get_llvm_label_from_str(&ts) { + if let Some(label) = get_llvm_label_from_str(s) { labels.push(label.to_owned()); } - template_str.push_str(&ts) + template_str.push_str(s) } } InlineAsmTemplatePiece::Placeholder { operand_idx, modifier, span: _ } => { @@ -252,9 +251,37 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } - for label in &labels { - template_str = - template_str.replace(label, format!("${{:private}}{}${{:uid}}", label).as_ref()); + fn valid_ident_continuation(c: char) -> bool { + ('a'..='z').contains(&c) + || ('A'..='Z').contains(&c) + || ('0'..='9').contains(&c) + || '_' == c + || '$' == c + || '.' == c + || '@' == c + } + + for label in labels.iter() { + let indices: Vec<_> = template_str.match_indices(label).map(|(idx, _)| idx).collect(); + let ranges: Vec<_> = indices + .iter() + .filter_map(|&idx| { + let label_end_idx = idx + label.len(); + let next_char = template_str[label_end_idx..].chars().nth(0); + if next_char.is_none() || !valid_ident_continuation(next_char.unwrap()) { + Some(idx..label_end_idx) + } else { + None + } + }) + .collect(); + + // We add a constant length of 18 to the string every time + // from ${:private} and ${:uid} + for (i, range) in ranges.iter().enumerate() { + let real_range = range.start + (i * 18)..range.end + (i * 18); + template_str.replace_range(real_range, &format!("${{:private}}{}${{:uid}}", label)) + } } if !options.contains(InlineAsmOptions::PRESERVES_FLAGS) { @@ -901,6 +928,7 @@ fn get_llvm_label_from_str(s: &str) -> Option<&str> { if chars.all(|c| { ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) + || ('0'..='9').contains(&c) || '_' == c || '$' == c || '.' == c diff --git a/src/test/ui/asm/label-substr-instruction.rs b/src/test/ui/asm/label-substr-instruction.rs new file mode 100644 index 0000000000000..82efd6fe7cd4a --- /dev/null +++ b/src/test/ui/asm/label-substr-instruction.rs @@ -0,0 +1,10 @@ +// compile-flags: -g +// build-pass + +#![feature(asm)] + +fn main() { + unsafe { + asm!("jmp j", "nop", "j: nop"); + } +} diff --git a/src/test/ui/asm/label-substr-label.rs b/src/test/ui/asm/label-substr-label.rs new file mode 100644 index 0000000000000..cd20ccbefa845 --- /dev/null +++ b/src/test/ui/asm/label-substr-label.rs @@ -0,0 +1,10 @@ +// compile-flags: -g +// build-pass + +#![feature(asm)] + +fn main() { + unsafe { + asm!("jmp l", "l: nop", "jmp l2", "l2: nop"); + } +} From be00fa4f445542bff5f78c6f971f033f5473da7b Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Sat, 30 Jan 2021 17:13:58 -0500 Subject: [PATCH 6/7] Better use of iterators and filter_map --- compiler/rustc_codegen_llvm/src/asm.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 8f95b9348f40b..592fb32b12e00 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -262,10 +262,9 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } for label in labels.iter() { - let indices: Vec<_> = template_str.match_indices(label).map(|(idx, _)| idx).collect(); - let ranges: Vec<_> = indices - .iter() - .filter_map(|&idx| { + let ranges: Vec<_> = template_str + .match_indices(label) + .filter_map(|(idx, _)| { let label_end_idx = idx + label.len(); let next_char = template_str[label_end_idx..].chars().nth(0); if next_char.is_none() || !valid_ident_continuation(next_char.unwrap()) { From 1e27711bec2e16f86b773b36457884883e40ebe9 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Sat, 30 Jan 2021 17:43:39 -0500 Subject: [PATCH 7/7] Better organize llvm identifier code --- compiler/rustc_codegen_llvm/src/asm.rs | 50 ++++++++++++-------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/asm.rs b/compiler/rustc_codegen_llvm/src/asm.rs index 592fb32b12e00..b62d4a2c2ca9a 100644 --- a/compiler/rustc_codegen_llvm/src/asm.rs +++ b/compiler/rustc_codegen_llvm/src/asm.rs @@ -213,8 +213,6 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } } else { - // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels - // Fixes https://github.com/rust-lang/rust/issues/74262 if let Some(label) = get_llvm_label_from_str(s) { labels.push(label.to_owned()); } @@ -251,23 +249,15 @@ impl AsmBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } - fn valid_ident_continuation(c: char) -> bool { - ('a'..='z').contains(&c) - || ('A'..='Z').contains(&c) - || ('0'..='9').contains(&c) - || '_' == c - || '$' == c - || '.' == c - || '@' == c - } - + // Labels should be made local to prevent issues with inlined `asm!` and duplicate labels + // Fixes https://github.com/rust-lang/rust/issues/74262 for label in labels.iter() { let ranges: Vec<_> = template_str .match_indices(label) .filter_map(|(idx, _)| { let label_end_idx = idx + label.len(); let next_char = template_str[label_end_idx..].chars().nth(0); - if next_char.is_none() || !valid_ident_continuation(next_char.unwrap()) { + if next_char.is_none() || !llvm_ident_continuation(next_char.unwrap()) { Some(idx..label_end_idx) } else { None @@ -914,27 +904,31 @@ fn llvm_fixup_output_type( } } +// Valid llvm symbols: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols +// First char is [a-zA-Z_.] +fn llvm_ident_start(c: char) -> bool { + ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c +} + +// All subsequent chars are [a-zA-Z0-9_$.@] +fn llvm_ident_continuation(c: char) -> bool { + ('a'..='z').contains(&c) + || ('A'..='Z').contains(&c) + || ('0'..='9').contains(&c) + || '_' == c + || '$' == c + || '.' == c + || '@' == c +} + /// Gets a LLVM label from a &str if it exists -/// Uses this as reference: https://llvm.org/docs/AMDGPUOperandSyntax.html#symbols fn get_llvm_label_from_str(s: &str) -> Option<&str> { if let Some(colon_idx) = s.find(':') { let substr = &s[..colon_idx]; let mut chars = substr.chars(); if let Some(c) = chars.next() { - // First char is [a-zA-Z_.] - if ('a'..='z').contains(&c) || ('A'..='Z').contains(&c) || '_' == c || '.' == c { - // All subsequent chars are [a-zA-Z0-9_$.@] - if chars.all(|c| { - ('a'..='z').contains(&c) - || ('A'..='Z').contains(&c) - || ('0'..='9').contains(&c) - || '_' == c - || '$' == c - || '.' == c - || '@' == c - }) { - return Some(substr); - } + if llvm_ident_start(c) && chars.all(llvm_ident_continuation) { + return Some(substr); } } }