From dbe96bce0ca4a699423b2de2f2fa5482af006dc8 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 17 Dec 2024 14:58:53 +0100 Subject: [PATCH 1/4] fix(symcache): Use saturating addition in writer Addition wraps in release mode. For large line size values, this resulted in ranges whose end was before their start, which caused us to skip a function's inlinees in some cases. Instead of using bare addition on `u64` and casting to `u32` at the end, we now use saturating addition on `u32` directly. --- symbolic-symcache/src/writer.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/symbolic-symcache/src/writer.rs b/symbolic-symcache/src/writer.rs index 7b0775b7d..2249520f7 100644 --- a/symbolic-symcache/src/writer.rs +++ b/symbolic-symcache/src/writer.rs @@ -189,7 +189,7 @@ impl<'a> SymCacheConverter<'a> { for inlinee in &function.inlinees { for line in &inlinee.lines { let start = line.address as u32; - let end = (line.address + line.size.unwrap_or(1)) as u32; + let end = start.saturating_add(line.size.unwrap_or(1) as u32); inlinee_ranges.push(start..end); } } @@ -214,7 +214,7 @@ impl<'a> SymCacheConverter<'a> { // Iterate over the line records. while let Some(line) = next_line.take() { let line_range_start = line.address as u32; - let line_range_end = (line.address + line.size.unwrap_or(1)) as u32; + let line_range_end = line_range_start.saturating_add(line.size.unwrap_or(1) as u32); // Find the call location for this line. while next_call_location.is_some() && next_call_location.unwrap().0 <= line_range_start @@ -308,16 +308,18 @@ impl<'a> SymCacheConverter<'a> { let mut current_address = line_range_start; while current_address < line_range_end { // Emit our source location at current_address if current_address is not covered by an inlinee. - if next_inline.is_none() || next_inline.as_ref().unwrap().start > current_address { + if next_inline + .as_ref() + .is_none_or(|next| next.start > current_address) + { // "insert_range" self.ranges.insert(current_address, source_location.clone()); } // If there is an inlinee range covered by this line record, turn this line into that // call's "call line". Make a `call_location_idx` for it and store it in `callee_call_locations`. - if next_inline.is_some() && next_inline.as_ref().unwrap().start < line_range_end { - let inline_range = next_inline.take().unwrap(); - + if let Some(inline_range) = next_inline.take_if(|next| next.start < line_range_end) + { // "make_call_location" let (call_location_idx, _) = self.call_locations.insert_full(source_location.clone()); @@ -341,8 +343,9 @@ impl<'a> SymCacheConverter<'a> { // multiple identical small "call line" records instead of one combined record // covering the entire inlinee range. We can't have different "call lines" for a single // inlinee range anyway, so it's fine to skip these. - while next_line.is_some() - && (next_line.as_ref().unwrap().address as u32) < current_address + while next_line + .as_ref() + .is_some_and(|next| (next.address as u32) < current_address) { next_line = line_iter.next(); } From 0159cb597b44621d5b892abba1f55b54a99dbfa3 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 18 Dec 2024 13:25:12 +0100 Subject: [PATCH 2/4] Use try_into --- symbolic-symcache/src/writer.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/symbolic-symcache/src/writer.rs b/symbolic-symcache/src/writer.rs index 2249520f7..b4bc03e6d 100644 --- a/symbolic-symcache/src/writer.rs +++ b/symbolic-symcache/src/writer.rs @@ -188,8 +188,9 @@ impl<'a> SymCacheConverter<'a> { let mut inlinee_ranges = Vec::new(); for inlinee in &function.inlinees { for line in &inlinee.lines { - let start = line.address as u32; - let end = start.saturating_add(line.size.unwrap_or(1) as u32); + let start = line.address.try_into().unwrap_or(u32::MAX); + let end = + start.saturating_add(line.size.unwrap_or(1).try_into().unwrap_or(u32::MAX)); inlinee_ranges.push(start..end); } } @@ -213,8 +214,9 @@ impl<'a> SymCacheConverter<'a> { // Iterate over the line records. while let Some(line) = next_line.take() { - let line_range_start = line.address as u32; - let line_range_end = line_range_start.saturating_add(line.size.unwrap_or(1) as u32); + let line_range_start = line.address.try_into().unwrap_or(u32::MAX); + let line_range_end = line_range_start + .saturating_add(line.size.unwrap_or(1).try_into().unwrap_or(u32::MAX)); // Find the call location for this line. while next_call_location.is_some() && next_call_location.unwrap().0 <= line_range_start From 6d1feb64fe5f9e425ced466bc3e2f6904049af02 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 18 Dec 2024 13:31:15 +0100 Subject: [PATCH 3/4] changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 365f9d504..189825777 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Unreleased + +**Fixes** +- symcache: Fixed a bug related to to inlinee resolution during symcache conversion. ([#883](https://github.com/getsentry/symbolic/pull/883)) + ## 12.12.3 **Fixes** From 161d7dd4354f242f3b3aaaeaf93844ad23377fc7 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 18 Dec 2024 13:59:59 +0100 Subject: [PATCH 4/4] Extract logic to function and add test --- symbolic-symcache/src/writer.rs | 45 ++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/symbolic-symcache/src/writer.rs b/symbolic-symcache/src/writer.rs index b4bc03e6d..e6c95c4c9 100644 --- a/symbolic-symcache/src/writer.rs +++ b/symbolic-symcache/src/writer.rs @@ -188,9 +188,7 @@ impl<'a> SymCacheConverter<'a> { let mut inlinee_ranges = Vec::new(); for inlinee in &function.inlinees { for line in &inlinee.lines { - let start = line.address.try_into().unwrap_or(u32::MAX); - let end = - start.saturating_add(line.size.unwrap_or(1).try_into().unwrap_or(u32::MAX)); + let (start, end) = line_boundaries(line.address, line.size); inlinee_ranges.push(start..end); } } @@ -214,9 +212,7 @@ impl<'a> SymCacheConverter<'a> { // Iterate over the line records. while let Some(line) = next_line.take() { - let line_range_start = line.address.try_into().unwrap_or(u32::MAX); - let line_range_end = line_range_start - .saturating_add(line.size.unwrap_or(1).try_into().unwrap_or(u32::MAX)); + let (line_range_start, line_range_end) = line_boundaries(line.address, line.size); // Find the call location for this line. while next_call_location.is_some() && next_call_location.unwrap().0 <= line_range_start @@ -555,3 +551,40 @@ fn undecorate_win_symbol(name: &str) -> &str { name } + +/// Returns the start and end address for a line record, clamped to `u32`. +fn line_boundaries(address: u64, size: Option) -> (u32, u32) { + let start = address.try_into().unwrap_or(u32::MAX); + let end = start.saturating_add(size.unwrap_or(1).try_into().unwrap_or(u32::MAX)); + (start, end) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Tests that computing a range with a large size naively + /// results in an empty range, but using `line_boundaries` + /// doesn't. + #[test] + fn test_large_range() { + // Line record values from an actual example + let address = 0x11d255; + let size = 0xffee9d55; + + let naive_range = { + let start = address as u32; + let end = (address + size) as u32; + start..end + }; + + assert!(naive_range.is_empty()); + + let range = { + let (start, end) = line_boundaries(address, Some(size)); + start..end + }; + + assert!(!range.is_empty()); + } +}