fix: Convert stackwalking/cfi loading into a fixpoint iteration #450
Conversation
|
Still to do: |
|
While updating the snapshots, I noticed the following: The CFI status is being changed from I think in this case, avoiding the false negatives (previously being marked as |
|
False positive: |
There was a problem hiding this comment.
I need another pass over the logic of how to collect statusses i'm afraid.
The CfiCacheModules went from something that was purely used to build a report post-hoc to something that now is merging that functionality with also providing and adding info during the fixpoint iterations. Which makes things more complicated? Haven't figured out yet if this can be simpler and more separated yet.
...r/src/services/snapshots/symbolicator__services__symbolication__tests__minidump_windows.snap
Outdated
Show resolved
Hide resolved
...r/src/services/snapshots/symbolicator__services__symbolication__tests__minidump_windows.snap
Outdated
Show resolved
Hide resolved
| // make sure to hold onto a reference to the CfiCacheFile, to make sure | ||
| // it will not be evicted in the middle of using it |
There was a problem hiding this comment.
Why does it suddenly become this cache's job to keep these files alive?
There was a problem hiding this comment.
Previously, they were moved into the stackwalking method, and that had an explicit drop at the end to keep the cache alive. This just moves this around a bit.
There was a problem hiding this comment.
nit: maybe move this to the doc comment on this entry in the struct definition.
|
|
||
| /// Returns a copy of the inner Module Map that can be used for processing. | ||
| fn for_processing(&self) -> BTreeMap<CodeModuleId, CfiModule> { | ||
| self.inner.clone() |
There was a problem hiding this comment.
cloning a BTreeMap sounds expensive?
There was a problem hiding this comment.
This is indeed unfortunate. Previously, the CfiCacheModules (which internally was a vec of tuples) would be serialized and sent via procspawn. Now it is the btreemap, so not too different actually.
| // get_referenced_modules nor marked as scanned during | ||
| // stackwalking, it can only be unused. | ||
| obj_info.unwind_status = Some(ObjectFileStatus::Unused); | ||
| obj_info.unwind_status = None; |
There was a problem hiding this comment.
I kind-of piggyback on the None to raising errors about files being missing.
| // make sure to hold onto a reference to the CfiCacheFile, to make sure | ||
| // it will not be evicted in the middle of using it |
There was a problem hiding this comment.
nit: maybe move this to the doc comment on this entry in the struct definition.
|
which bot are we talking about? |
Instead of two times, it will now do up to 5 stack walks, trying to load increasing CFI on each iteration.