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

Add an option to remove exception filters. #1770

Closed
wants to merge 1 commit into from

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Jan 20, 2021

Its controlled by the --remove-filters command line option.

This works by rewriting:

catch (Exception ex) when (<cond>) {
  ..
}

into:

catch (Exception ex) {
  if (!<cond>)
    throw ex;
  ..
}

@vargaz vargaz requested a review from marek-safar as a code owner January 20, 2021 14:07
@vargaz vargaz marked this pull request as draft January 20, 2021 14:07
@MichalStrehovsky
Copy link
Member

This test might be useful in this effort:

using System;

class P
{
    interface Q
    {
        static int s_counter;

        static void Main()
        {
            try
            {
                throw new Exception();
            }
            catch (ArgumentException) when (True())
            {
                Console.WriteLine("Unreached");
            }
            catch (Exception) when (Throw())
            {
                Console.WriteLine("Unreached");
                s_counter = -1;
            }
            catch (Exception) when (False())
            {
                Console.WriteLine("Unreached");
                s_counter = -1;
            }
            catch (Exception)
            {
                Console.WriteLine("Works as expected");
                s_counter |= 4;
            }

            if (s_counter == 7)
            {
                Console.WriteLine("Success");
            }
            else
            {
                Console.WriteLine("Fail");
            }
        }

        static bool Throw()
        {
            s_counter |= 1;
            throw new Exception();
        }

        static bool False()
        {
            s_counter |= 2;
            return false;
        }

        static bool True()
        {
            s_counter = -1;
            return true;
        }
    }
}

protected override void Process ()
{
foreach (var assembly in Context.Annotations.GetAssemblies ()) {
RewriteBodies (assembly.MainModule.Types);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to check if we are linking assembly otherwise do something else (e.g. issue warning).

@vargaz vargaz force-pushed the remove-filters branch 3 times, most recently from 8463f26 to 2cef215 Compare January 21, 2021 02:35
@vargaz vargaz marked this pull request as ready for review January 25, 2021 12:41
@marek-safar
Copy link
Contributor

I suspect that we'll need to run this code during the inner dev loop with an interpreter for consistency where hooking up linker would be a pain.

I'm wondering if we should take this as an opportunity to introduce a new task inside dotnet/runtime instead of injecting this functionality into linker. We have other similar needs for AOT pipeline which could be developed there instead of inside linker. It'd also allow us to run these steps heavily in parallel if we go with SRM instead of Cecil.

We would have run all of the following "steps" at the same time inside that task

  • exception filters rewriter
  • IL bodies remover
  • pinvoke generator
  • icalls generator

/cc @lewing

@vargaz vargaz marked this pull request as draft January 26, 2021 09:43
Its controlled by the --remove-filters command line option.

This works by rewriting:

catch (Exception ex) when (<cond>) {
  ..
}

into:

catch (Exception ex) {
  if (!<cond>)
    throw ex;
  ..
}
@vargaz
Copy link
Contributor Author

vargaz commented Jan 28, 2021

This currently can't handle the case when there are more than 1 filter clause for the same try range.

Closing for now in favor of: mono/mono#20796

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.

3 participants