Skip to content
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

Add Missing C++ Annotations to Control Symbol Visibility #612

Closed
david-cattermole opened this issue Dec 29, 2020 · 6 comments · Fixed by #615
Closed

Add Missing C++ Annotations to Control Symbol Visibility #612

david-cattermole opened this issue Dec 29, 2020 · 6 comments · Fixed by #615

Comments

@david-cattermole
Copy link
Contributor

Hello David,

I have noticed that the flag --cxx-impl-annotations used on the command cxxbridge (from cxxbridge-cmd) does not seem to add annotations to all function and method declarations.

As a result of this problem I was only able to use cxx on Windows by compiling downstream libraries that linked to the Rust library by including the C++ source files. (You can see this in my (CMake based) https://github.com/david-cattermole/cxx-demo-example project. See the main branch with the hack included.)

Since GCC and Clang default to exporting all symbols, but MSVC does not, I suspect this was not caught by other users. Or perhaps it's because I'm using cxxbridge-cmd directly, or maybe it's caused by older Windows compilers (Visual Studio 2015 / MSVC 14).

To fix the missing annotations I have made a commit to my cxx fork; david-cattermole@363c18b. The fix will add annotations to all the function (and method) declarations needed (in my libraries - I could be missing others).

You can see my cxx-demo-example project on the shared-object-symbol-visibility branch which uses my forked cxx repo. This has been tested on both Linux GCC and Windows MSVC and it appears to work (see output for Linux/GCC and Windows/MSVC).

Would you like a PR? Or you can simply use my commit david-cattermole@363c18b to implement a better fix?

Thanks,
David Cattermole

@dtolnay
Copy link
Owner

dtolnay commented Dec 30, 2020

FYI @adetaylor — hopefully this matches your understanding of where cxx_impl_annotations should be going.
Mentioning #219, #231.

@dtolnay
Copy link
Owner

dtolnay commented Dec 30, 2020

Would you like a PR? Or you can simply use my commit david-cattermole@363c18b to implement a better fix?

It would be great to get a PR adding a CI job which builds our test suite in a way that is sensitive to these annotations.

david-cattermole added a commit to david-cattermole/cxx that referenced this issue Jan 2, 2021
Annotations for symbol visibility (such as `__declspec(dllexport)` and
`__attribute__((visibility("default")))`) is the primary use case.
Such annotations are expected to be needed on declarations (headers)
only, however forcing the annotations into the header file only
is causes missing symbols. Therefore the fix is to force the
annotations into both declaration (header files) and definition
(source files).

Original issue: dtolnay#612

PR with the fix for dtolnay#612: dtolnay#615
david-cattermole added a commit to david-cattermole/cxx-demo-example that referenced this issue Jan 2, 2021
This fixes missing symbols that occur in v1.0.25.
@david-cattermole
Copy link
Contributor Author

Hello @dtolnay,

I am still getting missing symbols using cxx v1.0.25 (which includes the changes from #615).

I have fixed the problem for my cxx-demo-example repo using this patch in my fork of cxx:
david-cattermole@d58c650

If you want to test it out, you can use this repo commit:
david-cattermole/cxx-demo-example@7817d31

I don't understand why you were only excluding the annotations from source files.

if !out.header {

As I understand it the annotations should only be included in the declarations (header file), but right now in my project if I change begin_function_definition to only run on in headers, like this:

fn begin_function_definition(out: &mut OutFile) {
    if out.header {
        if let Some(annotation) = &out.opt.cxx_impl_annotations {
            write!(out, "{} ", annotation);
        }
    }
}

... I will get missing symbols for ::rust::Box<MyTypeHere>::drop().

I am continuing to debug the underlying problem but the patch (david-cattermole@d58c650) will fix things (for my projects at least) since the auto-generated files are never intended to be edited, so the problem on definition vs declaration doesn't matter - it's just a human readable problem I guess.

Thank you,
David C

P.S. I'm still looking into the GitHub actions to trigger the problem via CI on Windows.
I could checkout https://github.com/david-cattermole/cxx-demo-example and compile it on Windows, however that's using my repo, and I expect everything should be contained in cxx, so I'm going to need to create a minimum reproducible build error (probably using CMake).

@dtolnay
Copy link
Owner

dtolnay commented Jan 3, 2021

I pulled that commit into 1.0.26. I don't have a good way to know where those annotations are supposed to go so I am happy to do whatever works for now.

@david-cattermole
Copy link
Contributor Author

Thanks. I have updated to cxx 1.0.26 and the problem of annotations seem to be fixed.


I am however having other symbol problems with the update to 1.0.26 (specifically rust::cxxbridge1::sliceInit);

LINK: command "C:\PROGRA~2\MICROS~2.0\VC\bin\X86_AM~1\link.exe /nologo @CMakeFiles\opencompgraph_tests.dir\objects1.rsp /out:opencompgraph_tests.exe /implib:opencompgraph_tests.lib /pdb:C:\Users\catte\dev\OpenCompGraphMaya\src\OpenCompGraph\build_windows\tests\opencompgraph_tests.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console ..\src\opencompgraph.lib ..\..\target\release\opencompgraph_rs.lib ws2_32.lib userenv.lib advapi32.lib shell32.lib msvcrt.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:opencompgraph_tests.exe.manifest" failed (exit code 1120) with the following output:
test_i.cpp.obj : error LNK2019: unresolved external symbol "void __cdecl rust::cxxbridge1::sliceInit(void *,void const *,unsigned __int64)" (?sliceInit@cxxbridge1@rust@@YAXPEAXPEBX_K@Z) referenced in function "public: __cdecl rust::cxxbridge1::Slice<unsigned int const >::Slice<unsigned int const >(unsigned int const *,unsigned __int64)" (??0?$Slice@$$CBI@cxxbridge1@rust@@QEAA@PEBI_K@Z)
opencompgraph_tests.exe : fatal error LNK1120: 1 unresolved externals
NMAKE : fatal error U1077: '"C:\Program Files\CMake\bin\cmake.exe"' : return code '0xffffffff'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\nmake.exe"' : return code '0x2'
Stop.

I am currently trying to reproduce the error in cxx-demo-example, but so far everything is working very well in a simpler test environment.
It's probably an error in my code and I am trying to debug it, but I thought I should let you know in case it rings any bells.

@david-cattermole
Copy link
Contributor Author

Scratch that, please ignore me.

I have discovered the issue and it is unrelated to cxx (I had forgot to update my the cxx version, so I was using the header from 1.0.26 with the cxx.cc file from 1.0.25 - hence causing missing symbol errors.).

david-cattermole added a commit to david-cattermole/OpenCompGraph that referenced this issue Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants