Remove 'declspec's on Windows as they are currently inconsistent #42

[aytey]

This is a "hack" to fix the build issues as reported in #42. While I can't say these changes are "better", currently nothing sets EXPORT_API, which then means a lot of psychec's code ends-up with symbols having "incorrect" declspecs (leading to link-time issues).

I'd hoped that doing ninja install would remove the need for manually copying the DLL in-place:

• Compiling on Windows? #42 (comment)

but the "new" branch still expects to find psychecsolver-exe (from "original"), which means you can't do the install, so I can't really test that theory.

Signed-off-by: Andrew V. Jones [email protected]

[ltcmelo]

@andrewvaughanj I don't have a windows system, but I feel that this change is suspicious: I've used the #ifdef scheme in other projects and, as you can see in the // comment , the snippet comes from GCC's wiki.

You say that "nothing sets EXPORT_API", which makes sense. In "classic" windows development, this #define would go on a VS's project settings; here, I suppose that the proper solution is to have cmake pass -DEXPORT_API_ on windows builds.

Have you tried this, by any chance?

[aytey]

If you set EXPORT_API (e.g., as a part of CFLAGS to cmake) then everything will be __declspec(dllexport) -- this could be a problem if someone tries to use that symbol from a DLL. That is, users of a symbol should really be doing dllimport and "definers" should be doing dllexport.

I'll try it though, but it doesn't feel right ...

[aytey]

Yeah, no worky:

vuljaw@UK15132NB /d/dev/clones/psychec/master/build$ CXXFLAGS="-DEXPORT_API" CFLAGS="-DEXPORT_API" cmake -G Ninja -DCMAKE_INSTALL_PREFIX=$(readlink -f ../root) ..
vuljaw@UK15132NB /d/dev/clones/psychec/master/build$ ninja
[73/76] Linking CXX shared library C/msys-psychecfe.dll
FAILED: C/msys-psychecfe.dll C/libpsychecfe.dll.a
: && /mingw64/bin/c++.exe -DEXPORT_API   -shared -Wl,--enable-auto-import -o C/msys-psychecfe.dll -Wl,--out-implib,C/libpsychecfe.dll.a -Wl,--major-image-version,0,--minor-image-version,0 C/CMakeFiles/psychecfe.dir/Compilation.cpp.o C/CMakeFiles/psychecfe.dir/LanguageDialect.cpp.o C/CMakeFiles/psychecfe.dir/LanguageExtensions.cpp.o C/CMakeFiles/psychecfe.dir/Managed.cpp.o C/CMakeFiles/psychecfe.dir/MemoryPool.cpp.o C/CMakeFiles/psychecfe.dir/SemanticModel.cpp.o C/CMakeFiles/psychecfe.dir/SyntaxTree.cpp.o C/CMakeFiles/psychecfe.dir/Unparser.cpp.o C/CMakeFiles/psychecfe.dir/parser/Binder.cpp.o C/CMakeFiles/psychecfe.dir/parser/DiagnosticsReporter_Lexer.cpp.o C/CMakeFiles/psychecfe.dir/parser/DiagnosticsReporter_Parser.cpp.o C/CMakeFiles/psychecfe.dir/parser/Keywords.cpp.o C/CMakeFiles/psychecfe.dir/parser/LexedTokens.cpp.o C/CMakeFiles/psychecfe.dir/parser/Lexer.cpp.o C/CMakeFiles/psychecfe.dir/parser/LineDirective.cpp.o C/CMakeFiles/psychecfe.dir/parser/Parser.cpp.o C/CMakeFiles/psychecfe.dir/parser/Parser_Common.cpp.o C/CMakeFiles/psychecfe.dir/parser/Parser_Declarations.cpp.o C/CMakeFiles/psychecfe.dir/parser/Parser_Expressions.cpp.o C/CMakeFiles/psychecfe.dir/parser/Parser_Statements.cpp.o C/CMakeFiles/psychecfe.dir/parser/ParseOptions.cpp.o C/CMakeFiles/psychecfe.dir/parser/PreprocessorOptions.cpp.o C/CMakeFiles/psychecfe.dir/parser/TypeChecker.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxDisambiguator.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxHolder.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxLexeme.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxLexemes.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxNamePrinter.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxNode.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxNodeList.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxToken.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxUtilities.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxVisitor.cpp.o C/CMakeFiles/psychecfe.dir/syntax/SyntaxWriterDOTFormat.cpp.o C/CMakeFiles/psychecfe.dir/names/DeclarationName.cpp.o C/CMakeFiles/psychecfe.dir/names/DeclarationNames.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestBinder.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestFrontend.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestParser.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestParser_0000_0999.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestParser_1000_1999.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestParser_2000_2999.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestParser_3000_3999.cpp.o C/CMakeFiles/psychecfe.dir/tests/TestTypeChecker.cpp.o  common/libpsychecommon.dll.a && :
D:/DEV/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C/CMakeFiles/psychecfe.dir/syntax/SyntaxNamePrinter.cpp.o: in function `psy::C::SyntaxNamePrinter::print(psy::C::SyntaxNode const*, psy::C::SyntaxNamePrinter::Style, std::ostream&)':
D:\dev\clones\psychec\master\build/../C/syntax/SyntaxNamePrinter.cpp:117: undefined reference to `psy::operator<<(std::ostream&, psy::LinePosition const&)'
D:/DEV/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:\dev\clones\psychec\master\build/../C/syntax/SyntaxNamePrinter.cpp:120: undefined reference to `psy::operator<<(std::ostream&, psy::LinePosition const&)'
D:/DEV/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C/CMakeFiles/psychecfe.dir/tests/TestFrontend.cpp.o: in function `psy::C::TestFrontend::parse(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, psy::C::TestFrontend::Expectation, psy::C::SyntaxTree::SyntaxCategory)':
D:\dev\clones\psychec\master\build/../C/tests/TestFrontend.cpp:138: undefined reference to `psy::operator<<(std::ostream&, psy::Diagnostic const&)'
collect2.exe: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

[aytey]

Ah! Those operators don't have the export set ... let me try that for a second!

[ltcmelo]

I see… but in your approach here, CXXFLAGS="-DEXPORT_API" will hold for the build of every lib/(sub)lib of pscyhec, which is indeed wrong. The actual error is the general EXPORT_API macro.

If you take a look at the structure, there's a hierarchy of cmake files CMakeLists.txt where each (sub) "API header" relies on specific API macros, e.g., PSY_C_API, PLUGIN_API, and PSY_API. I'd say that "export" macros should correspond to those, i.e., EXPORT_C_API, EXPORT_PLUGIN_API, and EXPORT_API.

Then, in common's cmake file, we'd have -DEXPORT_API while in C's cmake file there's nothing about -DEXPORT_API but there's -DEXPORT_C_API. This way, the export/import relation is consistent.

[aytey]

Yeah, what you wrote makes sense ... we don't want one EXPORT, we want as many EXPORTs as we have DLLs, and then the each submodule should set the EXPORT only for the module it is currently building. That way, the importers get dllimport and the "exporters" use dllexport (which aligns with what I was trying to say in #43 (comment)).

I think there's also an issue that:

• std::ostream& operator<<(std::ostream& os, const Diagnostic& diagnostic); and
• std::ostream& operator<<(std::ostream& os, const LinePosition& pos);

aren't "tagged" at all.

However, even labelling those functions as PSY_API doesn't get the build to work.

I can try the per-module EXPORTs (but it isn't a high priority for me); maybe @geekrelief could try?

[aytey]

plugin-api probably doesn't need to be separate, because it goes into psychecfe.

[aytey]

Closing.

See: #44