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

Make APK and shared library alignment configurable #9046

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Make APK and shared library alignment configurable #9046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd9941
Make APK and shared library alignment configurable
grendello Jun 20, 2024
1be4938
Dry the code a bit
grendello Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,15 @@

<!-- Mono components -->
<AndroidEnableProfiler Condition=" '$(AndroidEnableProfiler)' == ''">false</AndroidEnableProfiler>

<!--
Android package (apt/aab) alignment, expressed as the page size in kilobytes. Two values are supported: 4 and 16.
Sometime next year the default value should be changed to 16 since it's going to be a Google Play store requirement for
application submissions.

When changing this default, change the value of `DefaultZipAlignment` in `src/Xamarin.Android.Build.Tasks/Tasks/AndroidZipAlign.cs` as well
-->
<_AndroidZipAlignment Condition=" '$(_AndroidZipAlignment)' == '' ">4</_AndroidZipAlignment>
</PropertyGroup>

<!-- User-facing configuration-specific defaults -->
Expand Down
7 changes: 5 additions & 2 deletions src/Xamarin.Android.Build.Tasks/Tasks/AndroidZipAlign.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ namespace Xamarin.Android.Tasks
{
public class AndroidZipAlign : AndroidRunToolTask
{
// Sometime next year the default value should be changed to 16 since it's going to be a Google Play store requirement for
// application submissions
internal const int DefaultZipAlignment = 4;

public override string TaskPrefix => "AZA";

[Required]
Expand All @@ -15,7 +19,7 @@ public class AndroidZipAlign : AndroidRunToolTask
[Required]
public ITaskItem DestinationDirectory { get; set; }

int alignment = 4;
int alignment = DefaultZipAlignment;
public int Alignment {
get {return alignment;}
set {alignment = value;}
Expand Down Expand Up @@ -53,4 +57,3 @@ protected override void LogEventsFromTextOutput (string singleLine, MessageImpor
}
}
}

8 changes: 8 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public class GeneratePackageManagerJava : AndroidTask
public string AndroidSequencePointsMode { get; set; }
public bool EnableSGenConcurrent { get; set; }
public string? CustomBundleConfigFile { get; set; }
public int ZipAlignmentPages { get; set; } = AndroidZipAlign.DefaultZipAlignment;

[Output]
public string BuildId { get; set; }
Expand Down Expand Up @@ -334,6 +335,12 @@ void AddEnvironment ()

bool haveRuntimeConfigBlob = !String.IsNullOrEmpty (RuntimeConfigBinFilePath) && File.Exists (RuntimeConfigBinFilePath);
var jniRemappingNativeCodeInfo = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<GenerateJniRemappingNativeCode.JniRemappingNativeCodeInfo> (ProjectSpecificTaskObjectKey (GenerateJniRemappingNativeCode.JniRemappingNativeCodeInfoKey), RegisteredTaskObjectLifetime.Build);
uint zipAlignmentMask = ZipAlignmentPages switch {
4 => 3,
16 => 15,
_ => throw new InvalidOperationException ($"Internal error: unsupported zip page alignment value {ZipAlignmentPages}")
Copy link
Member

Choose a reason for hiding this comment

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

Should we log an error with an error code instead of throw?

Copy link
Contributor Author

@grendello grendello Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I treat this condition as an internal error, because the property that specifies the alignment is "private" and shouldn't be modified by our users (it's mostly for our convenience of testing) and so I want the invalid value to be signaled in as loud and annoying kind of way :)

};

