feat(debuginfo): Extract srcsrv data from PDB for file mapping#943
feat(debuginfo): Extract srcsrv data from PDB for file mapping#943
Conversation
|
loewenheim
left a comment
loewenheim
left a comment
There was a problem hiding this comment.
The approach of implementing the remapping as a transformer is fundamentally sound, but I believe the better approach is to integrate it into PdbDebugSession directly. This is for a few reasons:
- The transformer machinery is used in Symbolicator to incorporate information from supplementary files (BCSymbolMaps, il2cpp mapping files) into a symcache. These files are separate from the object file being converted to a symcache. By contrast, in this case, the information is already present in the object file itself.
- Symbolicator doesn't know about different object file types—it relies on
symbolicto convert any object file into a symcache. This information is available to Symbolicator in principle, i.e. it could pattern match on what kind of object file it's dealing with and extract the source server information from PDB objects.
mujacica
force-pushed
the
feat/perforce-srcsrv-support
branch
from
2bc5c37 to
3a14467
Compare
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>
mujacica
force-pushed
the
feat/perforce-srcsrv-support
branch
from
3a14467 to
a5afa5e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| let depot_path = var_map.get("var3")?; | ||
| let changelist = var_map.get("var4").map(String::as_str); |
There was a problem hiding this comment.
my only feedback here is that if its expected that the layout uses very specific variables to house the path & revision information, you'll want to likely validate that the revision (changelist here, but I know this means revision) is a valid number (if that positional value exists). Our current layout has these at position 2 & 3 respectively, which means older pdbs from builds before we switch this position will try to map the path as the revision.
Please clearly document this layout:
<Absolute Path>*<User Data, likely Perforce server name>*<Depot Path>*<File Revision>
There was a problem hiding this comment.
Hi @BillFreist-ANET , I've now documented this in the code itself. More user-facing documentation to follow.
Forgot do do this in the merge commit.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bcc01e1. Configure here.
| /// Returns the SRCSRV VCS integration name if available. | ||
| /// | ||
| /// This extracts the version control system identifier from the SRCSRV stream, | ||
| /// if present. Common values include "perforce", "tfs", "git", etc. | ||
| /// Returns `None` if no SRCSRV stream exists or if it cannot be parsed. | ||
| pub fn srcsrv_vcs_name(&self) -> Option<String> { | ||
| let mut pdb = self.pdb.write(); | ||
|
|
||
| // Try to open the "srcsrv" named stream | ||
| let stream = match pdb.named_stream(b"srcsrv") { | ||
| Ok(stream) => stream, | ||
| Err(_) => return None, | ||
| }; | ||
|
|
||
| // Parse the stream to extract VCS name | ||
| let stream_data = stream.as_slice(); | ||
| if let Ok(parsed_stream) = srcsrv::SrcSrvStream::parse(stream_data) { | ||
| parsed_stream | ||
| .version_control_description() | ||
| .map(|s| s.to_string()) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding this function in the first place was probably a mistake. It parses a SrcSrvStream, which constructs various indices and maps. That's really more appropriate for a function on the debug session.
Since the symcache version bump causes a major release anyway, we can remove it.
| .srcsrv | ||
| .as_deref() | ||
| .map(SourceServerMappings::parse) | ||
| .transpose()?; |
There was a problem hiding this comment.
The clankers are correct in that we could be more lenient here and not abort the entire debug_session if parsing the srcsrv stream fails since it's not essential. Any opinions on this?
There was a problem hiding this comment.
It feels very much optional, but it would also be nice if errors would be visible to the caller. Is it an option to parse it lazily instead of the constructor and propagate an error on use that is not fatal?
Overall I'd more lean towards swallowing the error, we wouldn't want PDBs with srcsrv information we just don't understand to fail, I think.
loewenheim
force-pushed
the
feat/perforce-srcsrv-support
branch
from
5b93649 to
4ef2e49
Compare
| .srcsrv | ||
| .as_deref() | ||
| .map(SourceServerMappings::parse) | ||
| .transpose()?; |
There was a problem hiding this comment.
It feels very much optional, but it would also be nice if errors would be visible to the caller. Is it an option to parse it lazily instead of the constructor and propagate an error on use that is not fatal?
Overall I'd more lean towards swallowing the error, we wouldn't want PDBs with srcsrv information we just don't understand to fail, I think.
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
Thanks for cleaning up the tests, I find it important that tests are written well, they do act as a bit of documentation and are there to help us in the future preventing regressions.
At the same time I really don't wanna be annoying about it, symbolic-symcache/tests/test_cache.rs could also use a pass, but it's also nbd to do this later/not at all.
4 participants
No description provided.