Skip to content
This repository has been archived by the owner on Jun 20, 2019. It is now read-only.

Shared library support #214

Merged
merged 22 commits into from
Sep 10, 2016
Merged

Shared library support #214

merged 22 commits into from
Sep 10, 2016

Conversation

jpf91
Copy link
Contributor

@jpf91 jpf91 commented May 22, 2016

OK, I've now rewritten the shared library support in a DMD/LDC compatible way.

high level overview

  • Make find_field a global function: aggregate_find_field
    This function is useful for the shared library / ModuleInfo emission code so I made it a global function.
  • Add new build_simple_function_decl function
    Like build_simple_function, but do not call toObjfile. This allows further adjustments on the GCC tree (such as setting VISIBILITY=HIDDEN or calling d_comdat_linkage)
  • Emit ModuleInfo references into minfo section if necessary
    This commit contains the real compiler work.
    • First we need to decide whether we use the new approach, placing a pointer to ModuleInfo into a minfo section, or the old approach using _Dmodule_ref. I chose a solution which does not require changes to upstream druntime code but also does not hardcode the targets like DMD does: We try to load rt/sections. If the module is not found, we do not emit any ModuleInfo. If it is found, try to find the _Dmodule_ref, CompilerDSOData and _d_dso_registry symbols. Ignore permissions, as these declarations are private. If _d_dso_registry and CompilerDSOData exist, use the new approach. Otherwise if _Dmodule_ref exists, use the old approach. Otherwise do not emit ModuleInfo. Nice side-effect: Small runtimes don't have to provide _Dmodule_ref fake declarations anymore.
    • For the new approach
      • Place ModuleInfo into minfo section. LDC uses .minfo but we need to avoid leading dots to use this feature: https://sourceware.org/binutils/docs-2.26/ld/Orphan-Sections.html Binutils will the provide __start_minfo and __stop_minfo symbols bracketing the section.
      • Emit a constructor gdc_dso_ctor and a destructor gdc_dso_dtor (one per object file). We want one of these per library, so we set them as comdat decls and visibility=hidden. However, GCC doesn't allow setting the references in the .ctors section as comdat. We end up with one ctor/dtor function per library but it is called n times, were n is the number of modules in the library.
      • To make sure the code in the constructors is called only once per library, use a gdc_dso_initialized variable.
      • When generating the CompilerDSOData constant, use aggregate_find_field and the symbol from rt.sections. This way there'll never be a compiler/runtime mismatch, the debug info is nice & useful etc.
      • We reference the __start_minfo and __stop_minfo symbols to find the ModuleInfos. If these are marked as hidden the linker will make them per-library. Took some time to figure out I have to set DECL_VISIBILITY_SPECIFIED as well. Anyway, it's now working nicely.
      • The implementation is compatible with LDC, we only need to standardize the names (ldc.dso_slot vs gdc_dso_slot etc)
  • druntime: Copy DMD sections implementation
    Simply copy code unmodified from upstream.
  • Revert "Disable shared libraries for now"
    Start building shared libraries.
  • Also temporarily disable certain tests for shared libs
    We have some disabled tests in the static unittests. Also disable these for shared library unittests.
  • Update phobos shared library version
    Update to 2.068.2
  • Use -fno-emit-moduleinfo when compiling configure test programs
    The new ModuleInfo emission otherwise loads rt/sections.d. sections.d depends on rt.deh which depends on gcc.unwind which depends on gcc.config. But gcc.config is generated by the druntime configure script and not available yet when running GDC tests in configure.
  • Use -fversion=Shared when compilling druntime
    Object files going into the shared druntime need to be compiled with version=Shared. This implementation is a bit of a hack, as it uses the PIC flag to detect whether a object will be part of the shared library. Theoretically the static lib could also contain PIC objects, so this is incorrect. However, the libtool maintainers always used the same hack in C/C++ and even define DLL_EXPORT depending on the PIC mode (windows doesn't even have PIC mode...) so this should be OK.
    All proper solutions add lots of bloat to the Makefiles. We could add a second library definition for the static library, (libgdruntime_s.la) but we still need libtool to link libbacktrace.la. Then we'd have to handle (un)installation manually to rename libgdruntime_s.a to libgdruntime.a and even then, the libgdruntime.la file would still only refer to the shared library.
  • Backport rt.util array overflow fixes
  • Rebuild build script files
  • Add -shared-libphobos flag
    Make static linking the default for now. -static-libphobos is also available.

Testing so far

  • unittests (drt, phobos) with shared & static libraries
  • test suite: now runs with both shared & static library
  • passes druntime DSO tests
  • tested --disable-static and --disable-shared
  • some manual tests after make install

TODO list

  • Use a stack variable in the gdc_dso_ctor and gdc_dso_dtor functions
  • cleanup Emit ModuleInfo references into minfo section if necessary commit.
  • Add a configure test for druntime to check whether the linker actually supports adding the __start_minfo and __stop_minfo symbols. Output this information to gcc.config and add a static assert(HaveMinfoBracketing) to rt.sections for shared elf code.
  • Integrate the druntime shared library tests
  • Test both static and shared library in testsuite
  • Test --disable-static
  • Test --disable-shared
  • Look at LDC and DMD for library versioning, see if we can adapt one of their versioning schemes
  • debug failing test suite test case
  • write changelog
  • rebase

This is ready for review.

@ibuclaw
Copy link
Member

ibuclaw commented May 23, 2016

Note: Currently requires --disable-werror because of issue http://bugzilla.gdcproject.org/show_bug.cgi?id=211

In the meantime, we should probably raise a PR upstream to add overflow detection.

@ibuclaw
Copy link
Member

ibuclaw commented May 23, 2016

In the meantime, we should probably raise a PR upstream to add overflow detection.

Oh, I forgot, it is impossible to overflow!

Here's a working implementation.

    @property void length(size_t nlength)
    {
        if (nlength < length)
        {
            if (length >= nlength)  // XBUG: Tell the optimizer that no, this can't possibly be true.
                assert(0);
            foreach (ref val; _ptr[nlength .. length])
                common.destroy(val);
        }
        _ptr = cast(T*)common.xrealloc(_ptr, nlength * T.sizeof);
        if (nlength > length)
        {
            if (nlength <= nlength)  // XBUG: Tell the optimizer that no, this can't possibly be true.
                assert(0);
            foreach (ref val; _ptr[length .. nlength])
                common.initialize(val);
        }
        _length = nlength;
    }

@jpf91
Copy link
Contributor Author

jpf91 commented May 25, 2016

@jpf91 jpf91 force-pushed the shared3 branch 4 times, most recently from caee8e8 to ce8cca0 Compare August 1, 2016 15:39
@ibuclaw
Copy link
Member

ibuclaw commented Aug 1, 2016

If _d_dso_registry and CompilerDSOData exist, use the new approach. Otherwise if _Dmodule_ref exists, use the old approach. Otherwise do not emit ModuleInfo. Nice side-effect: Small runtimes don't have to provide _Dmodule_ref fake declarations anymore.

Yeah, keeping components of Druntime optional is the way to go.

I don't know how to properly set up a stack variable when manually building a function in the backend. See https://github.com/jpf91/GDC/blob/b1e537a2a29d9785ba699cee2b8ce01afb24d62b/gcc/d/d-objfile.cc#L2476 . @ibuclaw could you please explain how to best solve this?

Stack variables are referenced as the vars in a BIND_EXPR. The best order to do things so that existing code is reused would be:

current_function_decl = func_tree;
push_binding_level (level_function);
/* decl = non-static VAR_DECL node.  */
d_pushdecl (decl);
tree block = pop_binding_level ();
func_body = build3 (BIND_EXPR, void_type_node, BLOCK_VARS (block), func_body, block);
func_decl = build_simple_function_decl (func_name, func_body);

Though having a quick look at the area where this would go, you'd have to see if you can reorder how you generate the parts of the function somehow.

Can we somehow integrate the druntime DSO tests?

Does this differ from the testsuite somehow? I'd have to look at what it is doing be honest.

Should the testsuite use the shared library, the static library or both?

Aiming for running the entire testsuite and unittests linking against a shared library would be the target to go for.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 1, 2016

Compiler support looks simple enough, which is what I hoped for. Will try to fit in a proper review tomorrow. Thanks! :-)

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 1, 2016

