Add explicit @system attribute to extern functions

[pbackus]

Needed for compatibility with @safe-by-default.

---

These @system-by-default function declarations were found using the deprecation message introduced in dlang/dmd#11176.

These changes only apply to declarations that produce deprecation messages on version (Posix), since that's all I can test locally.

[dlang-bot]

Thanks for your pull request and interest in making D better, @pbackus! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 run digger -- build "master + druntime#3117"

[pbackus]

The Linux_32 and Linux_64_32 errors on the autotester are the same, and are caused by an unexpected .stringof result in the dmd test suite:

compilable/testInference.d(287): Error: static assert:  `"extern (C) void function() @system" == "extern (C) void function()"` is false

The Darwin_64_64 failure is due to a double-free in the forking GC test:

forkgc2(93369,0x108af9000) malloc: *** error for object 0x7f8f00d00150: double free
*** set a breakpoint in malloc_error_break to debug

Both of these appear to be unrelated to this PR.

[Geod24]

The second failure, on Mac OSX, is definitely unrelated, and gone. But the first one is not only consistently showing, it is also showing an extra @system that wasn't there before. What makes you think it is unrelated to this PR ?
I have restarted Linux_64 and Linux_32 just in case, but I expect them to fail again with the same, totally consistent, definitely related, error message.
It's interesting to note that it only shows up on Linux though.

Regarding the PR: going ahead with this would add a lot of cruft to support something that not a single contributor but the DIP author has voiced support for. Should Walter revise his position, this change would not be needed anymore, yet it touches enough places that a revert will be difficult, and add little value, so it probably wouldn't happen anyway.

[pbackus]

@Geod24 What makes me think it's unrelated is:

1. This PR does not touch the dmd test suite, and the test in question does not depend on druntime.
2. I can now reproduce the failure locally, and it occurs using unmodified checkouts of master for both dmd and druntime.

EDIT: I had stale files left over from a previous build; I cannot reproduce locally with a clean build, on any branch.

In fact, you yourself acknowledged that the very same test is likely suspect in a comment on #3009, so I'm not sure why you've changed your mind here.

Regarding the PR, I am perfectly happy to leave this un-merged as long as the status of DIP 1028 remains uncertain. My main motivation in submitting this was to address some of the test failures for dlang/dmd#11176.

[pbackus]

@Geod24 I've done some digging, and I'm pretty sure this is issue 3796, based on this comment in typesem.d. gdb backtrace shows this code path being taken during semantic analysis of the failing test case.

Here's a reduced version of testInference.d that fails on 32-bit Linux but succeeds on 64-bit Linux:

module testInference;

import core.demangle: demangle;

void foo8504()() {}

auto toDelegate8504a(F)(auto ref F fp) { return fp; }

extern(C) void testC8504() {}

void test8504()
{
    // commenting out the following line makes the test pass on 32-bit linux
    static assert(demangle(foo8504!().mangleof) == "pure nothrow @nogc @safe void testInference.foo8504!().foo8504()");

    auto fp1 = toDelegate8504a(&testC8504);
    static assert(typeof(fp1).stringof == "extern (C) void function()");
}

I think the correct thing to do here in the short term is to change the test to not use .stringof.

[Geod24]

@pbackus : Thanks for tracking it down.

To clarify my previous comment:
This PR is not guilty of introducing a bug, but it does trigger one in the compiler. So the failure is real (something changed) and not spurious (as it can be for env-dependent / non-deterministic tests). And any real failure need to be investigated.
Note that we had a few cases of real failures being dismissed as unrelated in the past, simply because one would sometimes either not care, read, or understand the error messages, which led to organization-wide CI outage for days. So any claim a failure is unrelated should be met with high scrutiny.

I think the correct thing to do here in the short term is to change the test to not use .stringof.

If we are to pull this, yes.

I've done some digging, and I'm pretty sure this is issue 3796

Since that issue is closed, can you either re-open it with your test-case, or open a new issue ? Thanks!

[pbackus]

@Geod24 Filed as issue 20878.

[pbackus]

Closing this now that DIP 1028 has been withdrawn.

[schveiguy]

I'm not sure it's a bad thing to have this anyway. DIP1028 is gone, but applying @system attributes where they are correct can only clarify things, and protect against stray @safe: things that might get added incorrectly.