-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
ref(symcache): FuncRecord::len must be nonzero #323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Agreed, this should be NonZeroU16
.
Please see my comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed the s as u16
conversion below. After that, this should be good to go.
symbolic-symcache/src/writer.rs
Outdated
0 => !0, | ||
s => s, | ||
0 => u16::MAX, | ||
s => s as u16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only noticing this now: This will not clamp to u16::MAX
and rather wrap around. Can you add the cmp::min
back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I made a mistake while testing this. I'll add it back.
symbolic-symcache/src/writer.rs
Outdated
|
||
// This unwrap cannot fail; if len is zero we have already panicked | ||
// or continued. | ||
let len = NonZeroU16::new(len).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This is functionally equivalent, but you can avoid the unwrap
if you write like this:
let len = match NonZeroU16::new(len) {
Some(len) => len,
None => continue,
};
This incorporates pull request getsentry/symbolic#323.
This incorporates pull request getsentry/symbolic#323.
* master: test: add similar-asserts' assert_eq (#333) release: 8.0.4 meta: Changelog for 8.0.4 build: Drop support for python 2.7 (#328) meta(vscode): Fix include paths for C++ (#331) doc(debuginfo): Add descriptions of records to breakpad.pest (#329) build: Replace virtualenv with venv doc(symcache): Symcache documentation fix(debuginfo): Support debug_addr indexes in DWARF functions (#326) fix(symcache): Fixed bug that caused functions to have len 0 (#324) ref(symcache): FuncRecord::len must be nonzero (#323) fix: Clippy 1.50 lints (#322) fix(symcache): Support lookup for public syms larger than 65k (#320) fix(symcache): Compute correct line offsets (#319) release: 8.0.3 meta: Changelog build: Update goblin to 0.3.1 (#318) fix(elf): Consider sections of type SHT_MIPS_DWARF (#317)
@jan-auer and I believe that there is no reason to consider functions of length 0 in
symcache
and all such functions occurring in a debug object should be discarded before building the cache. The type of thelen
field of theFuncRecord
struct is changed fromu16
toNonZeroU16
to enforce this invariant.