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

[linker] Remove non-bitcode compatible code, and show a warning. #5551

Merged
merged 9 commits into from
Feb 7, 2019

Conversation

rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne commented Feb 5, 2019

Remove code not currently compatible with bitcode and replace it with an exception instead (otherwise we may assert at runtime).

Also show a warning when we detect this.

This is quite helpful when looking at watch device test runs to filter out failures we already know about, since this exception message is much more specific than the one mono throws ("Attempting to JIT compile method ...").

This fixes point #2 in #4763.

Remove code not currently compatible with bitcode and replace it with an
exception instead (otherwise we'll assert at runtime).

Also show a warning when we detect this.

This is quite helpful when looking at watch device test runs to filter out
failures we already know about.

This fixes point #2 in dotnet#4763.
@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 82 tests passed.

Failed tests

  • MTouch tests/NUnit: Failed (Execution failed with exit code 2)


Currently Xamarin.iOS does not support the 'filter' exception clauses when
compiling to bitcode. Any methods containing such code will throw a
NotSupportedException exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is not clear when the exception is thrown.

Is the current behaviour to assert when executing the method ? or
only when an exception occurs ? or
only when an exception with a filter occurs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current behavior is to throw an exception when the method is executed (LLVM completely fails for the method if it contains a filter clause, which means that there's no native code for the method at all). I've seen asserts too, but I'm not sure when it asserts and when an exception is thrown.

I've updated the text to be more descriptive.

```csharp
void Method ()
{
throw new NotSupportedException ("This method contains IL not supported when compiled to bitcode.");
Copy link
Contributor

Choose a reason for hiding this comment

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

That example should be copied in the main documentation (or be linked from).

	try {
			ThrowsAnExceptionOnlyOnTuesday ();
		} catch (Exception e) {
			throw new NotSupportedException ("This method contains IL not supported when compiled to bitcode.");
		}
	}

since it's not clear (from the warning) which of the above code is generated as a replacement.

Copy link
Member Author

Choose a reason for hiding this comment

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

By "main documentation" you mean mtouch-errors.md?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's wait double-clicking on the error/warning will give to developers


namespace MonoTouch.Tuner {

public class RemoveBitcodeIncompatibleCodeStep : BaseStep {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a SubStep would be better (less code, since it shares the assembly/types/methods iterations) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it to use a SubStep.

* Add support for reporting more than one MT2105 at the same time when making
  the errors instead of warnings.
* Only report MT2105 for methods that haven't been linked away.
* Format the error message nicer for properties.
@monojenkins
Copy link
Collaborator

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
🔥 Test run failed 🔥

Test results

2 tests failed, 0 tests skipped, 81 tests passed.

Failed tests

  • MSBuild tests/iOS: Failed (Execution failed with exit code 2)
  • MTouch tests/NUnit: Failed (Execution failed with exit code 3)

using Xamarin.Linker;

namespace MonoTouch.Tuner {
public class RemoveBitcodeIncompatibleCodeStep : BaseSubStep {
Copy link
Contributor

Choose a reason for hiding this comment

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

oops, I guess I should have fully completed my train of thoughts... :)
ExceptionalSubStep makes support a lot easier if an exception is thrown during processing (which was the main drawback of substeps), otherwise there's no way to know (from logs) what caused the issue

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
Test run succeeded

@monojenkins
Copy link
Collaborator

Build failure
Build was aborted

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
Test run succeeded

@rolfbjarne rolfbjarne merged commit 1355079 into dotnet:master Feb 7, 2019
rolfbjarne added a commit that referenced this pull request Feb 26, 2019
* [linker] Remove non-bitcode compatible code, and show a warning.

Remove code not currently compatible with bitcode and replace it with an
exception instead (otherwise we'll assert at runtime).

Also show a warning when we detect this.

This is quite helpful when looking at watch device test runs to filter out
failures we already know about.

This fixes point #2 in #4763.

* Improve documentation.

* Simplify linker code by using a substep.

* Fix whitespace issues.

* Improve reporting.

* Add support for reporting more than one MT2105 at the same time when making
  the errors instead of warnings.
* Only report MT2105 for methods that haven't been linked away.
* Format the error message nicer for properties.

* Tweak a bit for warning tests to pass.

* Use ExceptionalSubStep to provide better error information.

* Adjust where linker warnings/errors are reported from to avoid a NullReferenceException.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants