Library autoloader #308

alexander-penev · 2024-07-10T17:28:53Z

No description provided.

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 12. Check the log or trigger a new build to see more.

lib/Interpreter/CppInterOp.cpp

codecov · 2024-07-10T17:40:19Z

Codecov Report

Attention: Patch coverage is 82.75862% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.30%. Comparing base (e0fa91e) to head (6cfe873).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   72.22%   74.30%   +2.07%     
==========================================
  Files           8        8              
  Lines        2963     3043      +80     
==========================================
+ Hits         2140     2261     +121     
+ Misses        823      782      -41

Files	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`74.56% <ø> (+6.91%)`	⬆️
lib/Interpreter/DynamicLibraryManager.cpp	`77.77% <60.00%> (+5.40%)`	⬆️
lib/Interpreter/CppInterOp.cpp	`78.79% <84.14%> (+0.26%)`	⬆️

... and 2 files with indirect coverage changes

Files	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.00% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`74.56% <ø> (+6.91%)`	⬆️
lib/Interpreter/DynamicLibraryManager.cpp	`77.77% <60.00%> (+5.40%)`	⬆️
lib/Interpreter/CppInterOp.cpp	`78.79% <84.14%> (+0.26%)`	⬆️

... and 2 files with indirect coverage changes

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

lib/Interpreter/DynamicLibraryManager.cpp

unittests/CppInterOp/TestSharedLib1/TestSharedLib1.h

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 14. Check the log or trigger a new build to see more.

lib/Interpreter/CppInterOp.cpp

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

include/clang/Interpreter/CppInterOp.h

lib/Interpreter/CppInterOp.cpp

lib/Interpreter/DynamicLibraryManagerSymbol.cpp

unittests/CppInterOp/CMakeLists.txt

vgvassilev · 2024-07-28T05:57:09Z

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

+  return nameForDlsym;
+}
+
+#include "../../lib/Interpreter/CppInterOp.cpp"


We probably should drop this.

We use from it functions that are not visible through the .h file ie. should be available only for the tests.

Okay, fair enough...

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

unittests/CppInterOp/TestSharedLib1/CMakeLists.txt

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

unittests/CppInterOp/TestSharedLib2/TestSharedLib2.h

unittests/CppInterOp/TestSharedLib3/TestSharedLib3.h

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 18. Check the log or trigger a new build to see more.

lib/Interpreter/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

lhames · 2024-08-02T04:09:30Z

A couple of quick thoughts for future upstreaming: we'd want to make this compatible with out-of-process execution, and it'd be great if we could re-use parts of it for MachO auto-linking.

We should consider whether SimpleExecutorDylibManager can be extended to handle all this.

vgvassilev · 2024-08-02T08:29:49Z

A couple of quick thoughts for future upstreaming: we'd want to make this compatible with out-of-process execution, and it'd be great if we could re-use parts of it for MachO auto-linking.

Thanks for the feedback, @lhames. That makes sense.

We should consider whether SimpleExecutorDylibManager can be extended to handle all this.

I was not aware such a thing existed. @SahilPatidar perhaps that's interesting in terms of your project, too.

github-actions

clang-tidy made some suggestions

lib/Interpreter/CppInterOp.cpp

github-actions

clang-tidy made some suggestions

github-actions · 2024-09-19T11:01:50Z

lib/Interpreter/CppInterOp.cpp