...
Though having a quick look at the area where this would go, you'd have to see if you can reorder how you generate the parts of the function somehow.

Yes, but that's not really a big problem. I can simply overwrite the function body tree in the WrappedExp. But I didn't know how to use push_binding_level, BIND_EXPR and all that stuff. Thanks for the sample code, I'll integrate that tomorrow :-)

Can we somehow integrate the druntime DSO tests?

Does this differ from the testsuite somehow? I'd have to look at what it is doing be honest.

It tests loading & linking shared libraries. This does not really fit well in the test suite framework so there is an extra Makefile for these tests. I guess we could just add a make call to check-local. But we also need to check whether the OS can actually run these tests before executing the Makefile (IIRC druntime only supports dynamic loading on linux).

Should the testsuite use the shared library, the static library or both?

Aiming for running the entire testsuite and unittests linking against a shared library would be the target to go for.

OK. Shared library only or shared & static library?
We already run the unittests with the shared and the static runtime. Testing both shared & static libraries in the testsuite should be simple but it will double the test suite run time.

Compiler support looks simple enough, which is what I hoped for. Will try to fit in a proper review tomorrow. Thanks! :-)

Sounds great, thanks! Most changes are in druntime in the druntime: Copy DMD sections implementation commit. This is simply copy/paste from upstream druntime though, no GDC specific changes. So you can skip all new files in that commit and the changed files are only upstream changes as well.

