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

[WIP] Windows: Fix wrongly caught exceptions due to non-unique TypeInfo_Class names #3517

Closed
wants to merge 1 commit into from

Conversation

kinke
Copy link
Member

@kinke kinke commented Jul 22, 2020

WinEH depends on string comparisons to check for matching catch clauses, and currently depends on TypeInfo_Class.name to generate these strings at runtime when throwing an exception, see
https://github.com/ldc-developers/druntime/blob/19731a92a97dbe4d7f7a4e15ceaff8444a1f879a/src/ldc/eh_msvc.d#L192-L211.
So if those names were fully qualified (cd->toPrettyChars(*true*) during ClassInfo generation), we'd be fine, but that'd obviously be a breaking change and diverge from upstream.

@kinke
Copy link
Member Author

kinke commented Jul 22, 2020

@Geod24: This caused it: https://github.com/Geod24/localrest/blob/5196f35edbc2a5ab83573665fb9f789217477c85/source/geod24/LocalRest.d#L529. You may want to extract it as a private module-level class to prevent needless 'duplicates' (nested in templated RemoteAPI class and templated spawned function).

@@ -0,0 +1,28 @@
// Tests EH with two exception types with the same `TypeInfo_Class.name`
// (due to template args not being fully qualified).
Copy link
Member

Choose a reason for hiding this comment

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

hm, do you want to ask the mailinglist if this is really intended behavior?

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 surely don't want to.

@JohanEngelen
Copy link
Member

JohanEngelen commented Jul 22, 2020

I think I misunderstood.
I thought this code would catch the exception (exceptions get same name), but it doesn't on Linux. So that's good. Is this working differently on Windows?

import std.stdio;

struct Foo(T) {
    // name: gh3501.foo!(S).foo.MyException
    static class MyException : Exception
    {
        this() { super(null); }
    }
}

void bar1() {
    struct S {}
    Foo!S f;
    throw new f.MyException();
}

void bar2() {
    struct S {}
    Foo!S f2;
    try  {
        bar1();
    }
    catch (f2.MyException e)  {
        writeln("Exception caught");
    }
}

void main() {
 	bar2();   
}

@kinke
Copy link
Member Author

kinke commented Jul 22, 2020

This is purely Windows specific (edit: and LDC-specific, works with DMD), and yes, the exception gets wrongly caught. - I'm not sure what the underlying requirement for the string comparison is, but I guess it's the used Microsoft VC++ personality function. We could theoretically work around it in the compiler, but it'd probably be a big PITA (as the whole different MSVC EH universe wrt. LLVM IR altogether).

@Geod24
Copy link
Contributor

Geod24 commented Jul 23, 2020

You may want to extract it as a private module-level class to prevent needless 'duplicates' (nested in templated RemoteAPI class and templated spawned function).

Thanks for the pointer! Indeed I'm going to go with that solution, since we want to support a range of compilers.

@kinke
Copy link
Member Author

kinke commented Jul 23, 2020

[I didn't mention this to work around this particular issue for LDC, but because of template bloat, compilation speed etc.]

@JohanEngelen
Copy link
Member

JohanEngelen commented Jul 23, 2020

This is purely Windows specific (edit: and LDC-specific, works with DMD), and yes, the exception gets wrongly caught.

If LDC behavior deviates from DMD in this case, I think we should mention this in the release notes as a known issue.
Moreso because programs will behave differently on Linux than on Windows.
Do you agree?

@kinke
Copy link
Member Author

kinke commented Jul 23, 2020

Well we should probably start by adding an LDC issue for this (I previously thought this wouldn't be a real issue in practice, but your example shows that it can indeed be). ;)

kinke added a commit to kinke/ldc that referenced this pull request Jul 23, 2020
… TypeDescriptor

At the cost of an extra slice for all ClassInfos (MSVC targets only),
`mangledName`, populated for Throwable and derived classes only.
It is required to still being able to generate matching TypeDescriptors
at runtime (for a *thrown* exception and its base classes).
@kinke
Copy link
Member Author

kinke commented Jul 23, 2020

I've pushed a fix, at the cost of an extra slice (mangledName) for all ClassInfos and MSVC targets, only populated for Throwable and derived classes.

kinke added a commit to kinke/ldc that referenced this pull request Jul 23, 2020
… TypeDescriptor

At the cost of an extra slice for all ClassInfos (MSVC targets only),
`mangledName`, populated for Throwable and derived classes only.
It is required to still being able to generate matching TypeDescriptors
at runtime (for a *thrown* exception and its base classes).
@rainers
Copy link
Contributor

rainers commented Jul 24, 2020

I guess a similar problem will appear upstream if dlang/druntime#2874 is merged, but the issue has already appeared for other types: dlang/druntime#3138. So I think fixing the name generation would be better.

@kinke
Copy link
Member Author

kinke commented Jul 24, 2020

I agree, but that's not something we can easily switch to here. Once it's fixed upstream, we can get rid of this workaround here.

@kinke
Copy link
Member Author

kinke commented Aug 22, 2020

Update: we've come up with a feasible approach upstream, see PR linked above. It's 'blocked' by the poor way to tackle tight compiler/druntime couplings upstream, probably requiring 4 PRs in total (2x DMD, 2x druntime) and more work just to handle the transitions.

@kinke kinke changed the title Fix colliding WinEH TypeDescriptors for exceptions with the same TypeInfo_Class name [WIP] Windows: Fix wrongly caught exceptions due to non-unique TypeInfo_Class names Nov 13, 2020
… TypeDescriptor

At the cost of an extra slice for all ClassInfos (MSVC targets only),
`mangledName`, populated for Throwable and derived classes only.
It is required to still being able to generate matching TypeDescriptors
at runtime (for a *thrown* exception and its base classes).
@kinke
Copy link
Member Author

kinke commented Sep 7, 2021

Superseded by fully qualified (and hence unique) TypeInfo_Class names since D v2.098 and according adaptations in #3821.

@kinke kinke closed this Sep 7, 2021
@kinke kinke deleted the wineh_desc branch September 7, 2021 23:35
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