rt.sections_elf_shared.d: Populate _tlsRanges in every thread

[ibuclaw]

I've hit a segfault in a non-main thread, dereferencing a global TLS static variable, that appears to have been GC'd.

A printf inside scanTLSRanges confirmed this.

[39936000] scanTLS: tlsRanges{ .length = 4, .ptr = 703090}
[39937280] scanTLS: tlsRanges{ .length = 0, .ptr = 0}    <-- thread 2
[39937280] scanTLS: tlsRanges{ .length = 4, .ptr = 703090}
Segmentation fault (core dumped)

Linking against the version(Shared) libphobos library does not have the same problem, as inheritLoadedLibraries is called on each new thread entrypoint, which calls getTLSRange again to get all thread-local ranges.

Neither does this appear to affect other sections support, as they either get the addresses of the thread local _tlstart/_tlsend brackets, or call pthread_[get|set]specific(_tlsKey) to test and allocate a new per-thread array.

Moving the initialization of _tlsRanges from _d_dso_registry - which is only called once in the initialization of the main thread - to initTLSRanges - which is called during the construction of every thread.

[39936000] scanTLS: tlsRanges{ .length = 4, .ptr = 703090}
[39937280] scanTLS: tlsRanges{ .length = 4, .ptr = 747090}
[39937280] scanTLS: tlsRanges{ .length = 4, .ptr = 703090}

[dlang-bot]

Thanks for your pull request, @ibuclaw!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2558"

[kinke]

Mhm, this appears to be similar to a draft @dnadlinger came up with in https://forum.dlang.org/post/mailman.6193.1546546742.29801.digitalmars-d-ldc@puremagic.com. He mentioned that this shouldn't be necessary for glibc.

This gist from @s-ludwig might serve as test; seems like we desperately need one for this serious issue.

[ibuclaw]

He mentioned that this shouldn't be necessary for glibc.

Ah, ok. I've only observed the segfault on FreeBSD libc so far, however I observed the same null tlsRange on glibc too but by then I ran out of time to look further as to why it doesn't crash.

I'm not sure about this comment by @dnadlinger

Nothing to do; glibc allocates the TLS area for additional
threads at the beginning of the stack space, so they will already
be scanned.

Unless there's a peculiar internals detail I'm missing, glibc can't possibly preempt how many threads you may have open after entering main().

Though it will certainly go unnoticed if glibc preallocates more than we ever test.

[ibuclaw]

The test I'm running is shared/linkD.exe, however linking against libphobos statically.

[ibuclaw]

@kinke - Confirmed that small program also segfaults on FreeBSD/x86_64, so have added it to the testsuite.

[dnadlinger]

Nothing to do; glibc allocates the TLS area for additional threads at the beginning of the stack space, so they will already be scanned.

Unless there's a peculiar internals detail I'm missing, glibc can't possibly preempt how many threads you may have open after entering main().

Yes, it can't – hence it just puts the TLS area for each new thread at the beginning of the newly created stack. Due to the way we detect the stack bottom, we hoover up the TLS data along with what we think the stack is on Linux/Glibc.

This is of course a dirty implementation detail, but explains why things don't crash on GNU/Linux the way they are.

[dnadlinger]

I don't currently have the time to properly review this – would be good to test that we now have all the TLS ranges, but without duplicates, for both non-Shared builds and link-time-bound libraries, and Shared builds with runtime-loaded libraries. (I guess there are no changes to Shared.)

[ibuclaw]

I don't currently have the time to properly review this – would be good to test that we now have all the TLS ranges, but without duplicates,

The already existing DSO check should be good enough to cover that.

I just switched to just adding a populate-if-empty in initTLSRange(), so any subsequent calls to _d_dso_registry() in a given thread still gets the array appended to.

[ibuclaw]

GCC release is imminent, so I'll apply it locally to gdc for now...

[dnadlinger]

GCC release is imminent, so I'll apply it locally to gdc for now...

Crashes on the auto-tester, though?

The already existing DSO check should be good enough to cover that.

What I meant is to check that we don't end up scanning the same memory region multiple times (at least beyond scanning the non-main-thread TLS regions on glibc as they are included with the stack too).

I'd be quite happy to accept a slight pessimisation for the time being, though, if you can make the PR pass the auto-tester and just put up an issue to revisit this in the future. (We could e.g. simply check for overlapping ranges when we initialise the list.)

[dnadlinger]

(Didn't review the whole mechanism in detail, but as discussed above, there is definitely a real problem here on non-Glibc platforms.)

[ibuclaw]

Crashes on the auto-tester, though?

Looks like the test takes too long for the timelimit given, maybe we don't need to loop and allocate so much.

timelimit: sending warning signal 15
Makefile:20: recipe for target 'generated/linux/debug/32/tlsgc_sections.done' failed
make[2]: *** [generated/linux/debug/32/tlsgc_sections.done] Error 143

[ibuclaw]

Reduced memory allocation from 1_000_000 to 10_000, it doesn't always hit, but the bug is still reproducible without this PR.

[kinke]

How about making the extra thread sleep for a bit, sleeping for a shorter while in the main thread too (waiting for the other thread to be fully initialized), then invoking GC.collect() in the main thread while the extra thread is still alive and sleeping?

[ibuclaw]

Going to need some decisive action here (fixed my end by forking rt.sections).

[s-ludwig]

I've rewritten the test to not rely on automatic collections or timing - it became a bit longer, but it works:

import core.memory;
import core.sync.condition;
import core.sync.mutex;
import core.thread;

__gshared Condition g_cond;
__gshared Mutex g_mutex;
__gshared int g_step = 0;

class C
{
    ~this()
    {
        import core.stdc.stdlib;
        abort();    // this gets triggered although the instance always stays referenced
    }
}

C c;

static this()
{
    c = new C;
}

static ~this()
{
    import core.memory;
    GC.free(cast(void*)c); // free without destruction to avoid triggering abort()
}

void test()
{
    assert(c !is null);

    // notify the main thread of the finished initialization
    synchronized (g_mutex) g_step = 1;
    g_cond.notifyAll();

    // wait until the GC collection is done
    synchronized (g_mutex) {
        while (g_step != 2)
            g_cond.wait();
    }
}


void main()
{
    g_mutex = new Mutex;
    g_cond = new Condition(g_mutex);

    auto th = new Thread(&test);
    th.start();

    // wait for thread to be fully initialized
    synchronized (g_mutex) {
        while (g_step != 1)
            g_cond.wait();
    }

    // this causes the other thread's C instance to be reaped with the bug present
    GC.collect();

    // allow the thread to shut down
    synchronized (g_mutex) g_step = 2;
    g_cond.notifyAll();

    th.join();
}

[ibuclaw]

Thanks @s-ludwig, I've now updated the test and rebased on master...

[ibuclaw]

Ping @thewilsonator

[dnadlinger]

Sorry, this had already slipped from view when the test was fixed – I really wasn't expecting this still to be unmerged.

[ibuclaw]

@dnadlinger - I've had interruptions in work, but there was nothing stopping others pushing the updated test to branch in my absence... :-)

[dnadlinger]

… or to merge this in mine. ;)

[Geod24]

Can you give this another push ? There was a network issue.

[ibuclaw]

I think it's safe to just merge. The socket timeout is currently ignored by our PR manager.