BTW: I forgot to mention that this currently removes the EmuTLS support. I have to rewrite that code for the new rt.sections modules. OTOH I think that's another pull request and EmuTLS is currently broken anyway.

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 2, 2016

But I didn't know how to use push_binding_level, BIND_EXPR and all that stuff. Thanks for the sample code, I'll integrate that tomorrow :-)

Turns out I can't use these functions directly, as they rely on other functions to initialize some globals (start_function, ...). I just 'inlined' the relevant parts of these functions.

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 3, 2016

Something completely unrelated, but I just realized that make check-d does not run the check-local targets in the multilib configurations for some reason. E.g. on x86_64/x86 mutlilib it only runs check-local for the 64 bit build, but not in x86_64-pc-linux-gnu/32/libphobos/. Is that expected?

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 4, 2016

So I had another look at the library versioning, ping @Dicebot

DMD:

SONAME=libphobos2.so.0.70
libphobos2.so
libphobos2.so.0.70
libphobos2.so.0.70.2

LDC:

SONAME=libdruntime-ldc.so.70
libdruntime-ldc.so
libdruntime-ldc.so.70
libdruntime-ldc.so.2.0.70

Here's what we can do with libtools:

  • -release: Appends the version to the library name, SONAME=libgphobos2-2.068.2.so libgphobos2-2.068.2.so. All versions are ABI incompatible.
  • -version-info=current:revision:age: The recommended libtool setting. SONAME=libgphobos2.so.[current - age], libgphobos2.so.[current - age].[age].[revision].
  • -version-number=maj:min:rel: SONAME=libgphobos2.so.[maj], libgphobos2.so.[maj].[min].[rel]. The following restrictions apply: SONAME is always libraryname.so.[maj]. The rel number must be always present, even if it's always 0.

Because of the SONAME restrictions we can't implement the DMD scheme. The LDC scheme can be implemented with both -version-number or -version-info (except for the strange libdruntime-ldc.so.2.0.70 symlink).

And here's an example (first value: file name; second value: soname):

phobos version abi release version-info version-number
2.066.0 1. release libgphobos-2.066.0.so libgphobos-2.066.0.so libgphobos.so.66.0.0 libgphobos.so.66 libgphobos.so.66.0.0 libgphobos.so.66
2.066.1 no change libgphobos-2.066.1.so libgphobos-2.066.1.so libgphobos.so.66.0.1 libgphobos.so.66 libgphobos.so.66.1.0 libgphobos.so.66
2.066.2 no change libgphobos-2.066.2.so libgphobos-2.066.2.so libgphobos.so.66.0.2 libgphobos.so.66 libgphobos.so.66.2.0 libgphobos.so.66
2.067.0 only additions libgphobos-2.067.0.so libgphobos-2.067.0.so libgphobos.so.66.1.0 libgphobos.so.66 libgphobos.so.66.3.0 libgphobos.so.66
2.067.1 no change libgphobos-2.067.1.so libgphobos-2.067.1.so libgphobos.so.66.1.1 libgphobos.so.66 libgphobos.so.66.4.0 libgphobos.so.66
2.066.3 no change (2.066.2) libgphobos-2.066.3.so libgphobos-2.066.3.so libgphobos.so.66.0.3 libgphobos.so.66 ???
2.068.0 abi breakage libgphobos-2.068.0.so libgphobos-2.068.0.so libgphobos.so.67.0.0 libgphobos.so.67 libgphobos.so.67.0.0 libgphobos.so.67
2.068.1 no change libgphobos-2.068.1.so libgphobos-2.068.1.so libgphobos.so.67.0.1 libgphobos.so.67 libgphobos.so.67.1.0 libgphobos.so.67

The starting release number is arbitrary (though we should start at 0 once we have ABI compatible releases). As you can see the -version-number scheme produces a conflict when an older phobos version gets a point release. This can be fixed by increasing the third number for revisions and the second number for non-ABI-breaking phobos releases. But then you get exactly the -version-info scheme.

TLDR: I think we can & should use the -version-info scheme. As soon as phobos has ABI compatible releases, the -version-info scheme is the best scheme.

