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

[runtime] Optionally disable inlining #8798

Merged
merged 7 commits into from
Mar 22, 2024
Merged

Conversation

grendello
Copy link
Contributor

The native runtime by default builds with function inlining enabled,
making heavy use of the feature in order to generate faster code.
However, when debugging a native crash inlining causes stack traces to
point to unlikely locations, reporting the outer function as the crash
location instead of the inlined function where crash actually happened.

In order to make such debugging easier, the XA_NO_INLINE environment
property may be exported before building the repository. This will
force all normally inlined functions to be strictly preserved and kept
separate. The generated code will be slower, but crash stack traces
should be much more precise.

Also add support for the XA_NO_STRIP variable which makes it easy
to produce runtime shared libraries with full debug symbols.

In normal builds, all the debugging information is stripped from the
runtime shared libraries, thus making stack traces rarely point to
anything more than the surrounding function name (which may sometimes be
misleading, too). The XA_NO_STRIP environment variable can be
exported before building the native runtime in order to leave the
symbols in the resulting shared library.

The native runtime by default builds with function inlining enabled,
making heavy use of the feature in order to generate faster code.
However, when debugging a native crash inlining causes stack traces to
point to unlikely locations, reporting the outer function as the crash
location instead of the inlined function where crash actually happened.

In order to make such debugging easier, the `XA_NO_INLINE` environment
property may be exported before building the repository.  This will
force all normally inlined functions to be strictly preserved and kept
separate.  The generated code will be slower, but crash stack traces
should be much more precise.

Also add support for the `XA_NO_STRIP` variable which makes it easy
to produce runtime shared libraries with full debug symbols.

In normal builds, all the debugging information is stripped from the
runtime shared libraries, thus making stack traces rarely point to
anything more than the surrounding function name (which may sometimes be
misleading, too).  The `XA_NO_STRIP` environment variable can be
exported before building the native runtime in order to leave the
symbols in the resulting shared library.
@grendello grendello requested a review from jonpryor as a code owner March 7, 2024 17:39
@jonpryor
Copy link
Member

Usability-wise, I think I'd prefer that this be set/controlled via MSBuild properties (Configuration.Override.props) or initial make … invocation (and preserved "somewhere").

My concern is around my own usage around building my xamarin-android checkout, in which I tend to have multiple different tabs open, each with potentially different environments. I can thus see a scenario where building from one tab results in no compiler inlining, while another tab doesn't, and getting confused as to what's going on.

Using MSBuild properties would likely be as easy as updating: https://github.com/xamarin/xamarin-android/blob/cbfcc23fec533973d9340118ad8713fcc70e08b9/src/monodroid/monodroid.targets#L78

<PropertyGroup>
  <_NoInline Condition=" '$(DoNotInlineMonodroid)' == 'true' ">-DXA_NO_INLINE=true</_NoInline>
  <_NoStrip Condition=" '$(DoNotStripMonodroid)' == 'true' ">-DXA_NO_STRIP=true<_NoStrip>
  <_CmakeAndroidFlags>$(_NoInline) $(_NoStrip) --debug-output -GNinja …</_CmakeAndroidFlags>
</PropertyGroup>

@grendello
Copy link
Contributor Author

@jonpryor I'll add the MSBuild properties, but I'll keep the environment variables as I prefer it for my workflow (make prepare all), since I don't have easy access to monodroid's msbuild properties with that.

