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.
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
[mono] Extend MonoAOTCompiler Task #70851
[mono] Extend MonoAOTCompiler Task #70851
Changes from all commits
1338ca1
1fffa3b
e6c6176
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This check is causing some Android AOT builds to fail with:
This started happening here: dotnet/android#7127
Here is the project with
msbuild.binlog
inside:BuildBasicApplicationReleaseProfiledAotTrue.zip
I think the file it's looking for doesn't actually exist:
And this file is at:
Let me know if I need to change something in the Android workload, thanks!
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.
In themono-aot-cross
command line you have,llvm-outfile=obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o
. Is this file not expected to be an output? I don't see it in theobj
directory.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.
Full mono-aot-cross command:
Should
obj\Release\android-arm\aot\UnnamedProject.dll-llvm.o
not be expected to exist?It gets added to the list of files at
runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs
Line 784 in cc3ac1b
because no cache file is being used,
proxy.TempFile
will be same asproxy.TargetFile
.Also,
runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs
Line 788 in cc3ac1b
mono-aot-cross
failed but didn't return a non-zero error code?in the output I see many
LLVM failed
messages, are those expected?We do delete the temp file (in this case == target file):
runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs
Line 1249 in cc3ac1b
(I'll anyway prep a PR for fixing some temp file stuff)
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.
#71411 should remove the error. But IIUC, in this case if the
dll-llvm.o
file doesn't exist, then it will fail outside this task.Maybe we should add a check to ensure that all the "expected" output files actually exist after running
mono-aot-cross
. Unless you think there is some reason not to.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.
I'm honestly not sure about any of the intermediate files that are created here. We only really use the
.so
files that exist at the end, and those appear to exist? (Back when this task completed successfully)We also don't use
EnableLLVM=true
by default, so there could be an issue or two when that is enabled.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.
@jonathanpeppers
aotAssembly.SetMetadata("LlvmObjectFile", proxyFile.TargetFile);
- this isn't used?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.
No, we don't set this metadata at all. I think it's because we use the
.so
files as-is -- and don't currently do anything extra.There were some ideas about using a native linker to do extra things (which would need the
.o
files) -- but we haven't implemented that yet.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.
@akoeplinger please help out with this if ankit's fix doesn't solve the issue
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 code indicates that it is expecting an output file by creating this
ProxyFile
object:runtime/src/tasks/AotCompilerTask/MonoAOTCompiler.cs
Lines 784 to 785 in cc3ac1b
So, from correctness pov it would be completely fair to check that the expected output file exists after
mono-aot-cross
has run. But in this case it seems that:aotAssembly.SetMetadata("LlvmObjectFile", proxyFile.TargetFile);
, it is not being used else I would expect the task user/caller to break since the file is not generated.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.
I see it being used at:
runtime/src/tasks/AppleAppBuilder/AppleAppBuilder.cs
Lines 203 to 205 in 5877e8b
runtime/src/tasks/AndroidAppBuilder/ApkBuilder.cs
Lines 141 to 143 in 5877e8b
I'll make this file optional (for the existence check), and check for all other files.