For now we can simply bump the current version on every release and always set age to 0. There are two minor problems with this:

  • We assume revisions (2.066.0/2.066.1) are always ABI compatible, which might not be true.
  • We 'waste' the library names. If we restart at 0 when we have ABI compatible releases, we'll get conflicts once we reach 66 again. That's probably an academic problem ;-)

The -release versioning scheme would avoid these problems, but probably cause further trouble for packagers, so I'd use the -version-info scheme.

@jpf91 jpf91 changed the title WIP: Shared library support Shared library support Aug 4, 2016
@@ -20,7 +20,7 @@
## with this library; see the file COPYING3. If not see
## <http://www.gnu.org/licenses/>.

SUBDIRS = libdruntime src
SUBDIRS = libdruntime src test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name the directory testsuite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

@ibuclaw
Copy link
Member

ibuclaw commented Aug 6, 2016

We should be able to reliably use the -version-info for druntime now. So we can use whatever more closely resembles the supported upstream version, eg: -version-info 2:68:1.

For phobos, according to the libtool docs, you can force each release to be binary incompatible using the -release switch.

https://www.gnu.org/software/automake/faq/libtool.html#Release-numbers

@ibuclaw
Copy link
Member

ibuclaw commented Aug 6, 2016

Using -release should work with package distributions too, as libphobos would be tied to a specific compiler release. A D program built with gdc-5.x will have a it's package depend on libgphobos-5, so you can have libgphobos-6.so and libgphobos-5.so coexist on the same system.

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 6, 2016

We should be able to reliably use the -version-info for druntime now. So we can use whatever more closely resembles the supported upstream version, eg: -version-info 2:68:1.

I wouldn't do that. The libtool docs are quite clear that breaking ABI changes must always increment the first (current number). So I think we should use -version-info 68:0:1.

For phobos, according to the libtool docs, you can force each release to be binary incompatible using the -release switch.

Yes, that's certainly possible and maybe the best solution, but @Dicebot mentioned he would prefer a more DMD/LDC compatible versioning.

A D program built with gdc-5.x will have a it's package depend on libgphobos-5, so you can have libgphobos-6.so and libgphobos-5.so coexist on the same system.

I don't think we should base our versions on the GCC release. The GCC backend does not break ABI so druntime version 2.068.2 built with gdc-5 should also work fine with a 2.068.2 gdc-6, no? I think if we use release, we should use libgphobos-2.068.2.so.

@ibuclaw
Copy link
Member

ibuclaw commented Aug 7, 2016

I think if we use release, we should use libgphobos-2.068.2.so.

I don't think the point release should be included. Unless there's any examples of adding/removing interfaces between 2.xxx.0 and 2.xxx.1.

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 7, 2016

I don't think the point release should be included. Unless there's any examples of adding/removing interfaces between 2.xxx.0 and 2.xxx.1.

You're right, the point release should not be included if we use -release. So it's libgphobos-2.068.so instead. (As our library is called libgphobos2 right now, the real name would actually be libgphobos2-2.068.so or libgphobos2-68.so or something similar)

@ibuclaw
Copy link
Member

ibuclaw commented Aug 13, 2016

Yeah, I'd move the two into the version number. It's been years since D1, it should go.

@jpf91
Copy link
Contributor Author

jpf91 commented Aug 13, 2016

Maybe name the directory testsuite?

Done. Also changed to use -version-info=68:2:0 for druntime, -release=2.068 for phobos and renamed the library from libgphobos2 to libgphobos.

@ibuclaw
Copy link
Member

ibuclaw commented Sep 10, 2016

@jpf91 - Any last things holding up? (Apart from changelog, rebase)

@ibuclaw
Copy link
Member

ibuclaw commented Sep 10, 2016

I'd say merge this yourself when you are ready. There are some re-factorings that need doing for the D conversion that are waiting on this.

For now, only do error reporting. Theoretically we could also
fall back to the _Dmodule_ref model in such cases by simply
not defining the _d_dso_registry function:

static if (Minfo_Bracketing)
{
    _d_dso_registry...
}
else
{
    void* _Dmodule_ref...;
}
The new ModuleInfo emission otherwise loads rt/sections.d. sections.d
depends on rt.deh which depends on gcc.unwind which depends on
gcc.config. But gcc.config is generated by the druntime configure
script and not available yet when running GDC tests in configure.
For dynamic library loading support
Calling Runtime.unloadLibrary(h) in assert() breaks the test when
compiled with -frelease
@jpf91 jpf91 merged commit cc5c0d3 into D-Programming-GDC:master Sep 10, 2016
@jpf91
Copy link
Contributor Author

jpf91 commented Sep 10, 2016

There are some re-factorings that need doing for the D conversion that are waiting on this.

Sounds interesting ;-) Merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants