[cxx-interop] Don't crash when importing invalid decl. #35962

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

Merged

zoecarver merged 1 commit into swiftlang:main from zoecarver:cxx/fix-invalid-decls

Mar 20, 2021

Contributor

zoecarver commented Feb 13, 2021

If we see an invalid decl, rather than potentially crashing, simply bail.

zoecarver added the c++ interop label

zoecarver requested a review from varungandhi-apple

February 13, 2021 01:29

Contributor Author

zoecarver commented Feb 13, 2021

@swift-ci please smoke test.

varungandhi-apple reviewed

View reviewed changes

test/Interop/Cxx/static/Inputs/constexpr-static-member-var-errors.h Outdated

    
            @@ -0,0 +1,6 @@
          
              template <class T>

              struct IsSubtypeSame {

                static constexpr const bool value = __is_same(typename T::type, int);

Contributor

varungandhi-apple Feb 15, 2021

It would be nice if the test could avoid using a built-in directly. Instead you can do something like (for example):

struct S {};
template <class T>
 struct IsSubtypeSame {
   static constexpr const S value = T();
};
using Invalid = IsSubtypeSame<int>;

Contributor Author

zoecarver Feb 16, 2021

Hey, you got it to crash in a different way!

Anyway, I take your point. I'll try to re-write this test without using the builtin (and I'll submit another patch to fix the issue you exposed).

zoecarver force-pushed the cxx/fix-invalid-decls branch from 439d9df to 32a875c Compare

February 16, 2021 05:42

Contributor Author

zoecarver commented Feb 16, 2021

@swift-ci please smoke test.

Contributor Author

zoecarver commented Feb 16, 2021

@swift-ci please smoke test macOS.

Contributor Author

zoecarver commented Feb 16, 2021

Same error, but it doesn't seem related 😕 I'll give it one more try.

@swift-ci please smoke test macOS.

Contributor Author

zoecarver commented Feb 22, 2021

@swift-ci please smoke test macOS.

1 similar comment

Contributor Author

zoecarver commented Feb 22, 2021

@swift-ci please smoke test macOS.

zoecarver force-pushed the cxx/fix-invalid-decls branch from 32a875c to f89a6e5 Compare

March 7, 2021 21:15

Contributor Author

zoecarver commented Mar 7, 2021

@swift-ci please smoke test macOS.

Contributor Author

zoecarver commented Mar 8, 2021

@swift-ci please test.

Contributor

swift-ci commented Mar 8, 2021

Build failed
Swift Test Linux Platform
Git Sha - f89a6e5914065a87f031d575d754d5e4875600b7

Contributor

swift-ci commented Mar 8, 2021

Build failed
Swift Test OS X Platform
Git Sha - f89a6e5914065a87f031d575d754d5e4875600b7

Contributor Author

zoecarver commented Mar 9, 2021

@swift-ci please test.

Contributor

swift-ci commented Mar 9, 2021

Build failed
Swift Test Linux Platform
Git Sha - f89a6e5914065a87f031d575d754d5e4875600b7

Contributor

swift-ci commented Mar 9, 2021

Build failed
Swift Test OS X Platform
Git Sha - f89a6e5914065a87f031d575d754d5e4875600b7

Contributor Author

zoecarver commented Mar 14, 2021

@swift-ci please test.

1 similar comment

Contributor Author

zoecarver commented Mar 14, 2021

@swift-ci please test.

Contributor

swift-ci commented Mar 14, 2021

Build failed
Swift Test OS X Platform
Git Sha - f89a6e5914065a87f031d575d754d5e4875600b7

Contributor Author

zoecarver commented Mar 15, 2021

@swift-ci please test macOS.

varungandhi-apple reviewed

View reviewed changes

test/Interop/Cxx/static/constexpr-static-member-var-errors.swift Outdated Show resolved Hide resolved

varungandhi-apple approved these changes

View reviewed changes

Contributor Author

zoecarver commented Mar 17, 2021

It appears that Clang does not mark templates as invalid on Windows: https://godbolt.org/z/Mv5osj This means that we still crash on Windows because the compiler doesn't reject invalid decls. I added a special-case in tryCreateConstexprAccessor to prevent this specific crash, but that isn't a great general solution. I'm going to file a bug against Clang, to see if we can get this fixed on trunk (or find a way to tell clang to mark these decls as invalid, if this is the intended behavior).

zoecarver force-pushed the cxx/fix-invalid-decls branch from f89a6e5 to 52fd808 Compare

March 17, 2021 05:18

zoecarver commented

View reviewed changes

test/Interop/Cxx/static/constexpr-static-member-var-errors.swift Outdated

    
              // Clang does not properly mark decls as invalid on Windows, so we won't get an

              // error here.

              // XFAIL: OS=windows-msvc

Contributor Author

zoecarver Mar 17, 2021

While I added a special-case so that we no longer crash on Windows, I still can't enable this test, because Clang won't generate an error on Windows. @varungandhi-apple do you think it would be a good idea for me to add an additional test that just ensures we don't crash when importing ConstexprStaticMemberVarErrors (that would run on Windows)?

This comment has been minimized.

Sign in to view

1 similar comment

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Contributor Author

zoecarver commented Mar 17, 2021

@swift-ci please test macOS.

Contributor Author

zoecarver commented Mar 17, 2021

Here's a link to the bug: https://llvm.org/PR49618

Contributor Author

zoecarver commented Mar 17, 2021

@swift-ci please test Windows.

Contributor Author

zoecarver commented Mar 18, 2021

And here's the patch to fix this on Windows: https://reviews.llvm.org/D98904

zoecarver force-pushed the cxx/fix-invalid-decls branch from 52fd808 to d675dab Compare

March 19, 2021 15:49

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test.

Contributor

swift-ci commented Mar 19, 2021

Build failed
Swift Test OS X Platform
Git Sha - d675dab0345e89e98e4760cc5935eb1b7451b153

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test macOS.

Contributor

swift-ci commented Mar 19, 2021

Build failed
Swift Test OS X Platform
Git Sha - d675dab0345e89e98e4760cc5935eb1b7451b153

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test macOS.

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test Windows.


          [cxx-interop] Don't crash when importing invalid decl.

794c0ea

If we see an invalid decl, rather than potentially crashing, simply
bail.

zoecarver force-pushed the cxx/fix-invalid-decls branch from d675dab to 794c0ea Compare

March 19, 2021 18:38

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test Windows.

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test macOS.

Contributor Author

zoecarver commented Mar 19, 2021

@swift-ci please test Linux.

zoecarver merged commit 3b06438 into swiftlang:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels