- 
                Notifications
    You must be signed in to change notification settings 
- Fork 35
Add llvm libunwind callback to suppress exceptions on apple silicon #254
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
base: main
Are you sure you want to change the base?
Conversation
| clang-tidy review says "All clean, LGTM! 👍" | 
    
      
        1 similar comment
      
    
  
    | clang-tidy review says "All clean, LGTM! 👍" | 
| clang-tidy review says "All clean, LGTM! 👍" | 
| Codecov ReportAttention: Patch coverage is  
 
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
+ Coverage   77.31%   77.36%   +0.04%     
==========================================
  Files           9        9              
  Lines        3685     3693       +8     
==========================================
+ Hits         2849     2857       +8     
  Misses        836      836              
 ... and 2 files with indirect coverage changes 
 🚀 New features to boost your workflow:
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
        
          
                lib/Interpreter/Compatibility.h
              
                Outdated
          
        
      | #include "llvm/Support/Casting.h" | ||
| #include "llvm/Support/Path.h" | ||
|  | ||
| /home/maximus/cppyy-interop-dev/CppInterOp/llvm-project/compiler-rt/lib/orc/macho_platform.cpp | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: expected unqualified-id [clang-diagnostic-error]
/home/maximus/cppyy-interop-dev/CppInterOp/llvm-project/compiler-rt/lib/orc/macho_platform.cpp
^| clang-tidy review says "All clean, LGTM! 👍" | 
| clang-tidy review says "All clean, LGTM! 👍" | 
a7ae7ef    to
    be0085b      
    Compare
  
    | clang-tidy review says "All clean, LGTM! 👍" | 
| clang-tidy review says "All clean, LGTM! 👍" | 
    
      
        1 similar comment
      
    
  
    | clang-tidy review says "All clean, LGTM! 👍" | 
| @mcbarton, our cache sizes nowadays allow for having 1-2 osx-x86 builds. Are they easy to bring back? | 
| 
 @vgvassilev Yes, they are easy to bring back. I'll put in a PR which brings them back. | 
| 
 Nice! Thanks for the speed-of-light reply and PR! | 
| clang-tidy review says "All clean, LGTM! 👍" | 
| clang-tidy review says "All clean, LGTM! 👍" | 
| clang-tidy review says "All clean, LGTM! 👍" | 
c3c8510    to
    3a6db23      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
        
          
                lib/Interpreter/CppInterOp.cpp
              
                Outdated
          
        
      | dlsym(RTLD_DEFAULT, "__unw_remove_find_dynamic_unwind_sections")) | ||
| unw_remove_find_dynamic_unwind_sections(find_dynamic_unwind_sections); | ||
| #endif | ||
| sInterpreter.release(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: the value returned by this function should be used [bugprone-unused-return-value]
      sInterpreter.release();
      ^Additional context
lib/Interpreter/CppInterOp.cpp:109: cast the expression to void to silence this warning
      sInterpreter.release();
      ^| This PR is stale because it has been open for 90 days with no activity. | 
3a6db23    to
    e20f5df      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #include "llvm/ADT/StringRef.h" | ||
| #include "llvm/ADT/Twine.h" | ||
| #include "llvm/BinaryFormat/MachO.h" | ||
| #include "llvm/Config/llvm-config.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header MachO.h is not used directly [misc-include-cleaner]
| #include "llvm/Config/llvm-config.h" | |
| #include "llvm/Config/llvm-config.h" | 
| #include "llvm/ExecutionEngine/JITSymbol.h" | ||
| #include "llvm/ExecutionEngine/Orc/LLJIT.h" | ||
| #include "llvm/Object/MachO.h" | ||
| #include "llvm/Support/Casting.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header MachO.h is not used directly [misc-include-cleaner]
| #include "llvm/Support/Casting.h" | |
| #include "llvm/Support/Casting.h" | 
d192285    to
    02ca483      
    Compare
  
    | Hey @aaronj0 was interested in a use case revolving around this ! Would like to know if you would find some time to address whatever remains here ? | 
| I think there was something that still needed to be done on the apple side. cc: @lhames. | 
| From memory this should be sufficient, but it's been a while since I looked at this. I'd check the error logs but it looks like they've expired. Is there a way to re-run the tests? | 
| 
 I just rebased the PR on latest main if that would help | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #endif | ||
| } | ||
|  | ||
| TEST(InterpreterTest, InterpreterExceptions) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| TEST(InterpreterTest, InterpreterExceptions) { | |
| TESTstatic (InterpreterTest, InterpreterExceptions) { | 
| << "Failed to catch exceptions in interpreter"; | ||
| } | ||
|  | ||
| TEST(InterpreterTest, InterpreterExceptionsCompiledCode) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]
| TEST(InterpreterTest, InterpreterExceptionsCompiledCode) { | |
| TESTstatic (InterpreterTest, InterpreterExceptionsCompiledCode) { | 
| The tests on OSX fail with this https://github.com/compiler-research/CppInterOp/actions/runs/14876386113/job/41774415677?pr=254#step:9:457 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
| #include "llvm/ADT/StringRef.h" | ||
| #include "llvm/ADT/Twine.h" | ||
| #include "llvm/BinaryFormat/MachO.h" | ||
| #include "llvm/Config/llvm-config.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header MachO.h is not used directly [misc-include-cleaner]
| #include "llvm/Config/llvm-config.h" | |
| #include "llvm/Config/llvm-config.h" | 
| #include "llvm/ExecutionEngine/JITSymbol.h" | ||
| #include "llvm/ExecutionEngine/Orc/LLJIT.h" | ||
| #include "llvm/Object/MachO.h" | ||
| #include "llvm/Support/Casting.h" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header MachO.h is not used directly [misc-include-cleaner]
| #include "llvm/Support/Casting.h" | |
| #include "llvm/Support/Casting.h" | 
| @aaronj0, can you rebase this PR? | 
Adding fix from
This adds the
find_dynamic_unwind_sectionscallback, used by libunwind to fix thelibc++abi: terminating due to uncaught exception of type inton apple silicon. This returns fake_mach_header as the header for all JIT'd code.The checks are performed during library loading and unloading time, and so have been added during interpreter creation and destruction called in
CppInterOp.cpp, via the leakedcompat::Interpreterclass