This is an archive of the discontinued LLVM Phabricator instance.

Replace APFloatBase static fltSemantics data members with getter functions
ClosedPublic

Authored by sberg on Nov 15 2016, 7:38 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sberg updated this revision to Diff 77999.Nov 15 2016, 7:38 AM
sberg retitled this revision from to Export llvm::APFloatBase static fltSemantics data members in extract_symbols.py.
sberg updated this object.
sberg added a reviewer: john.brawn.
sberg added a subscriber: llvm-commits.

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".

john.brawn edited edge metadata.Nov 15 2016, 10:21 AM

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.

sberg updated this revision to Diff 78397.Nov 17 2016, 12:18 PM
sberg retitled this revision from Export llvm::APFloatBase static fltSemantics data members in extract_symbols.py to Replace APFloatBase static fltSemantics data members with getter functions.
sberg updated this object.
sberg added a reviewer: chandlerc.
  • 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?)
jlebar edited edge metadata.Nov 17 2016, 4:22 PM
jlebar added a subscriber: timshen.
jlebar added a subscriber: jlebar.Nov 17 2016, 4:25 PM

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.

sberg added a comment.Nov 24 2016, 1:00 AM

friendly ping

sberg added a comment.Dec 1 2016, 2:36 AM

friendly ping

RKSimon added a subscriber: RKSimon.Dec 1 2016, 2:44 PM
timshen edited edge metadata.Dec 1 2016, 5:11 PM

Adding @zturner, in the hope to get more understanding on DLLs.

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.

sberg added a comment.Dec 2 2016, 12:20 AM

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?

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?

zturner edited edge metadata.Dec 2 2016, 12:22 PM

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.

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?

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?)"

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.).

sberg added a comment.Dec 5 2016, 2:48 AM

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.

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.)

sberg added a comment.Dec 5 2016, 6:28 AM

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?

sberg added a comment.Dec 5 2016, 8:27 AM

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)

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."

sberg added a comment.Dec 5 2016, 8:30 AM

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?

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:

  1. Make it return a void* instead of a fltSemantics&
  2. 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.

sberg added a comment.Dec 5 2016, 11:57 PM

I see. My mild preference would be to remove as many C++isms as possible from exported interfaces.

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?

sberg updated this revision to Diff 80382.Dec 6 2016, 12:14 AM
sberg edited edge metadata.

Ah, wasn't that hard after all to set up arc. :) So here's the full monorepo diff now.

Any further input? If not, I'm going to land this in its current form soon.

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.

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:

  1. Make it return a void* instead of a fltSemantics&
  2. Make these global free functions marked extern "C"

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.

This revision was automatically updated to reflect the committed changes.