var appConfigAsmGen = new ApplicationConfigNativeAssemblyGenerator (environmentVariables, systemProperties, Log) {
UsesMonoAOT = usesMonoAOT,
UsesMonoLLVM = EnableLLVM,
Expand All @@ -357,6 +364,7 @@ void AddEnvironment ()
JNIEnvRegisterJniNativesToken = jnienv_registerjninatives_method_token,
JniRemappingReplacementTypeCount = jniRemappingNativeCodeInfo == null ? 0 : jniRemappingNativeCodeInfo.ReplacementTypeCount,
JniRemappingReplacementMethodIndexEntryCount = jniRemappingNativeCodeInfo == null ? 0 : jniRemappingNativeCodeInfo.ReplacementMethodIndexEntryCount,
ZipAlignmentMask = zipAlignmentMask,
MarshalMethodsEnabled = EnableMarshalMethods,
IgnoreSplitConfigs = ShouldIgnoreSplitConfigs (),
};
Expand Down
11 changes: 10 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/LinkApplicationSharedLibraries.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ sealed class InputFiles
[Required]
public string AndroidBinUtilsDirectory { get; set; }

public int ZipAlignmentPages { get; set; } = AndroidZipAlign.DefaultZipAlignment;

public override System.Threading.Tasks.Task RunTaskAsync ()
{
return this.WhenAll (GetLinkerConfigs (), RunLinker);
Expand Down Expand Up @@ -129,7 +131,6 @@ IEnumerable<Config> GetLinkerConfigs ()
"-soname libxamarin-app.so " +
"-z relro " +
"-z noexecstack " +
"-z max-page-size=4096 " +
"--enable-new-dtags " +
"--build-id " +
"--warn-shared-textrel " +
Expand Down Expand Up @@ -186,6 +187,14 @@ IEnumerable<Config> GetLinkerConfigs ()
}
}

uint maxPageSize = ZipAlignmentPages switch {
4 => 4096,
16 => 16384,
_ => throw new InvalidOperationException ($"Internal error: unsupported zip page alignment value {ZipAlignmentPages}")
Copy link
Member

Choose a reason for hiding this comment

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

I guess we'd use LogError() here too, if we changed the other one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably should just move this switch to a method inside MonoAndroidHelper class.

};
targetLinkerArgs.Add ("-z");
targetLinkerArgs.Add ($"max-page-size={maxPageSize}");

string targetArgs = String.Join (" ", targetLinkerArgs);
yield return new Config {
LinkerPath = ld,
Expand Down
12 changes: 9 additions & 3 deletions ...arin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,12 @@ public sealed class ApplicationConfig
public uint jnienv_registerjninatives_method_token;
public uint jni_remapping_replacement_type_count;
public uint jni_remapping_replacement_method_index_entry_count;
public uint zip_alignment_mask;
public uint mono_components_mask;
public string android_package_name = String.Empty;
}

const uint ApplicationConfigFieldCount = 26;
const uint ApplicationConfigFieldCount = 27;

