You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Move SuperPMI and ilasm/ildasm off PAL file APIs - #136939
Split out of #136821 so these tool changes can be merged first; the PAL APIs themselves are removed there.
SuperPMI (superpmi, mcs, shim collector): use stdio FILE* instead of the PAL CreateFile/ReadFile/WriteFile/SetFilePointer/GetFileSize and file-mapping APIs; resolve the JIT path with realpath on Unix.
ilasm, ildasm and metainfo: map files with VMToOSInterface, use minimal Unix path handling instead of GetFullPathName/SearchPathW, and drop the unused console handles.
- SuperPMI (superpmi, mcs, shim collector): use stdio FILE* instead of
the PAL CreateFile/ReadFile/WriteFile/SetFilePointer/GetFileSize and
file-mapping APIs; resolve the JIT path with realpath on Unix.
- ilasm, ildasm and metainfo: map files with VMToOSInterface, use minimal
Unix path handling instead of GetFullPathName/SearchPathW, and drop the
unused console handles.
Co-authored-by: Cursor <cursoragent@cursor.com>
Azure Pipelines:
Successfully started running 4 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unresolved critical issues can crash the host when collection-file opening fails and truncate source files when input and output paths alias on Windows.
- mcs: refuse to copy/strip/stat/removeDup a file into itself.
- Shim collector: skip writes when the collection file failed to open.
- WriteArrayToMCL: report a failed fclose.
- VMToOSInterface::OpenFile: let ilasm, ildasm and metainfo open files
without requesting execute access on Windows.
Co-authored-by: Cursor <cursoragent@cursor.com>
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Fracture can destroy its input collection on Windows when a generated output aliases it, and shared file helpers still break replay of MCH files larger than 2 GiB on 32-bit Unix.
ftell/fseek take long, which is 32-bit on 32-bit Unix; use ftello/fseeko so files over 2 GiB work there. Also reject a failed size query in MethodContextReader.
Co-authored-by: Cursor <cursoragent@cursor.com>
mcs -concat x.mch x.mch (including hard-link aliases) is now allowed on Windows, where the previous exclusive handle rejected it. When the original file exceeds BUFFER_SIZE and has a partial final block, later reads can consume bytes appended by earlier iterations, appending more than the original input. Cap each read at the remaining captured input size, or reject same-file aliases before copying.
The reason will be displayed to describe this comment to others. Learn more.
I think this will make superpmi memory usage scale with the size of the input collection which isn't great.
FileLineReader is only used by parallel replay to merge the per-worker details/metrics CSVs (one file at a time), not to read the collection itself. But fair point, there's no need to buffer the whole file since AdvanceLine copies each line out anyway.
The reason will be displayed to describe this comment to others. Learn more.
Those files scale with input collection size (by some factor smaller than 1). The details CSVs can be many hundreds of megabytes in size. If there is a simple way to keep this as a mapping it would be nice.
The reason will be displayed to describe this comment to others. Learn more.
#136984 uses C# read lines without loading whole file in the memory. If we are going forward with C# version, we can skip it (this is just in case; currently CI is not blocked by it).
The reason will be displayed to describe this comment to others. Learn more.
getline should work too, and it looks like it would be a pretty straightforward replacement to the current use pattern.
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
area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member
5 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.
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
jkoritzinsky
left a comment
jkotas
left a comment
Split out of #136821 so these tool changes can be merged first; the PAL APIs themselves are removed there.
FILE*instead of the PALCreateFile/ReadFile/WriteFile/SetFilePointer/GetFileSizeand file-mapping APIs; resolve the JIT path withrealpathon Unix.VMToOSInterface, use minimal Unix path handling instead ofGetFullPathName/SearchPathW, and drop the unused console handles.