process: replace fileToMapping map with OpenELFMapping#1402
Draft
nsavoire wants to merge 1 commit into
Draft
Conversation
Replace the `systemProcess.fileToMapping` map (populated as a side effect of IterateMappings, then consulted by OpenELF) with an explicit `OpenELFMapping(*RawMapping)` method on the Process interface. The mapping is now passed directly, removing the temporal coupling between the two methods and the path-keyed lookup. In processmanager.newFrameMapping, a small `mappingELFOpener` adapter routes opens of the bound mapping's path to `OpenELFMapping` while forwarding everything else (e.g. .gnu_debuglink targets) to `OpenELF`. This also fixes some issues with `fileToMapping`: - The first iteration after process creation missed the map_files route because `sp.fileToMapping` was assigned only at the end of `IterateMappings` and subsequent iterations could use a stale RawMapping (from the previous iteration). - Avoid path-keyed lookup that could pick the wrong file in some very rare cases (open-telemetry#812).
fabled
reviewed
May 7, 2026
Contributor
fabled
left a comment
fabled
left a comment
There was a problem hiding this comment.
Thanks! This and other clean ups have been on my to-do list for too long. So making forward here is great!
But could this be done without the new method? See the comment.
| // OpenELFMapping opens a memory mapping as an ELF file. The mapping must | ||
| // originate from this Process (via IterateMappings), passing a foreign | ||
| // mapping has undefined results. | ||
| OpenELFMapping(*RawMapping) (*pfelf.File, error) |
Contributor
There was a problem hiding this comment.
Instead of adding this as new method, could there be helper that just calls OpenMappingFile and then calls pfelf.OpenELF.
I'd also like to get rid of the `OpenELF' method here.
After #1395 things should work good with this approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace the
systemProcess.fileToMappingmap (populated as a side effect of IterateMappings, then consulted by OpenELF) with an explicitOpenELFMapping(*RawMapping)method on the Process interface. The mapping is now passed directly, removing the temporal coupling between the two methods and the path-keyed lookup.In processmanager.newFrameMapping, a small
mappingELFOpeneradapter routes opens of the bound mapping's path toOpenELFMappingwhile forwarding everything else (e.g. .gnu_debuglink targets) toOpenELF.This also fixes some issues with
fileToMapping:sp.fileToMappingwas assigned only at the end ofIterateMappingsand subsequent iterations could use a stale RawMapping (from the previous iteration).