At least the plugin used by the LibreOffice build (https://wiki.documentfoundation.org/Development/Clang_plugins) indirectly uses those members (through inline functions in LLVM/Clang include files in turn using them), but they are not exported by utils/extract_symbols.py on Windows, and accessing data across DLL/EXE boundaries on Windows is generally problematic.
Details
- Reviewers
chandlerc • tstellarAMD zturner john.brawn timshen - Commits
- rG17c7f703620f: Replace APFloatBase static fltSemantics data members with getter functions
rLLDB289647: Replace APFloatBase static fltSemantics data members with getter functions
rC289647: Replace APFloatBase static fltSemantics data members with getter functions
rCTE289647: Replace APFloatBase static fltSemantics data members with getter functions
rL289647: Replace APFloatBase static fltSemantics data members with getter functions
Diff Detail
- Repository
- rL LLVM
Event Timeline
One more came up now in the LibreOffice build after https://reviews.llvm.org/D26455 "Handle non-inlined clang::Type::getAs specializations in extract_symbols.py".
Doing this may allow the plugin to link against clang, but it may or may not behave correctly at runtime. MSVC unfortunately handles exported/imported data symbols differently to function symbols, and to get everything working properly the variable needs to be exported as a data symbol and annotated with __declspec(dllimport) in the source code. Using the value of the variable definitely doesn't work, but just taking the address of the variable (as is done with the fltSemantics) _may_ work though I can't remember exactly - I seem to remember that in some situations you get a different addresses for the same variable in exe and in the dll. The ideal solution is to refactor everything that declares class static data members in header files so that it doesn't, which has other benefits (it would make LLVM_LINK_LLVM_DYLIB work when building using MSVC, and would be the first step towards getting BUILD_SHARED_LIBS working), but that's also a huge amount of work.
One way to sidestep this problem would be to refactor the plugin to use the TargetInfo::getXXFormat() functions to get the fltSemantics, instead of accessing them directly, but I don't know if that's appropriate for whatever it is that the plugin is doing.
The plugin doesn't itself access those members at all; it is inline functions in the LLVM/Clang include files that cause the access.
At first I thought such data access across DLL/EXE boundaries wouldn't work at all with MSCV, and that I'd need to replace those exposed data members with getter functions across all of LLVM. Then I found it did work and vaguely remembered there'd been some MSVC improvements in that area in the past years. But probably it just happened to work by luck then.
Doing the replacement with getters for these specific members didn't appear to be that much work when I initially looked, so when you say that would overall be considered beneficial, anyway, I'll see to do that instead.
- Replaced the data members with getter functions of the same names (or should they better be prefixed with get*, say?).
- Made the data private to APFloat.cpp (so everything outside of it needs to go via the getters, even in the same library; somewhat arbitrarily kept everything inside APFloat.cpp accessing the data directly).
- Checked additional repos tools/clang and tools/clang/tools/extra (which each needs a corresponding patch, which I'll submit once the details of this one are discussed), projects/libcxxabi (which mentions those data members' mangled names in test/test_demangle.pass.cpp, but apparently only as sample test data, so doesn't need changing?), projects/libcxx, and project/compiler-rt (which are unaffected). Is there a list of more repos to check? (And how would commits for such an incompatible cross-repo change be done atomically?)
Is there a list of more repos to check? (And how would commits for such an incompatible cross-repo change be done atomically?)
Check out http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo.
Also if you like this, please make your voice heard. Whether or not we switch to this as our canonical repository is still yet to be decided. But ease of cross-repository commits is one of the selling points of the monorepo, and some have downplayed the importance of this feature.
I don't know how Windows DLL works, so I'll leave that part to others.
As far as I can tell, Clang actively uses these semantics, so I expect this change to break clang build. Do you have other patches to adjust other repositories? I don't know if it's necessary, but it's better to have them checked in in a single svn commit.
see my previous: "Checked additional repos tools/clang and tools/clang/tools/extra (which each needs a corresponding patch, which I'll submit once the details of this one are discussed), projects/libcxxabi (which mentions those data members' mangled names in test/test_demangle.pass.cpp, but apparently only as sample test data, so doesn't need changing?), projects/libcxx, and project/compiler-rt (which are unaffected). Is there a list of more repos to check? (And how would commits for such an incompatible cross-repo change be done atomically?)"
I don't know if it's necessary, but it's better to have them checked in in a single svn commit.
How do I do that? Any pointer?
I don't know anything about the code in question, but from a purely design perspective, it looks like fltSemantics is only forward declared in the header, and as such no external client can ever use a fltSemantics for anything meaningful anyway. So it looks like it's intended to be an opaque data structure only useful within the confines of APFloat.cpp.
If this is the case, why don't we remove the forward declarations entirely, and write something like: typedef void* FLT_SEMANTICS, get rid of all the getters and convert them back into data members, and have the data members declared as actual declared in APFloat.h as being of type FLT_SEMANTICS (feel free to choose a different name).
Returning pointers across DLL boundaries is fine, and this way you don 't even need to export anything.
Regarding Justin's comment on the mono repo, I grepped https://github.com/llvm-project/llvm-project/ for "APFloat", and found out that klee/lib/Core/Executor.cpp, clang-tools-extra/clang-tidy/misc/IncorrectRoundings.cpp, multiple files from lldb, and dragonegg do use these semantics, besides libcxx and clang.'
However, the github monorepo doesn't seem to hold all llvm projects, see https://llvm.org/svn/llvm-project/.
I'm not sure whether you should change them all together, or it's fine to leave some repositories broken in a very short period of time.
I don't know if it's necessary, but it's better to have them checked in in a single svn commit.
How do I do that? Any pointer?
If you use svn, just apply all patches in a single commit; or you can use the git monorepo check-in script, see http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo .
I'm not sure whether you should change them all together, or it's fine to leave some repositories broken in a very short period of time.
@mehdi_amini can probably speak to this, as he recently did a very large refactoring over LLVM.
I recommend using http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo .
And trying to build with cmake -DLLVM_ENABLE_PROJECTS=all
(We don't maintain Klee or dragonegg anymore, the -DLLVM_ENABLE_PROJECTS=all should get you what matters.).
I don't see how changing those static data members from type fltSemantics to fltSemantics* or void* would change anything wrt the original MSVC export issue. While code outside APFloat.cpp appears to indeed be only interested in opaque pointers to those objects, it still needs to obtain those pointers, and doesn't do so via the return values of existing functions. (What /could/ be changed in the patch is to make the IEEEhalf etc. functions return fltSemantics* instead of fltSemantics&; I'm dispassionate about that.)
Regarding Justin's comment on the mono repo, I grepped https://github.com/llvm-project/llvm-project/ for "APFloat", and found out that klee/lib/Core/Executor.cpp, clang-tools-extra/clang-tidy/misc/IncorrectRoundings.cpp, multiple files from lldb, and dragonegg do use these semantics, besides libcxx and clang.'
So besides the places I'd already covered in llvm, clang, and clang-tools-extra, I've now also covered lldb and klee (even though the latter isn't part of the monorepo's -DLLVM_ENABLE_PROJECTS=all). (I didn't find any hits in dragonegg?) See https://github.com/stbergmann/llvm-project/commit/f914d64f6ba3506d04ec8e74f859aea17a695497 for the full patch.
If you use svn, just apply all patches in a single commit; or you can use the git monorepo check-in script, see http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo .
But I can't upload such a single patch here in Phabricator, right?
If you use svn, just apply all patches in a single commit; or you can use the git monorepo check-in script, see http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-a-git-monorepo .
But I can't upload such a single patch here in Phabricator, right?
Sure you can!
(I updated the doc to mention the use of arc but you can also generate a textual diff if you prefer)
I think I may still be missing something. In this patch, the accessor functions are not marked dllexport either, so how does this solve the problem? I don't know how this extract_symbols.py works, does it have something to do with that?
Which doc is that? I've never felt the need to spend any amount of time in setting up and learning about arc, so I've only ever uploaded patches here via 'git diff -U999999'. However, when I try that with this combined patch, Phabricator complains: "Uploaded file is too large: current limit is 8M. To adjust this limit change 'upload_max_filesize' in php.ini."
Yes, the magic of selecting the symbols to export is done by extract_symbols.py. (In short, most function symbols are exported, but no data symbols are, and the general understanding is that exported data symbols are unreliable with MSVC; see earlier comments for details.)
I see. My mild preference would be to remove as many C++isms as possible from exported interfaces. So, in descending order of preference, this would be:
- Make it return a void* instead of a fltSemantics&
- Make these global free functions marked extern "C"
The first identifies at the contract level that fltSemantics is an opaque type and prevents any accidental forward declarations, and the second is just a nice-to-have for any exported function, especially when you're trying to do diagnostics and you don't want to deal with looking at mangled names.
Not going to block over either of these though.
That surprises me. Virtually all of include/clang and include/llvm is available to Clang plugins, and at least the LibreOffice plugin does use lots of that. Do you want to C-ify all that?
Ah, wasn't that hard after all to set up arc. :) So here's the full monorepo diff now.
I don't have anything against this patch as is, but I don't know about the MSVC issue so I won't be able to help here.
If you want a C-like API, you should extend the LLVM C API or write wrappers around the C++ API, I don't believe there is a wide agreement to "remove C++isms" from LLVM C++ APIs themselves (it is possible I misunderstood what you're suggesting here).
llvm/include/llvm/ADT/APFloat.h | ||
---|---|---|
143 ↗ | (On Diff #80382) | Functions name in LLVM usually starts with a verb (getters start with get). |
LGTM for APFloat.
You probably need to rebase, since I changed APFloat.h/cpp and APFloatTest.cpp yesterday.