[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. #117475

SahilPatidar · 2024-11-24T10:41:02Z

Apply the fix suggested by Lang Hames to address a crash in Clang-REPL that occurs during the execution of __run_exit_handlers when using dynamic libraries.

…aries

llvmbot · 2024-11-24T10:43:13Z

@llvm/pr-subscribers-clang

Author: None (SahilPatidar)

Changes

Apply the fix suggested by Lang Hames to address a crash in Clang-REPL that occurs during the execution of __run_exit_handlers when using dynamic libraries.

Full diff: https://github.com/llvm/llvm-project/pull/117475.diff

1 Files Affected:

(modified) clang/lib/Interpreter/Interpreter.cpp (+3-1)

diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 5dc67f6375098f..71299f7176e2fe 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -719,7 +719,9 @@ llvm::Error Interpreter::LoadDynamicLibrary(const char *name) {
 
   if (auto DLSG = llvm::orc::DynamicLibrarySearchGenerator::Load(
           name, DL.getGlobalPrefix()))
-    EE->getMainJITDylib().addGenerator(std::move(*DLSG));
+    // FIXME: Eventually we should put each library in its own JITDylib and
+    //        turn off process symbols by default.
+    EE->getProcessSymbolsJITDylib()->addGenerator(std::move(*DLSG));
   else
     return DLSG.takeError();

vgvassilev · 2024-11-24T13:37:44Z

It is worth adding a test here.

SahilPatidar · 2024-11-25T09:09:37Z

I have added a test, but it should only run on Darwin (macOS) platforms.

// REQUIRES: host-supports-jit

// RUN: cat %s | env SDKROOT=$(xcrun --show-sdk-path) DYLD_LIBRARY_PATH=%S/Inputs:$DYLD_LIBRARY_PATH clang-repl

%lib vec.dylib
#include <vector>
std::vector<int> v;
%quit

vgvassilev · 2024-11-25T11:20:11Z

I have added a test, but it should only run on Darwin (macOS) platforms.

// REQUIRES: host-supports-jit

// RUN: cat %s | env SDKROOT=$(xcrun --show-sdk-path) DYLD_LIBRARY_PATH=%S/Inputs:$DYLD_LIBRARY_PATH clang-repl

%lib vec.dylib
#include <vector>
std::vector<int> v;
%quit

In this case we will need an extra requires clause. Also I do not think we need the xcrun. There was some way to make a library out of yaml, you can look in the list of tests what it was. Maybe that’s useful.

SahilPatidar · 2024-11-26T06:44:43Z

I came across the following setup:

// REQUIRES: host-supports-jit

// RUN: rm -rf %t
// RUN: mkdir -p %t
//
// RUN: split-file %s %t
//
// RUN: %clang++ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
// RUN: %clang++ -shared %t/vec.o -o %t/vec.dylib
//
// RUN: cat %t/Test.cpp | DYLD_LIBRARY_PATH=%t:$DYLD_LIBRARY_PATH clang-repl

//--- vec.cpp
#include <vector>

//--- Test.cpp
%lib vec.dylib
#include <vector>
std::vector<int> v;
%quit

vgvassilev · 2024-11-26T06:50:01Z

I came across the following setup:

// REQUIRES: host-supports-jit

// RUN: rm -rf %t
// RUN: mkdir -p %t
//
// RUN: split-file %s %t
//
// RUN: %clang++ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
// RUN: %clang++ -shared %t/vec.o -o %t/vec.dylib
//
// RUN: cat %t/Test.cpp | DYLD_LIBRARY_PATH=%t:$DYLD_LIBRARY_PATH clang-repl

//--- vec.cpp
#include <vector>

//--- Test.cpp
%lib vec.dylib
#include <vector>
std::vector<int> v;
%quit

Hm, okay, would that work on linux? If so, then I guess we can follow that pattern.

SahilPatidar · 2024-11-26T08:52:13Z

Would it be okay if I push the test for the Linux modification and check the results here? I don’t have access to a Linux x86 setup for testing.

vgvassilev · 2024-11-26T11:43:50Z

Would it be okay if I push the test for the Linux modification and check the results here? I don’t have access to a Linux x86 setup for testing.

Yes, that's what the pre-merge checks are for ;)

vgvassilev

LGTM!

vgvassilev · 2024-11-27T09:07:18Z

clang/test/Interpreter/crash.cpp

+#include <vector>
+
+//--- Test.cpp
+%lib vec.dylib


Suggested change

%lib vec.dylib

%lib vec.so

yuxuanchen1997 · 2024-12-13T16:32:32Z

This blocks some CI jobs on our end because we don't have a systemwide libc++ or libstdc++ installed. As a general rule, you can't just #include <vector> in clang filecheck tests. Because there's no requirement that the host have put a C++ standard library in the standard locations.

You can see how this is done in edbc6e9. You have to add a file like this: https://github.com/llvm/llvm-project/blob/main/clang/test/Modules/Inputs/PR28752/vector and pass something like -nostdsysteminc -I%S/Inputs/PR28752 to clang.

yuxuanchen1997 · 2024-12-13T16:35:24Z

Also Interpreter/crash.cpp is not a good name for a test. Usually when fixing a bug, we name the test with the GitHub issue number. Like gh12345.cpp.

30ad53b introduced a new test that includes `<vector>` from the system include path without honoring environment variables that may provide the path to C++ standard library. This is not supported in some CI systems because we don't always have the C++ library in the standard location. The conventional way of doing includes in the test is through `Inputs` directory and pass it as an include path. The `vector` file included in this patch has been shortened, but I have verified that it works with this test. i.e. the clang repl crashes on this test in the same way if the fix in #117475 is reverted.

yuxuanchen1997

I landed my previous fix but I found that this test to be more problematic than I originally anticipated. I left some comments. I may send more patches to fix it until I am satisfied.

yuxuanchen1997 · 2024-12-13T20:36:34Z

clang/test/Interpreter/crash.cpp

+// RUN: split-file %s %t
+//
+// RUN: %clang++ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
+// RUN: %clang++ -shared %t/vec.o -o %t/vec.so


This invokes the system linker. From the build system's point of view, it's almost never correct to invoke the system linker without the environment variables $LDFLAGS and $LIBS. In this case, you expect to make the shared library to link against the default libs.

I am thinking of changing this line to read

// RUN: %clang++ -shared %t/vec.o -o %t/vec.so $LDFLAGS $LIBS

Not particularly satisfied with this approach though.

I also think that your vec.o and vec.so are both empty after my previous fix. This suggests that the two steps here may not even be needed at all for the sake of testing this fix.

Perhaps we should use this yaml-to-so tool to build the library.

Perhaps we should use this yaml-to-so tool to build the library.

Can you point me to the tool? A naive search returned nothing.

If there's no quick solution, I would propose to revert the testing portion of this patch. This has been blocking CI on our end for a couple of days. No other tests in llvm repo invokes system linker like this one.

The tools is called yaml2obj. Alternatively we can upload the binary file which is my least favorite but still should get you going...

yuxuanchen1997 · 2024-12-13T20:47:36Z

clang/test/Interpreter/crash.cpp

+// RUN: %clang++ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
+// RUN: %clang++ -shared %t/vec.o -o %t/vec.so
+//
+// RUN: cat %t/Test.cpp | LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH clang-repl


How does clang-repl respond to #include <vector> if the path to the library is not provided on the command line? IIUC, this will continue to search for a systemwide installation of the C++ standard library. If that's the case, this continues to be wrong.

vgvassilev · 2024-12-14T06:58:20Z

@SahilPatidar, could you help us with this?

yuxuanchen1997 · 2024-12-14T18:05:24Z

@SahilPatidar, could you help us with this?

Can we temporarily disable this test when @SahilPatidar is looking for a solution? I am fine with a solution that doesn't invoke the system ld.

…mic libraries. (#117475)" This reverts commit 30ad53b as it breaks systems that don't have a systemwide libc++ or libstdc++ installed. It should be rewritten to not invoke the system linker. In the meanwhile, reverting to unblock the bots.

dcci · 2024-12-14T18:31:25Z

Given the fix forward didn't work, I reverted this to unblock

commit 61ab36a3e226df32855286dd31a2c3859800475d (HEAD -> main, upstream/main)
Author: Davide Italiano <[email protected]>
Date:   Sat Dec 14 18:25:50 2024 +0000

    Revert "[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. (#117475)"
    
    This reverts commit 30ad53b92cec0cff9679d559edcc5b933312ba0c as it breaks
    systems that don't have a systemwide libc++ or libstdc++ installed. It should
    be rewritten to not invoke the system linker. In the meanwhile, reverting
    to unblock the bots.

commit 9ddcaed3a64c2a187a0cfff4ba8f989c665ae1e5
Author: Davide Italiano <[email protected]>
Date:   Sat Dec 14 18:25:02 2024 +0000

    Revert "[Clang] Interpreter test should not depend on system header (#119903)"
    
    This reverts commit 8ab6912831277d87838518c5f775f79d14616860.

as it seems to need some amount of work. Feel free to ping us so we can try this patch once you have something ready to confirm it fixes the build.

@SahilPatidar

SahilPatidar · 2024-12-18T10:02:48Z

Sorry for the delay. We'll come up with a solution, or perhaps we can use a direct binary as suggested by @vgvassilev, since we want to avoid invoking the system linker.

[Clang-REPL] Fix crash during __run_exit_handlers with dynamic libr…

Loading
Loading status checks…

559a2d0

…aries

llvmbot added clang clang:frontend labels Nov 24, 2024

SahilPatidar requested a review from vgvassilev November 24, 2024 11:24

Add test to verify fix for crash

Loading
Loading status checks…

86210ed

vgvassilev approved these changes Nov 27, 2024

View reviewed changes

Fix minor typo

Loading
Loading status checks…

89015b3

vgvassilev merged commit 30ad53b into llvm:main Dec 10, 2024
8 checks passed

yuxuanchen1997 mentioned this pull request Dec 13, 2024

[Clang] Interpreter test should not depend on system header #119903

Merged

yuxuanchen1997 reviewed Dec 13, 2024

View reviewed changes

yuxuanchen1997 mentioned this pull request Dec 14, 2024

Temporarily disable test clang/Interpreter/crash.cpp #119978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. #117475

[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. #117475

SahilPatidar commented Nov 24, 2024

llvmbot commented Nov 24, 2024

vgvassilev commented Nov 24, 2024

SahilPatidar commented Nov 25, 2024

vgvassilev commented Nov 25, 2024

SahilPatidar commented Nov 26, 2024

vgvassilev commented Nov 26, 2024

SahilPatidar commented Nov 26, 2024

vgvassilev commented Nov 26, 2024

vgvassilev left a comment

vgvassilev Nov 27, 2024

yuxuanchen1997 commented Dec 13, 2024 •

edited

Loading

yuxuanchen1997 commented Dec 13, 2024

yuxuanchen1997 left a comment

yuxuanchen1997 Dec 13, 2024

vgvassilev Dec 13, 2024

yuxuanchen1997 Dec 13, 2024

yuxuanchen1997 Dec 13, 2024

vgvassilev Dec 14, 2024 •

edited

Loading

yuxuanchen1997 Dec 13, 2024

vgvassilev commented Dec 14, 2024

yuxuanchen1997 commented Dec 14, 2024 •

edited

Loading

dcci commented Dec 14, 2024 •

edited

Loading

SahilPatidar commented Dec 18, 2024

[Clang-REPL] Fix crash during __run_exit_handlers with dynamic libraries. #117475

[Clang-REPL] Fix crash during __run_exit_handlers with dynamic libraries. #117475

Conversation

SahilPatidar commented Nov 24, 2024

llvmbot commented Nov 24, 2024

vgvassilev commented Nov 24, 2024

SahilPatidar commented Nov 25, 2024

vgvassilev commented Nov 25, 2024

SahilPatidar commented Nov 26, 2024

vgvassilev commented Nov 26, 2024

SahilPatidar commented Nov 26, 2024

vgvassilev commented Nov 26, 2024

vgvassilev left a comment

Choose a reason for hiding this comment

vgvassilev Nov 27, 2024

Choose a reason for hiding this comment

yuxuanchen1997 commented Dec 13, 2024 • edited Loading

yuxuanchen1997 commented Dec 13, 2024

yuxuanchen1997 left a comment

Choose a reason for hiding this comment

yuxuanchen1997 Dec 13, 2024

Choose a reason for hiding this comment

vgvassilev Dec 13, 2024

Choose a reason for hiding this comment

yuxuanchen1997 Dec 13, 2024

Choose a reason for hiding this comment

yuxuanchen1997 Dec 13, 2024

Choose a reason for hiding this comment

vgvassilev Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

yuxuanchen1997 Dec 13, 2024

Choose a reason for hiding this comment

vgvassilev commented Dec 14, 2024

yuxuanchen1997 commented Dec 14, 2024 • edited Loading

dcci commented Dec 14, 2024 • edited Loading

SahilPatidar commented Dec 18, 2024

[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. #117475

[Clang-REPL] Fix crash during `__run_exit_handlers` with dynamic libraries. #117475

yuxuanchen1997 commented Dec 13, 2024 •

edited

Loading

vgvassilev Dec 14, 2024 •

edited

Loading

yuxuanchen1997 commented Dec 14, 2024 •

edited

Loading

dcci commented Dec 14, 2024 •

edited

Loading