- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
ILC: Use temporary file to prevent invalid object files on compilation errors #121192
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
base: main
Are you sure you want to change the base?
Conversation
- Add HasLoggedErrors property to ILogWriter interface - Implement error tracking in TextLogWriter - Expose HasLoggedErrors through Logger class - Check for errors before writing object file in RyuJitCompilation and ReadyToRunCodegenCompilation Co-authored-by: agocke <[email protected]>
ReadyToRun/crossgen2 doesn't have --warnaserror option, so the fix only applies to ILC Co-authored-by: agocke <[email protected]>
Warnings treated as errors (MessageCategory.WarningAsError) now also set the _hasLoggedErrors flag, preventing object file generation Co-authored-by: agocke <[email protected]>
| @copilot what happens if warnings as errors are emitted during or after object writing? Do we already have such spots? | 
Move error check to delete file after all compilation steps complete, catching errors from typeSystemContext.LogWarnings() and other post-compilation warnings. Co-authored-by: MichalStrehovsky <[email protected]>
| 
 Good catch! There are indeed places where warnings can be logged after object writing: 
 To handle both scenarios, I've updated the implementation with a two-stage approach: 
 This ensures the object file is removed regardless of when errors occur during the compilation process. (commit f9940d1) | 
- Exit with non-zero code when errors are logged - Remove redundant file deletion from ObjectWriter.EmitObject since Program.Run handles it Co-authored-by: MichalStrehovsky <[email protected]>
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.
@copilot there is a build failure src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TrimmingTestLogger.cs(12,39): error CS0535: (NETCORE_ENGINEERING_TELEMETRY=Build) 'TrimmingTestLogger' does not implement interface member 'ILogWriter.HasLoggedErrors'
        
          
                src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TextLogWriter.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      - Implement HasLoggedErrors in TrimmingTestLogger - Revert unnecessary changes to ObjectWriter.cs - Add empty line after if block in TextLogWriter.cs for style - Remove comment line that could go stale in Program.cs Co-authored-by: MichalStrehovsky <[email protected]>
| 
 Fixed by implementing  | 
- Track errors directly in Logger class instead of through ILogWriter interface - Remove HasLoggedErrors from ILogWriter interface - Remove error tracking from TextLogWriter and TrimmingTestLogger - Remove unnecessary File.Exists check before File.Delete (File.Delete handles non-existent files) Co-authored-by: agocke <[email protected]>
Changed HasLoggedErrors from expression-bodied property with backing field to auto-property with private setter. Co-authored-by: agocke <[email protected]>
| One more thing we might want to consider - looks like many tools will first create the output using a temporary file name and rename it with the correct name on success or delete it on failure. @agocke what do you think about doing that (not tagging copilot yet)? I can see positives/negatives with either, but it feels like the rename approach is pretty common. | 
| That sounds like a good approach to me. @copilot please implement that | 
Write to a temporary file (.tmp extension) during compilation and rename to final output on success. On failure, the temp file is deleted. This is more robust than writing directly and deleting on error. Co-authored-by: agocke <[email protected]>
When compilation errors occur, we no longer need to explicitly delete the temp file since it will be overwritten on the next compilation attempt (File.Move uses overwrite: true). Co-authored-by: jkotas <[email protected]>
This reverts commit 3a1b59d. Restoring the File.Delete call for temp file cleanup when compilation errors occur. Co-authored-by: jkotas <[email protected]>
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.
Pull Request Overview
This PR implements atomic output file writing for the AOT compiler to prevent partial files from being left on disk when compilation fails. The compiler now writes to a temporary file and only renames it to the final output on successful completion.
Key Changes:
- Introduced a HasLoggedErrorsproperty in theLoggerclass to track compilation errors and warnings-as-errors
- Modified the compilation process to write to a temporary file first, then atomically rename it on success
- Added cleanup logic to delete temporary files when compilation fails
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description | 
|---|---|
| src/coreclr/tools/aot/ILCompiler/Program.cs | Implements temporary file creation, error-based cleanup, and atomic rename on success | 
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Logger.cs | Adds HasLoggedErrorsproperty and sets it when errors or warnings-as-errors are logged | 
| } | ||
| catch (Exception ex) | ||
| { | ||
| Console.Error.WriteLine($"Failed to create output file: {ex.Message}"); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
The error message 'Failed to create output file' is misleading since the output file was already successfully created as a temporary file. This is actually a failure to rename/move the file to its final location. Consider changing to 'Failed to move output file to final location' or 'Failed to rename temporary output file'.
| Console.Error.WriteLine($"Failed to create output file: {ex.Message}"); | |
| Console.Error.WriteLine($"Failed to move temporary output file to final location: {ex.Message}"); | 
| if (warning.Value.Category == MessageCategory.WarningAsError) | ||
| HasLoggedErrors = true; | 
    
      
    
      Copilot
AI
    
    
    
      Oct 31, 2025 
    
  
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.
The logic for setting HasLoggedErrors is duplicated across multiple methods (lines 99-100, 110-111, 151, 161). Consider extracting this into a helper method or setting the flag in a centralized location like _logWriter.WriteWarning and _logWriter.WriteError to reduce duplication and ensure consistency.
Fix: Prevent ILC from writing object file when errors are produced
When
--warnaserroris enabled and warnings are produced, ILC writes the output object file, causing build systems to incorrectly consider targets up-to-date on subsequent builds.Changes Made
Loggerclass (no interface changes needed).tmpfile and rename on successHow It Works
The compiler now uses a temporary file approach to ensure atomic output:
{outputFile}.tmpLoggerclass tracks when errors or warnings-as-errors are loggedProgram.Run():File.Move(..., overwrite: true)This ensures that:
Implementation:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.