* main:
  Bump to dotnet/installer@e911f5c82c 9.0.100-preview.3.24161.2 (#8802)
  Bump to xamarin/Java.Interop/main@3436a30 (#8799)
  [templates] Remove redundant "template" from display name. (#8773)
* main:
  [runtime] Remove the last vestiges of desktop builds (#8810)
  [ci] Don't auto-retry APK test suites. (#8811)
  [Microsoft.Android.Templates] Update EN l10n template strings (#8808)
  Bump to xamarin/Java.Interop/main@651de42 (#8809)
  [Mono.Android] is now "trimming safe" (#8778)
  [Mono.Android] Fix missing enum issues that cause BG8800 warnings. (#8707)
  Bump external/Java.Interop from `3436a30` to `5bca8ad` (#8803)
  Bump to xamarin/monodroid@77124dc1 (#8804)
* main:
  LEGO: Merge pull request 8818
  Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817)
  Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816)
  [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795)
  Localized file check-in by OneLocBuild Task (#8815)
  [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478)
  Localized file check-in by OneLocBuild Task (#8813)
  [Xamarin.Android.Build.Tasks] %(AndroidAsset.AssetPack) Support (#8631)
@jonpryor
Copy link
Member

Draft commit message:

The native runtime by default builds with function inlining enabled,
making heavy use of the feature in order to generate faster code.
However, when debugging a native crash inlining causes stack traces to
point to unlikely locations, reporting the outer function as the crash
location instead of the inlined function where crash actually happened.

In order to make such debugging easier, do one of the following:

  * Export the `XA_NO_INLINE` environment variable to "true-ish"

        export XA_NO_INLINE=1

  * Set the `$(DoNotInlineMonodroid)` MSBuild property to true,
    either by adding to `Configuration.Override.props`:

        <DoNotInlineMonodroid>true</DoNotInlineMonodroid>>

    or by specifying on the command-line:

        ./dotnet-local.sh build Xamarin.Android.sln -p:DoNotInlineMonodroid=true …

This will force all normally inlined functions to be strictly
preserved and kept separate.  The generated code will be slower, but
crash stack traces should be much more precise.

Additionally, `strip`ing of the native shared runtime can be disabled,
making it easier to get full debug symbols.  This can similarly be
enabled by doing one of the following:

  * Export the `XA_NO_STRIP` environment variable to "true-ish"

        export XA_NO_STRIP=1

  * Set the `$(DoNotStripMonodroid)` MSBuild property to true,
    either by adding to `Configuration.Override.props`:

        <DoNotStripMonodroid>true</DoNotStripMonodroid>>

    or by specifying on the command-line:

        ./dotnet-local.sh build Xamarin.Android.sln -p:DoNotStripMonodroid=true …

@jonpryor
Copy link
Member

jonpryor commented Mar 20, 2024

Part of me wonders if this should be the default behavior for Debug builds…

@grendello replied on Discord:

inlining no, I'd like the generated code to be as close to Release as possible, but we can disable stripping, sure

* main:
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
@jonpryor jonpryor merged commit e206c52 into main Mar 22, 2024
48 checks passed
@jonpryor jonpryor deleted the dev/grendel/optional-no-inline branch March 22, 2024 18:50
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
grendello added a commit that referenced this pull request Mar 27, 2024
* main:
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
  LEGO: Merge pull request 8818
  Bump to dotnet/installer@b40c44502d 9.0.100-preview.3.24165.20 (#8817)
  Bump com.android.tools:r8 from 8.2.47 to 8.3.37 (#8816)
  [Mono.Android] Prevent NullPointerException in TranslateStackTrace (#8795)
  Localized file check-in by OneLocBuild Task (#8815)
  [Xamarin.Android.Build.Tasks] Make all assemblies RID-specific (#8478)
  Localized file check-in by OneLocBuild Task (#8813)
grendello added a commit that referenced this pull request Apr 3, 2024
* main:
  Bump to dotnet/installer@dc43d363d2 9.0.100-preview.4.24175.5 (#8828)
  [Xamarin.Android.Build.Tasks] Update to newer ILRepack which supports debug files. (#8839)
  Bump 'NuGet.*' and 'Newtonsoft.Json' NuGet versions. (#8825)
  Localized file check-in by OneLocBuild Task (#8844)
  [LayoutBindings] Fix '[Preserve]' is obsolete warnings (#8529)
  LEGO: Merge pull request 8837
  [ci] Use managed identity for ApiScan (#8823)
  [Xamarin.Android.Build.Tasks] DTBs should not rm generator output (#8706)
  [Xamarin.Android.Build.Tasks] Bump to NuGet 6.7.1 (#8833)
  $(AndroidPackVersionSuffix)=preview.4; net9 is 34.99.0.preview.4 (#8831)
  Localized file check-in by OneLocBuild Task (#8824)
  [Xamarin.Android.Build.Tasks] Enable POM verification features. (#8649)
  [runtime] Optionally disable inlining (#8798)
  Fix assembly count when satellite assemblies are present (#8790)
  [One .NET] new "greenfield" projects are trimmed by default (#8805)
  Localized file check-in by OneLocBuild Task (#8819)
  LEGO: Merge pull request 8820
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants