Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ILASM] Added /DET flag for deterministic compilations #85344

Closed
wants to merge 32 commits into from
Closed

[ILASM] Added /DET flag for deterministic compilations #85344

wants to merge 32 commits into from

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Apr 25, 2023 •
edited
Loading

Description

Resolves #8293
Resolves #62484

Part of this is from the work I did two years ago on the internal version of ILASM for .NET Framework.

Additional changes:

  • PdbChecksum and Deterministic Debug Directory entries
  • Deterministic Pdbs

Acceptance Criteria

  • SHA256 hash for non-win32
  • Investigate Portable PDB determinism (PR includes generating a PDB GUID based on the PDB contents)
  • Investigate Full paths to the source files (This is already handled)
  • Deterministic tests
    • When running ilasmroundtrip tests, they will also verify deterministic assemblies and pdbs emitted by ILASM.

@ghost ghost assigned TIHan Apr 25, 2023
@ghost
Copy link

ghost commented Apr 25, 2023

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Resolves #8293

Part of this is from the work I did last year on the internal version of ILASM from .NET Framework.

Author: TIHan
Assignees: -
Labels:

area-ILTools-coreclr
Milestone: -

@TIHan TIHan changed the title [ILASM] Added '/DET' flag for deterministic compilations [ILASM] Added /DET flag for deterministic compilations Apr 25, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Apr 25, 2023

@GrabYourPitchforks I need to use a SHA256 hash algo for all platforms, do you have any recommendations?

@build-analysis build-analysis bot mentioned this pull request Apr 26, 2023
Closed
@TIHan TIHan mentioned this pull request Apr 26, 2023
Open
@ghost ghost closed this May 27, 2023
@ghost
Copy link

ghost commented May 27, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@TIHan TIHan reopened this May 31, 2023
@ghost ghost closed this Jun 30, 2023
@ghost
Copy link

ghost commented Jun 30, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@TIHan
Copy link
Contributor Author

TIHan commented Jul 14, 2023

Re-opening as I want to finish this up.

@TIHan TIHan reopened this Jul 14, 2023
@TIHan TIHan added this to the 9.0.0 milestone Jul 24, 2023
@ghost ghost closed this Aug 23, 2023
@ghost
Copy link

ghost commented Aug 23, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Jun 5, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jun 5, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jun 5, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jun 6, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jun 6, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jun 14, 2024

So far, I'm happy with the changes, but CI times out as I think there is a problem with how I'm constructing the debug directories.

@dotnet-policy-service dotnet-policy-service bot removed this from the 9.0.0 milestone Jul 14, 2024
@TIHan TIHan reopened this Jul 23, 2024
@TIHan
Copy link
Contributor Author

TIHan commented Jul 23, 2024

/azp run runtime-coreclr ilasm

jkotas
jkotas reviewed Jul 24, 2024
View reviewed changes
src/coreclr/md/enc/sha256.cpp
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
//*****************************************************************************
// sha256.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having our own implementation of sha256 is going to be compliance burden. You should discuss this with the security folks (email .NET Security Team).

Copy link
Contributor Author

@TIHan TIHan Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair. Will do.

Before, I had the impl using the built-in bcrypt on windows, but I do not know an equivalent for the other OSes, other than OpenSSL. I think using OpenSSL might be overkill for this, but is there any other alternative?

@TIHan
Copy link
Contributor Author

TIHan commented Jul 24, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jul 24, 2024

/azp run runtime-coreclr ilasm

@TIHan
Copy link
Contributor Author

TIHan commented Jul 25, 2024

/azp run runtime-coreclr ilasm

@TIHan TIHan added this to the 10.0.0 milestone Aug 12, 2024
@dotnet-policy-service dotnet-policy-service bot removed this from the 10.0.0 milestone Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidwrighton davidwrighton davidwrighton left review comments

@jkotas jkotas jkotas left review comments

Assignees

@TIHan TIHan

Labels
area-ILTools-coreclr
Projects
None yet
Milestone
No milestone
3 participants
@TIHan @jkotas @davidwrighton