remove Boost.DLL dependency by doing DLL handling ourselves #1
Conversation
DyXel
force-pushed
the
program-defined-metafunctions-no-boost-dll
branch
from
7ce5cd1 to
fbdbaba
Compare
|
Another thing: Boost.DLL was probably resolving the absolute path of a DLL before opening, atm this doesn't do that, so in the CLI you now need to do This brings another point up: How to deal with the error logging? (I put some TODOs for now), I immediately identified the error because I had some debug statements to print whatever |
|
Finally, before I forget, we should probably be adding something akin to:
to mark these functions explicitly exported. A lot of programs define a macro that expands to either of those and they are usually called |
There was a problem hiding this comment.
Thank you for doing this.
I had forgotten to watch this repository, so I noticed 30 minutes late.
- Since this can always be provided, I removed the
_neverheader, and renamed the_boost_dlltoreflect_load_metafunction.h(simplifies the CLI, removes the need ofCPPFRONT_LOAD_METAFUNCTION_IMPL_HEADER).
This is great.
Although I'm not confident that Boost.DLL isn't ported to more than just Windows and Unix.
But this should help reduce the friction when considering for merging the feature upstream.
Another thing: Boost.DLL was probably resolving the absolute path of a DLL before opening, atm this doesn't do that, so in the CLI you now need to do
CPPFRONT_METAFUNCTION_LIBRARIES=./libmetafunctions.soinstead ofCPPFRONT_METAFUNCTION_LIBRARIES=libmetafunctions.so.
Boost.DLL depends on Boost.Filesystem, so it was probably using that.
__declspec(dllexport)on VS
Yeah, this probably won't work at all on Windows without it, considering its default.
The same is true for the emitted symbol, right?
So this is a macro that we should add to cpp2util.h.
| } | ||
| } | ||
|
|
||
| return {}; |
There was a problem hiding this comment.
This brings another point up: How to deal with the error logging? (I put some TODOs for now), I immediately identified the error because I had some debug statements to print whatever
dlerrorgave me, I think there is value in providing that to end-users but I wouldn't know how to integrate that with the rest of cppfront.
This is an important QoI issue that we should discuss with Herb.
I already have 3 different modules authoring metafunctions.
Just imagine how much worse this error message could get
if it listed all metafunctions authored in which libraries (https://cpp2.godbolt.org/z/vqMYYKv5e):
t: @ugh type = { }
main: () = { }
main.cpp2...
main.cpp2(2,1): error: unrecognized metafunction name: ugh
main.cpp2(2,1): error: (temporary alpha limitation) currently the supported names are: interface, polymorphic_base, ordered, weakly_ordered, partially_ordered, copyable, basic_value, value, weakly_ordered_value, partially_ordered_value, struct, enum, flag_enum, union, print
Yeah, it'll probably not work by default Edit: Special thanks to @edo9300 for fixing and testing windows' side of things (even mingw!) |
Properly take a std::string in those functions to ensure that the passed strings are actually properly null terminated.
Check the ``_WIN32`` macro rather than ``_MSC_VER`` to detect if we're targeting Windows, rather than checking if we're compiling using Visual Studio. Add ``function_cast`` to properly convert the function pointer retrieved from GetProcAddress when building under mingw.
|
I switched to compiling my modules with hidden visibility. With regards to commit 32063ad, |
The usage of that macro in that file is to check for compiler specific capabilities and features, so it's correct to check for Visual Studio. But for Windows targeting checking for |
|
Another thing to note regarding the Windows side of the changes. At the moment I could only test its behavior while building with mingw, as I could use a similar approach as on linux to have gcc export every symbol present in cppfront.exe (via |
|
It should be OK to only emit the metafunction symbol with a |
|
@DyXel Will you add an export macro to |
I'm wondering. |
Unfortunately that seems to be the only way, from https://learn.microsoft.com/en-us/cpp/build/reference/export-exports-a-function?view=msvc-170 |
Yes, I'll be on it a bit later, currently running some errands. |
Yeah, that's a bit unfortunate, I just tried to confirm if in theory those things would work under visual studio, (and after fixing header inclusions, damn you NOMINMAX), by putting strategic __declspec(dllexport) to the 2 functions your sample program uses, I managed to get a working dll that could be then used by cppfront (and it required even less steps)
|
I suppose you did something like this: That works only in the global namespace. |
CPPFRONTAPI should be used to expose c++ functions that would be used by metafunctions. The question remains how to mark cpp2 code with this during lowering, at the moment manually marking the generated files with it makes Windows work. CPP2_C_API is emitted when generating the "entrypoint" for each metafunction by making it extern "C".
I did it in a way more hacky way, direclty putting the export in the generated cpp files, specifically here and here, and in the generated entrypoint |
At some point those strings will need to be reported anyways, and its unbearable to test without zero error logging.
There was a problem hiding this comment.
Sorry for the delay, I was trying to understand an error.
I have a corner case where a library that uses a metafunction in another library won't load.
Note that it worked with the implementation using Boost.DLL.
In this case, libmetafunctions.so depends on libutilities.so.
When failing to open libmetafunctions.so,
the error is that the symbol that libutilities.so defines is undefined.
[johel@sundown source]$ cat utilities.cpp2
printer: (inout t: cpp2::meta::type_declaration) = {
t.add_member($R"(print: (s) = std::cout << s;)");
}
[johel@sundown source]$ cat metafunctions.cpp2
auto printer(cpp2::meta::type_declaration &t) -> void;
greeter: (inout t: cpp2::meta::type_declaration) = {
t.printer();
t.add_member($R"(say_hi: () = print("Hello, world!\nFrom (t.name())$\n");)");
}
[johel@sundown source]$ cat main.cpp2
my_class: @greeter type = {}
my_struct: @greeter type = {}
main: () = {
my_class().say_hi();
my_struct().say_hi();
}
[johel@sundown source]$ g++ -std=c++20 -o cppfront.cpp.o -c cppfront.cpp
[johel@sundown source]$ g++ -Wl,--export-dynamic -rdynamic cppfront.cpp.o -o cppfront
[johel@sundown source]$ ./cppfront utilities.cpp2
utilities.cpp2... ok (all Cpp2, passes safety checks)
[johel@sundown source]$ g++ -std=c++20 -fPIC -o utilities.cpp.o -c utilities.cpp
[johel@sundown source]$ g++ -fPIC -shared -Wl,-soname,libutilities.so -o libutilities.so utilities.cpp.o
[johel@sundown source]$ ./cppfront metafunctions.cpp2
metafunctions.cpp2... ok (mixed Cpp1/Cpp2, Cpp2 code passes safety checks)
[johel@sundown source]$ g++ -std=c++20 -fPIC -o metafunctions.cpp.o -c metafunctions.cpp
[johel@sundown source]$ g++ -fPIC -shared -Wl,-soname,libmetafunctions.so -o libmetafunctions.so metafunctions.cpp.o
[johel@sundown source]$ CPPFRONT_METAFUNCTION_LIBRARIES=utilities.so:libmetafunctions.so ./cppfront main.cpp2
main.cpp2...Error while looking up DLL symbol 'cpp2_metafunction_greeter': utilities.so: undefined symbol: _cpp2_metafunction_greeter
Error while loading DLL 'libmetafunctions.so': libmetafunctions.so: undefined symbol: _Z7printerRN4cpp24meta16type_declarationE
main.cpp2(2,1): error: unrecognized metafunction name: greeter
main.cpp2(2,1): error: (temporary alpha limitation) currently the supported names are: interface, polymorphic_base, ordered, weakly_ordered, partially_ordered, copyable, basic_value, value, weakly_ordered_value, partially_ordered_value, struct, enum, flag_enum, union, print
I can't seem to push to your branch.
Here's a patch for an issue I found:
commit 717f725132a9e54dc860105541f883808279fe7d
Author: Johel Ernesto Guerrero Peña <[email protected]>
Date: Wed Dec 27 14:59:20 2023 -0400
fix(reflect): guard clean up
diff --git a/source/reflect_load_metafunction.h b/source/reflect_load_metafunction.h
index a8b1b28..26a0930 100644
--- a/source/reflect_load_metafunction.h
+++ b/source/reflect_load_metafunction.h
@@ -32,13 +32,14 @@ public:
~dll() noexcept
{
- if(handle_ == nullptr);
- return;
+ if(handle_ != nullptr)
+ {
#ifdef _WIN32
- FreeLibrary(static_cast<HMODULE>(handle_));
+ FreeLibrary(static_cast<HMODULE>(handle_));
#else
- dlclose(handle_);
+ dlclose(handle_);
#endif // _WIN32
+ }
}
// Uncopyable
|
|
I am wondering if it would work if you loaded the DLLs in different order, also, I think the resolution has to do with
|
|
Looking closer at the documentation, I think using |
They don't. |
I suspect it's going to be a common use case for library writers. |
In that case in order to properly support these use-cases we would need to make the loading orthogonal (like modules!), it would require a bit of re-design, but basically I believe this is how it would work:
|
Actually, the Boost.DLL implementation also fails So this issue isn't something that should block this PR. I do note that switching to the default visibility preset did make it work for me. Why does visibility make a difference here? |
|
To be honest, I am not that well-versed in how the symbol look-up is supposed to work, but according to this: |
Just like this one, the previous implementation loaded each library one at a time. |
|
Then no idea why changing the global visibility would affect how symbols are resolved. And honestly not the most thrilling rabbit-hole to go down to 😅 |
There was a problem hiding this comment.
Thank you both for this!
Since I can't push to the branch, I'll merge this now and work on integrating the error reporting.
We also need to report #1 (comment) upstream, which blocks VS.
Do all declarations in cpp2reflect.h need to be manually prepended CPPFRONTAPI to work on VS (what about the definitions)?
It should be possible to write a script to post-process the generated header.
That would be a good place to document the problem and for Herb to notice the need to extend Cpp2.
After all that, I should update the upstream's opening comment
to show how this PR greatly simplified the requirements
by removing the dependency on Boost.DLL.
|
The windows CI failed with (https://github.com/JohelEGP/cppfront/actions/runs/7342334703/job/19991420610#step:5:68):
|
I think yes, at least the thing you want to be accessible for metafunction usage, so I am guessing most of |
oops, my bad. |
It turns out that I didn't update my branch https://github.com/JohelEGP/cppfront/tree/cpp_modules I also opened the related llvm/llvm-project#76526. |
|
Actually, that's not exactly it. It's that It's the same issue as hsutter#907 (comment):
|
That's because you were trying to use a function, right? |
From what I understand, at the ABI level there should be no difference between reference parameters and pointer parameters, yet, I was getting segfaults when trying to call the function with a reference, and the backtrace didn't tell me much of what was going on. It could have been a bad cast on my side but not sure. The safest and most portable way is to type-erase when dealing with C APIs so I'd say you can (and should) go with:
|
Follow-up to hsutter#907 (comment)
Implement minimal
dllclass that acts as "almost drop-in replacement" for Boost.DLL stuff. Tested only on Linux with g++.Some notes:
_neverheader, and renamed the_boost_dlltoreflect_load_metafunction.h(simplifies the CLI, removes the need ofCPPFRONT_LOAD_METAFUNCTION_IMPL_HEADER).shared_ptrrather than by the class implementation (dunno if Boost.DLL did that, butstd::functionwas complaining about needing to copy so I am guessing some ref-counting was happening already behind the scenes).