Skip to content

CMake: Include missing modules in druntime/Phobos#1917

Merged
kinke merged 2 commits intoldc-developers:masterfrom
kinke:gh1915
Feb 7, 2017
Merged

CMake: Include missing modules in druntime/Phobos#1917
kinke merged 2 commits intoldc-developers:masterfrom
kinke:gh1915

Conversation

@kinke
Copy link
Member

@kinke kinke commented Dec 8, 2016

Fixes issue #1915. Using exclusively blacklists and no more whitelists (especially for subdirectories) should be way more robust wrt. future restructurings.
I also got rid of obsolete existing blacklist entries and removed a hopefully obsolete workaround wrt. disabling invariants for some parts of druntime.

@dnadlinger
Copy link
Member

dnadlinger commented Dec 8, 2016

Could you also push the obvious one-line band-aid to the release branch? The druntime flag changes definitely shouldn't go into the release.

@kinke
Copy link
Member Author

kinke commented Dec 8, 2016

I'm kind of reluctant of adding that particular module as hack for the release just to close that issue early. On Win64, we are talking about 18 additional modules for the all target, i.e., a total of 9 for druntime/Phobos (debug + release), incl. core.stdcpp.* etc.

@kinke
Copy link
Member Author

kinke commented Dec 8, 2016

Reason for the (current) CI errors: core.stdcpp.typeinfo contains a C-style static array declaration leading to a warning, thus the object file isn't generated. The declaration is fixed upstream.
On Windows, the druntime testrunner then fails to link due to an unresolved default ctor for std::exception (only declared in D, to be pulled in from MSVCRT). I recall some C++ ctor mangling issues for MSVC when working on real_t... or a deliberate decision to mangle the ctors differently (and so having to duplicate them for D and C++) as D ctors assume this is initialized with T.init, and so calling ctors implemented in D from the C++ side would be evil. Ah, there it is: dlang/dmd#5884.

@kinke
Copy link
Member Author

kinke commented Dec 8, 2016

After excluding core.stdcpp.* due to misc. issues, the only new module causing trouble is - of course - the requested etc.linux.memoryerror. ;)

@kinke
Copy link
Member Author

kinke commented Dec 9, 2016

See dlang/druntime#1709 wrt. core.stdcpp.*.

list(APPEND DRUNTIME_D ${DRUNTIME_D_LINUX})
elseif(${CMAKE_SYSTEM} MATCHES "SunOS")
list(APPEND DRUNTIME_D ${DRUNTIME_D_SOLARIS})
# TODO: Android (Bionic)
Copy link
Member Author

Choose a reason for hiding this comment

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

@joakim-noah: Can you help out here wrt. detecting a native CMake build on Android?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think CMake supports such builds well, as it doesn't have any built-in support for native Android compiles and the package I'm using is just compiled similar to linux/ARM. So CMAKE_SYSTEM_NAME and CMAKE_SYSTEM are set to the same as on any linux, ie Linux and the same with the kernel version appended.

I suggest you just add the bionic folder for linux too. It will not affect linux/glibc because its declarations are versioned out and Android isn't bothered by both linux/ and bionic/ being compiled.

Copy link
Member Author

@kinke kinke Dec 11, 2016

Choose a reason for hiding this comment

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

Thanks. You do set CMAKE_SYSTEM_NAME to Android in your build.sh script though, but if that isn't standard, we may as well just include the 2 Bionic modules for Linux generally. My only worries were that they would then show up as tested as part of the unittests, although they may be versioned out. But oh well, they only contain a single declaration each... ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is to cross-compile CMake itself for Android. Once that's done, there's no native support in the resulting native Android CMake binary or its scripts. It might as well be linux/ARM from CMake's perspective.

Bionic is versioned out and no tests, only declarations, so no problem.

@kinke
Copy link
Member Author

kinke commented Dec 10, 2016

Wrt. the etc.linux.memoryerror unittest, it fails when throwing a new NullPointerError in sigsegvUserspaceProcess() after the SIGSEGV signal: Fatal error in EH code: _Unwind_RaiseException failed with reason code: 5 (END_OF_STACK). Putting the code into a main() function instead of a unittest block doesn't change anything.

@kinke
Copy link
Member Author

kinke commented Dec 11, 2016

When radically reducing the signal handler to

void sigsegvDataHandler()
{
    asm
    {
        naked;
        push RSI;   // return address (original RIP).
        // Parameter address is already set as RDI.
        jmp sigsegvUserspaceProcess;
    }
}

the exception is thrown correctly.
This then works:

import etc.linux.memoryerror, core.stdc.stdio;

int* getNull() { return null; }
void setNull() { *getNull() = 42; }

void main()
{
    registerMemoryErrorHandler();

    try
    {
        //*getNull() = 42;
	setNull();
    }
    catch (NullPointerError)
    {
        printf("Caught\n");
    }

    deregisterMemoryErrorHandler();
}

But the commented-out code doesn't (unhandled NullPointerError), as it isn't invoked like the function call, it's just an assignment happening to result in a thrown Error. No idea how this fits into LLVM's landing pad model...

@kinke
Copy link
Member Author

kinke commented Jan 22, 2017

As I recall reading somewhere that Walter plans to deprecate etc.linux.memoryerror, I simply keep on excluding that module now in this PR.
dlang/druntime#1709 has been merged, meaning that we should soon be able to include the core.stdcpp.* modules without running into linking errors for the druntime testrunners (and/or even compilation warnings).

@kinke kinke changed the title [WIP] CMake: Include missing modules in druntime/Phobos CMake: Include missing modules in druntime/Phobos Jan 22, 2017
kinke added 2 commits February 6, 2017 21:43
Using exclusively blacklists and no more whitelists (especially for
subdirectories) should be way more robust wrt. future restructurings and
additions.
I also got rid of obsolete existing blacklist entries and removed a
hopefully obsolete workaround wrt. disabling invariants for some parts
of druntime.
The most urgent linking errors have been fixed upstream by
dlang/druntime#1709 in the meantime.
@kinke kinke merged commit 6a932c5 into ldc-developers:master Feb 7, 2017
@kinke kinke deleted the gh1915 branch February 7, 2017 12:52
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 this pull request may close these issues.

3 participants