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.

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

Update Architecture to unknown machine #101038

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Update Architecture to unknown machine #101038

merged 4 commits into from
Apr 15, 2024

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Apr 15, 2024

Fix #96978 issue

Part of #84834
cc @dotnet/samsung

@clamp03 clamp03 requested a review from marek-safar as a code owner April 15, 2024 05:19
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Apr 15, 2024
@dotnet-policy-service dotnet-policy-service bot added linkable-framework Issues associated with delivering a linker friendly framework community-contribution Indicates that the PR has been added by a community member labels Apr 15, 2024
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Looks great!
(one less thing to worry about when adding a new platform support 😅)

Suggested by @am11. Thank you.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Fix build error by adding TargetArchitecture casting for int value

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@vitek-karas
Copy link
Member

/cc @sbomer @agocke

@clamp03
Copy link
Member Author

clamp03 commented Apr 15, 2024

I will handle failures tomorrow. Thank you.
Or @am11 you can take the issue, close this PR and make new PR. I think you are better than me to solve this issue. :)

@am11
Copy link
Member

am11 commented Apr 15, 2024

I was able to locally repro the illink error we are seeing in the CI; it repros with crossbuild+release config. This patch seem to fix it:

--- a/src/coreclr/binder/assemblybindercommon.cpp
+++ b/src/coreclr/binder/assemblybindercommon.cpp
@@ -143,8 +143,7 @@ namespace BINDER_SPACE
         }
         else
         {
-            if ((CLRPeKind & peILonly) && !(CLRPeKind & pe32Plus) &&
-                !(CLRPeKind & pe32BitRequired) && dwImageType == IMAGE_FILE_MACHINE_I386)
+            if ((CLRPeKind & peILonly) && !(CLRPeKind & pe32Plus) && !(CLRPeKind & pe32BitRequired))
             {
                 // Processor-agnostic (MSIL)
                 *PeKind = peMSIL;
diff --git a/src/coreclr/vm/peassembly.cpp b/src/coreclr/vm/peassembly.cpp
index c7c618b4852..99ccb8638f9 100644
--- a/src/coreclr/vm/peassembly.cpp
+++ b/src/coreclr/vm/peassembly.cpp
@@ -39,7 +39,7 @@ static void ValidatePEFileMachineType(PEAssembly *pPEAssembly)
     DWORD actualMachineType;
     pPEAssembly->GetPEKindAndMachine(&peKind, &actualMachineType);

-    if (actualMachineType == IMAGE_FILE_MACHINE_I386 && ((peKind & (peILonly | pe32BitRequired)) == peILonly))
+    if ((peKind & (peILonly | pe32BitRequired)) == peILonly)
         return;    // Image is marked CPU-agnostic.

     if (actualMachineType != IMAGE_FILE_MACHINE_NATIVE && actualMachineType != IMAGE_FILE_MACHINE_NATIVE_NI)

@am11
Copy link
Member

am11 commented Apr 15, 2024

Mono has a different behavior than existing coreclr and proposed patch:

case COFF_MACHINE_I386:
it ignores the "unknown" and "msil" cases for non-i386 archs and simply does not set the machine. Unless there is a downside, we can align that to the proposed behavior as well? cc @jkotas, @lambdageek

@jkotas
Copy link
Member

jkotas commented Apr 15, 2024

I was able to locally repro the illink error we are seeing in the CI; it repros with crossbuild+release config.

These failures suggests that the fix is not producing proper AnyCPU files. Have you checked that the fix is producing AnyCPU files?

AnyCPU are IMAGE_FILE_MACHINE_I386, COMIMAGE_FLAGS_ILONLY bit set, both COMIMAGE_FLAGS_32BITREQUIRED and COMIMAGE_FLAGS_32BITPREFERRED bits cleared.

I do not think we want to be changing the assembly loader as part of this fix.

@am11
Copy link
Member

am11 commented Apr 15, 2024

@jkotas, it is failing to read corelib:

Could not load file or assembly '/__w/1/s/artifacts/bin/trimmingTests/projects/System.ComponentModel.Primitives.TrimmingTests/VerifyCategoryNamesDontGetTrimmed/linux-x64/bin/Release/net9.0/linux-x64/publish/System.Private.CoreLib.dll'. An attempt was made to load a program with an incorrect format.

Shouldn't corelib be machine specific?

before:

Version   : v4.0.30319
CLR Header: 2.5
PE        : PE32+
CorFlags  : 0xc
ILONLY    : 0
32BITREQ  : 0
32BITPREF : 0
Signed    : 1

after:

Version   : v4.0.30319
CLR Header: 2.5
PE        : PE32
CorFlags  : 0x9
ILONLY    : 1
32BITREQ  : 0
32BITPREF : 0
Signed    : 1

If so, then maybe this PR is a no-go (revive dotnet/cecil#171)?

@jkotas
Copy link
Member

jkotas commented Apr 15, 2024

Shouldn't corelib be machine specific?

CoreLib is machine, OS and runtime specific, but this fact is not encoded in the PE header.

The runtime should not require CoreLib to be machine specific.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@jkotas jkotas merged commit 69062fd into dotnet:main Apr 15, 2024
78 of 80 checks passed
jkotas added a commit to jkotas/runtime that referenced this pull request Apr 15, 2024
Reduce amount of architecture specific code. Minor cleanup motivated by dotnet#101038.
jkotas added a commit that referenced this pull request Apr 16, 2024
Reduce amount of architecture specific code. Minor cleanup motivated by #101038.
@clamp03
Copy link
Member Author

clamp03 commented Apr 16, 2024

@am11 @jkotas Thank you so much!

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Update Architecture to unknown machine

* Update src/tools/illink/src/linker/Linker.Steps/OutputStep.cs

Suggested by @am11. Thank you.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

---------

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
Reduce amount of architecture specific code. Minor cleanup motivated by dotnet#101038.
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers community-contribution Indicates that the PR has been added by a community member linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RISC-V] Optimizing assemblies for size failed
7 participants