const string ApplicationConfigSymbolName = "application_config";
const string AppEnvironmentVariablesSymbolName = "app_environment_variables";
Expand Down Expand Up @@ -326,12 +327,17 @@ static ApplicationConfig ReadApplicationConfig (EnvironmentFile envFile)
ret.jni_remapping_replacement_method_index_entry_count = ConvertFieldToUInt32 ("jni_remapping_replacement_method_index_entry_count", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 24: // mono_components_mask: uint32_t / .word | .long
case 24: // zip_alignment_mask: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.zip_alignment_mask = ConvertFieldToUInt32 ("zip_alignment_mask", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;
Comment on lines -329 to +333
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit to putting new settings at the end? Then it could be slightly more compatible with existing files? (Does it even matter?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't matter. There's no guarantee of compatibility between any releases of XA with regards to this (by design).


case 25: // mono_components_mask: uint32_t / .word | .long
Assert.IsTrue (expectedUInt32Types.Contains (field [0]), $"Unexpected uint32_t field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
ret.mono_components_mask = ConvertFieldToUInt32 ("mono_components_mask", envFile.Path, parser.SourceFilePath, item.LineNumber, field [1]);
break;

case 25: // android_package_name: string / [pointer type]
case 26: // android_package_name: string / [pointer type]
Assert.IsTrue (expectedPointerTypes.Contains (field [0]), $"Unexpected pointer field type in '{envFile.Path}:{item.LineNumber}': {field [0]}");
pointers.Add (field [1].Trim ());
break;
Expand Down
3 changes: 3 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ sealed class ApplicationConfig
public uint jni_remapping_replacement_type_count;
public uint jni_remapping_replacement_method_index_entry_count;

// 3, for 4-byte alignment (4k memory pages); 15, for 16-byte alignment (16k memory pages)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are the mask 3 and 15 respecitvely? noth 4 and 16? (silly question).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With alignment to 4k, bits 0 and 1 of the address must be 0 and with 16k, bits 0-3 must be 0. We could use the actual alignment here and then subtract 1 to obtain a mask from it, but it would require an additional instruction on the run time :) As it is now, we can add 1 to the mask to get the alignment value when reporting an error - an extra instruction but only in error situations, so performance isn't an issue. Yes, I know, it's just one instruction :)

The relevant code is here

public uint zip_alignment_mask;

[NativeAssembler (NumberFormat = LLVMIR.LlvmIrVariableNumberFormat.Hexadecimal)]
public uint mono_components_mask;
public string android_package_name = String.Empty;
Expand Down
2 changes: 2 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ sealed class XamarinAndroidBundledAssembly
public int JNIEnvRegisterJniNativesToken { get; set; }
public int JniRemappingReplacementTypeCount { get; set; }
public int JniRemappingReplacementMethodIndexEntryCount { get; set; }
public uint ZipAlignmentMask { get; set; }
public MonoComponent MonoComponents { get; set; }
public PackageNamingPolicy PackageNamingPolicy { get; set; }
public List<ITaskItem> NativeLibraries { get; set; }
Expand Down Expand Up @@ -244,6 +245,7 @@ protected override void Construct (LlvmIrModule module)
jnienv_registerjninatives_method_token = (uint)JNIEnvRegisterJniNativesToken,
jni_remapping_replacement_type_count = (uint)JniRemappingReplacementTypeCount,
jni_remapping_replacement_method_index_entry_count = (uint)JniRemappingReplacementMethodIndexEntryCount,
zip_alignment_mask = ZipAlignmentMask,
mono_components_mask = (uint)MonoComponents,
android_package_name = AndroidPackageName,
};
Expand Down
4 changes: 4 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,6 +1724,7 @@ because xbuild doesn't support framework reference assemblies.
UseAssemblyStore="$(AndroidUseAssemblyStore)"
EnableMarshalMethods="$(_AndroidUseMarshalMethods)"
CustomBundleConfigFile="$(AndroidBundleConfigurationFile)"
ZipAlignmentPages="$(_AndroidZipAlignment)"
>
<Output TaskParameter="BuildId" PropertyName="_XamarinBuildId" />
</GeneratePackageManagerJava>
Expand Down Expand Up @@ -2010,6 +2011,7 @@ because xbuild doesn't support framework reference assemblies.
ApplicationSharedLibraries="@(_ApplicationSharedLibrary)"
DebugBuild="$(AndroidIncludeDebugSymbols)"
AndroidBinUtilsDirectory="$(AndroidBinUtilsDirectory)"
ZipAlignmentPages="$(_AndroidZipAlignment)"
/>
<ItemGroup>
<FileWrites Include="@(_ApplicationSharedLibrary)" />
Expand Down Expand Up @@ -2354,6 +2356,7 @@ because xbuild doesn't support framework reference assemblies.
<Delete Files="%(ApkAbiFilesSigned.FullPath)" Condition=" '$(AndroidUseApkSigner)' == 'true' "/>
<AndroidZipAlign Condition=" '$(AndroidUseApkSigner)' == 'true' "
Source="%(ApkAbiFilesIntermediate.Identity)"
Alignment="$(_AndroidZipAlignment)"
DestinationDirectory="$(OutDir)"
ToolPath="$(ZipAlignToolPath)"
ToolExe="$(ZipalignToolExe)"
Expand Down Expand Up @@ -2389,6 +2392,7 @@ because xbuild doesn't support framework reference assemblies.
<Message Text="Unaligned android package '%(ApkAbiFilesUnaligned.FullPath)'" Condition=" '$(AndroidUseApkSigner)' != 'True' And '$(AndroidPackageFormat)' != 'aab' "/>
<AndroidZipAlign Condition=" '$(AndroidUseApkSigner)' != 'True' And '$(AndroidPackageFormat)' != 'aab' "
Source="%(ApkAbiFilesUnaligned.Identity)"
Alignment="$(_AndroidZipAlignment)"
DestinationDirectory="$(OutDir)"
ToolPath="$(ZipAlignToolPath)"
ToolExe="$(ZipalignToolExe)"
Expand Down
1 change: 1 addition & 0 deletions src/native/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ set(POTENTIAL_XA_COMMON_COMPILER_ARGS
-Wformat-security
-Wformat=2
-Wno-format-nonliteral
-Wno-vla-cxx-extension
-Wimplicit-fallthrough
-Wmisleading-indentation
-Wnull-dereference
Expand Down
6 changes: 3 additions & 3 deletions src/native/monodroid/embedded-assemblies-zip.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ EmbeddedAssemblies::zip_load_entry_common (size_t entry_index, std::vector<uint8
}

// assemblies must be 4-byte aligned, or Bad Things happen
if ((state.data_offset & 0x3) != 0) {
log_fatal (LOG_ASSEMBLY, "Assembly '%s' is located at bad offset %lu within the .apk\n", entry_name.get (), state.data_offset);
log_fatal (LOG_ASSEMBLY, "You MUST run `zipalign` on %s\n", strrchr (state.file_name, '/') + 1);
if ((state.data_offset & application_config.zip_alignment_mask) != 0) {
log_fatal (LOG_ASSEMBLY, "Assembly '%s' is located at bad offset %lu within the .apk", entry_name.get (), state.data_offset);
log_fatal (LOG_ASSEMBLY, "You MUST run `zipalign` on %s to align it on %u bytes ", strrchr (state.file_name, '/') + 1, application_config.zip_alignment_mask + 1);
Helpers::abort_application ();
}

Expand Down
5 changes: 5 additions & 0 deletions src/native/xamarin-app-stub/application_dso_stub.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,13 @@ const ApplicationConfig application_config = {
.number_of_assemblies_in_apk = 2,
.bundled_assembly_name_width = 0,
.number_of_dso_cache_entries = 2,
.number_of_shared_libraries = 2,
.android_runtime_jnienv_class_token = 1,
.jnienv_initialize_method_token = 2,
.jnienv_registerjninatives_method_token = 3,
.jni_remapping_replacement_type_count = 2,
.jni_remapping_replacement_method_index_entry_count = 2,
.zip_alignment_mask = 3,
.mono_components_mask = MonoComponent::None,
.android_package_name = android_package_name,
};
Expand All @@ -81,6 +83,7 @@ static char second_assembly_name[AssemblyNameWidth];
XamarinAndroidBundledAssembly bundled_assemblies[] = {
{
.file_fd = -1,
.file_name = nullptr,
.data_offset = 0,
.data_size = 0,
.data = nullptr,
Expand All @@ -90,6 +93,7 @@ XamarinAndroidBundledAssembly bundled_assemblies[] = {

{
.file_fd = -1,
.file_name = nullptr,
.data_offset = 0,
.data_size = 0,
.data = nullptr,
Expand Down Expand Up @@ -117,6 +121,7 @@ AssemblyStoreSingleAssemblyRuntimeData assembly_store_bundled_assemblies[] = {
AssemblyStoreRuntimeData assembly_store = {
.data_start = nullptr,
.assembly_count = 0,
.index_entry_count = 0,
.assemblies = nullptr,
};

Expand Down
1 change: 1 addition & 0 deletions src/native/xamarin-app-stub/xamarin-app.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ struct ApplicationConfig
uint32_t jnienv_registerjninatives_method_token;
uint32_t jni_remapping_replacement_type_count;
uint32_t jni_remapping_replacement_method_index_entry_count;
uint32_t zip_alignment_mask; // 3, for 4-byte alignment (4k memory pages); 15, for 16-byte alignment (16k memory pages)
MonoComponent mono_components_mask;
const char *android_package_name;
};
Expand Down
Loading