+    }
+
+    void setEnabled(bool enabled) {
+      Enabled = enabled;


warning: method 'isEnabled' can be made const [readability-make-member-function-const]

Suggested change

Enabled = enabled;

bool isEnabled() const {

github-actions · 2024-09-19T11:01:50Z

lib/Interpreter/CppInterOp.cpp

+      }
+
+      void materialize(std::unique_ptr<llvm::orc::MaterializationResponsibility> R) override {
+        if (!sAutoSG || !sAutoSG->isEnabled()) {


warning: function 'getName' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change

if (!sAutoSG || !sAutoSG->isEnabled()) {

[[nodiscard]] StringRef getName() const override {

vgvassilev · 2024-09-19T16:56:51Z

@aaronj0, do you have a clue why cppyy fails?

aaronj0 · 2024-09-20T09:09:51Z

@aaronj0, do you have a clue why cppyy fails?

cppyy does not fail, InterOp does not build on any of the jobs. This comes back to the point that CppInterOp is only built on the jobs tagged with "cppyy" not the first stage that only builds llvm.

On a preliminary look, it seems like on llvm19 we get:

error: no matching function for call to ‘llvm::orc::MaterializationResponsibility::notifyEmitted()’
 3470 |           llvm::cantFail(R->notifyEmitted());

and on 18 and 17, these "undefined reference to typeinfo for llvm::orc::DefinitionGenerator"

They also fail in some of the emscripten builds but I am more interested in the fact that they pass on llvm 17 and 18 with clang-repl on all platforms

Co-authored-by: Vassil Vassilev <[email protected]>

github-actions bot reviewed Jul 10, 2024

View reviewed changes

github-actions bot reviewed Jul 11, 2024

View reviewed changes

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Jul 28, 2024

View reviewed changes

vgvassilev reviewed Jul 28, 2024

View reviewed changes

github-actions bot reviewed Jul 30, 2024

View reviewed changes

github-actions bot reviewed Jul 31, 2024

View reviewed changes

github-actions bot reviewed Sep 6, 2024

View reviewed changes

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

alexander-penev force-pushed the AddLibraryLoader branch from 0686324 to c1dd45f Compare September 19, 2024 10:53

github-actions bot reviewed Sep 19, 2024

View reviewed changes

alexander-penev force-pushed the AddLibraryLoader branch from c1dd45f to a04706e Compare September 19, 2024 11:12

alexander-penev and others added 13 commits October 27, 2024 16:59

Library autoloader

d698b4e

Fix some clang-tidy suggested issues

290d7f5

Fix some clang-tidy suggested issues 2

530d8e8

Fix

101533c

Update CppInterOp.cpp

bde824b

Fix demangle

33fc684

Apply suggestions from code review

858d565

Co-authored-by: Vassil Vassilev <[email protected]>

Update DynamicLibraryManagerSymbol.cpp

3a95001

Update DynamicLibraryManagerSymbol.cpp

d0554c3

Update DynamicLibraryManagerTest.cpp

5c49ca7

Update CMakeLists.txt

cb85568

Fix demangle build

8e1c748

Cleanup

876169a

mcbarton force-pushed the AddLibraryLoader branch from a04706e to 876169a Compare October 27, 2024 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Library autoloader #308

Library autoloader #308

alexander-penev commented Jul 10, 2024

github-actions bot left a comment

codecov bot commented Jul 10, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

vgvassilev Jul 28, 2024

alexander-penev Jul 31, 2024

vgvassilev Jul 31, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

lhames commented Aug 2, 2024

vgvassilev commented Aug 2, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot Sep 19, 2024

github-actions bot Sep 19, 2024

vgvassilev commented Sep 19, 2024

aaronj0 commented Sep 20, 2024 •

edited

Loading

	if (!sAutoSG \|\| !sAutoSG->isEnabled()) {
	[[nodiscard]] StringRef getName() const override {

Library autoloader #308

Are you sure you want to change the base?

Library autoloader #308

Conversation

alexander-penev commented Jul 10, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev Jul 28, 2024

Choose a reason for hiding this comment

alexander-penev Jul 31, 2024

Choose a reason for hiding this comment

vgvassilev Jul 31, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

lhames commented Aug 2, 2024

vgvassilev commented Aug 2, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Sep 19, 2024

Choose a reason for hiding this comment

github-actions bot Sep 19, 2024

Choose a reason for hiding this comment

vgvassilev commented Sep 19, 2024

aaronj0 commented Sep 20, 2024 • edited Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

aaronj0 commented Sep 20, 2024 •

edited

Loading