Fix Issue 19481: mutex.d(95) Error: pthread_mutex_init failed

[ibuclaw]

As seen on at least Solaris, maybe SPARC/Linux as well.

[dlang-bot]

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19481	major	Aborting from local/libphobos/libdruntime/core/sync/mutex.d(95) Error: pthread_mutex_init failed.

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#2411"

[thewilsonator]

@TurkeyMan is about to add something like maxAlignment soon.

[ibuclaw]

But in the meantime, Sparc and i386 Solaris runtime is failing now, and this fixes it.

[thewilsonator]

See #2412 for the traits from @TurkeyMan's PR.

[schveiguy]

Would it be better/easier to avoid using an array to define _locks? In other words, just _lock0, and _lock1 to allow more control over the alignment of the second lock?

[ibuclaw]

Would it be better/easier to avoid using an array to define _locks?

Granted that there's only two locks, there's nothing lost having two separate static variables instead.

[ibuclaw]

This can be simplified like this:

I think that's just over engineering the problem. There's no need to wrap it up in a struct.

[PetarKirov]

I'm strongly against repeating the compiler's alignment calculation logic. Repeating it is over-engineering.

I agree with with @schveiguy that code could be even simpler and less error prone if we do away with the array entirely.

[ibuclaw]

I'm strongly against repeating the compiler's alignment calculation logic. Repeating it is over-engineering.

The small pattern that rounds a number up to an alignment boundary? It's hardly repeating the compiler (we're doing something that the compiler didn't - as it doesn't make sense to add padding to the end of classes), and is used throughout in various other places from varargs to sections (perhaps there should be an alignTo!(X) template).

[kinke]

perhaps there should be an alignTo!(X) template

I think such a helper would be really useful, I missed it countless times too.

[ibuclaw]

Rebased.

[schveiguy]

Looks great!

[thewilsonator]

Hmm

src/mecca/lib/memory.d:279: error: undefined reference to '_D4core6thread6Thread6_locksG2G72v'
--
  | src/mecca/lib/memory.d:291: error: undefined reference to '_D4core6thread6Thread6_locksG2G72v'

[thewilsonator]

Urgh: https://github.com/weka-io/mecca/blob/master/src/mecca/lib/memory.d#L443-L451

[ibuclaw]

This is what happens when private does not have any effect on symbol visibility in the object file. :-)

[thewilsonator]

cc @EyalIO whats the best way forward? Commit this and then fix up your end?

Also @ibuclaw this should target stable and then merge stable into master straight after, that way the DMD releases can be used to build mecca.

[dnadlinger]

This is what happens when private does not have any effect on symbol visibility in the object file. :-)

@ibuclaw: private is a source-level concept and should not have any effect on object file visibility. You want to be able to give out an alias to a private symbol to another module.

[ibuclaw]

@thewilsonator - Shall we just move forward? The fix for Mecca will not work until this gets merged anyway...

@klickverbot - I know, but I still like to complain about long compile times caused by inability to discard instantiated symbols. ;-)

[ibuclaw]

Meanwhile, Solaris port is broken...

[jacob-carlborg]

The fix for Mecca will not work until this gets merged anyway...

Mecca can be patched to check for the _locks member. Something like this:

static if (__traits(hasMember, Thread, "_locks"))
{
    pragma(mangle, "_D4core6thread6Thread6_locksG2G72v") extern __gshared static
            void[__traits(classInstanceSize, Mutex)][2] _locks;
}

else
{
    pragma(mangle, ...) extern __gshared 
        align(mutexAlign) void[mutexClassInstanceSize] _slock;
}

[thewilsonator]

dlang-community/mecca#14

[ibuclaw]

There should be some impending deadline or high priority label, as gcc-9 release is only 4 weeks away and this needs to be in before then to unblock the completion of porting of a few primary architectures.

[wilzbach]

We can always temporarily remove mecca if the maintainers are unresponsive or switch to master if creating a new tag is not possible. We have done that in the past too.

[thewilsonator]

The maintainers are reasonably responsive, the breakage is due to them needing internals of core.Thread.Context because core.thread is poorly designed. The easiest way forward is to not affect the mangling of Thread._locks.

[jacob-carlborg]

it's still a public ABI change in the resultant binary

We don't offer any ABI compatibility.

[jacob-carlborg]

The easiest way forward is to not affect the mangling of Thread._locks.

Using pragma(mangle) here to match mecca? The mangling in this case if platform dependent since it contains the size of Mutex, IIRC. But that can be fixed as well.

[ibuclaw]

The mangling in this case if platform dependent since it contains the size of Mutex, IIRC. But that can be fixed as well.

The Mecca lib has locked itself out of being platform independent anyway, due to this assert.

static assert (__traits(classInstanceSize, Mutex) == 72); // This size is part of the mangle

[jacob-carlborg]

The Mecca lib has locked itself out of being platform independent anyway, due to this assert.

static assert (__traits(classInstanceSize, Mutex) == 72); // This size is part of the mangle

Yes, but if we’re going to use pragma(mangle) in druntime doesn’t it need to be correct on all platforms? There’s also this PR that adds support for macOS to mecca: dlang-community/mecca#12. Which handles the issue by using __traits(classInstanceSize, Mutex) as part of the mangled name instead of completely hard coding it.

[thewilsonator]

Thats not the problem:

error: undefined reference to '_D4core6thread6Thread6_locksG2G72v'

struct DRuntimeStackDescriptor {
    static if (__traits(hasMember, Thread, "_locks")) { // not the case anymore
       //...
    }
    else {
        pragma(mangle,"_D4core6thread6Thread6_slockG72v") extern __gshared static
            void[__traits(classInstanceSize, Mutex)] _slock;
   }
}

Thread doesn't have a member _locks anymore (this PR changes it) so I don't know why its taking the wrong static branch.

[jacob-carlborg]

It's still using the v0.0.8 tag because the new tag is missing the v prefix: https://github.com/weka-io/mecca/releases/tag/0.0.9.

[thewilsonator]

Ahh.

[thewilsonator]

dlang-community/mecca#16

[jacob-carlborg]

@thewilsonator this still looks suspicious to me: https://github.com/weka-io/mecca/blob/1f6909ec02d19ef2115b7c7402cf4c0327bd316a/src/mecca/lib/memory.d#L453-L454. Having two symbols with the same name in the same scope, as far as I can see.

[thewilsonator]

Probably, but thats not the reason it is failing at the moment.

[jacob-carlborg]

Probably, but thats not the reason it is failing at the moment.

No, but it still needs to be fixed. When they fix/add a new tag it's going to fail for another reason.

[ibuclaw]

When they fix/add a new tag it's going to fail for another reason.

Indeed. https://explore.dgnu.org/z/dNR7_q

[EyalIO]

Sorry about the missing v in the tag, the v0.0.9 tag is now equal to 0.0.9 in https://github.com/weka-io/mecca

[ibuclaw]

Rebased, interestingly still get undefined reference.

[jacob-carlborg]

Using the correct tag now at least.

[ibuclaw]

Nope, the tag was never correct in the first place.

dlang-community/mecca@v0.0.9...master

[EyalIO]

Created a v0.0.10 tag with the new commit: https://github.com/weka-io/mecca/releases/tag/v0.0.10

[ibuclaw]

And now it is failing as we expected. No point creating more tags until the problem in mecca is fixed.

[JinShil]

It appears mecca has already been disabled: https://github.com/dlang/ci/blob/9d820657ea140e7128ba9daf4b60f68bfd54afb8/buildkite.sh#L145 Please rebase.

[ibuclaw]

Rebased.

[thewilsonator]

Auto-merge toggled on