-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[ILASM] Add support for deterministic builds and PDB checksums #109091
Conversation
The determinism tests were failing because I was using different assembly names as inputs into ilasm, which would change the CodeView debug directory entry. This should be fixed |
/azp run runtime-coreclr ilasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-coreclr ilasm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-coreclr ilasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -1289,18 +1454,59 @@ HRESULT Assembler::CreatePEFile(_In_ __nullterminated WCHAR *pwzOutputFilename) | |||
|
|||
if (FAILED(hr=CreateTLSDirectory())) goto exit; | |||
|
|||
if (FAILED(hr=CreateDebugDirectory())) goto exit; | |||
|
|||
if (FAILED(hr=m_pCeeFileGen->SetOutputFileName(m_pCeeFile, pwzOutputFilename))) goto exit; |
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.
Was it problematic in some way to set the file name here?
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.
Looks like a no-op change -- reverted.
Moving the assemblies produced by ILAsm around to compare them in the determinism tests was causing sporadic failures on Windows. @jkotas are you ok with me reverting to comparing the assemblies' hashes? |
/azp run runtime-coreclr ilasm |
Azure Pipelines successfully started running 1 pipeline(s). |
What is an example of the sporadic failure that you are trying to work around? |
From the last run (example log), after running the initial ILDasm/ILAsm roundtrip, the |
Antivirus scanners tend to lock the binary after it was created for some time on Windows. It is best to avoid renaming or deleting the binary shortly after it was created. If you do that, you typically have to have a retry loop around it to make it reliable. Is it possible to change the test such that it does not rename or delete the binaries shortly after they are created? |
The problem is the output name we give to ILAsm has to be consistent to not break determinism, but we also don't want the second run of ILAsm to overwrite the previous assembly since we need to compare their contents. We can avoid renaming the files by reading each binary completely into memory after each ILAsm invocation, but that seems impractical for large assemblies. With the hashing strategy, we don't have to keep each file completely in memory to do the comparison. |
Does the full path need to be the same or just the file name? |
I believe the whole path has to be the same, as the path of the corresponding PDB is included in the CodeView entry. Ex:
|
I suspect that you may see intermittent failures with the hashing approach as well since you are still overwriting a recently created binary. The hashing may have reduced the probability of the intermittent failures by adding a delay that is sufficient for antivirus to finish the scan and let the binary go most of the time. I do not have strong opinion about this. If you want to go with the hashing, it is fine with me. |
Sounds good, I'll keep this in mind. Thank you for the reviews! ILAsm CI failure on macOS looks related to infra. |
Follow-up to #85344. Fixes #8293. Fixes #62484.
This PR simply pulls in the SHA-256 wrappers added in #109559 into the prototype in #85344.