Fix issue 22234 - __traits(getLinkage) returns wrong value for extern(System) functions

[dkorpel]

Looking at the test cases that came with the implementation (dlang/dmd#6822), it looks like intentional behavior that __traits(getLinkage) gives the actual linkage of extern(System) instead of "System". It also sounds more useful to me this way and avoids code breakage, hence the spec PR instead of dmd PR.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! 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

Auto-close	Bugzilla	Severity	Description
✓	22234	normal	__traits(getLinkage) returns wrong value for extern(System) functions

[adamdruppe]

I don't remember what I was doing when making the bug report, but I know it bit me somewhat hard - hard enough to check the spec and open the bug report anyway.

[dkorpel]

Was it important for your code that __traits(getLinkage) returned "System" instead of "Windows" or "C"?

[adamdruppe]

Again, I don't remember exactly what I was doing then, but it was broken enough to spend the time debugging it, comparing against the spec, and filing the bug report. So it obviously was important.

One case where this would make a difference is using built-in reflection to generate a binding file ahead of time. Saying "C" when it is actually "System" means you now get crashes on Windows.

And that reminds me, the linkage is also wrong in the dmd json output, which is sometimes used to generate bindings - the old dtoh program worked that way.

[
 {
  "kind" : "module",
  "file" : "system.d",
  "members" : [
   {
    "name" : "foo",
    "kind" : "function",
    "protection" : "public",
    "line" : 1,
    "char" : 21,
    "linkage" : "c",
    "deco" : "UZv",
    "originalType" : "extern (System) void()",

At least the "originalType" can be tested as a string, but the "linkage" field doesn't show it.

The dmd -H also gets it wrong, meaning if you use the header generation feature and commit the file it makes, you get random crashes.

dmd -HC also gets it wrong. Again, keep that file and you're in trouble.

The root problem here is the same for all of them.

[dkorpel]

The root problem here is the same for all of them.

Yes, extern(System) is resolved in the constructor of LinkDeclaration, so dmd loses the LINK.system information very early on. Do you think we can break the existing behavior of __traits(getLinkage), or would you want to see __traits(getLinkage2) or an extra parameter / return value or something?

[adamdruppe]

On Mon, Jan 24, 2022 at 05:52:09AM -0800, Dennis wrote: Do you think we can break the existing behavior of `__traits(getLinkage)`

The existing behavior is *already broken*, not in line with the spec and giving incorrect results. It should obviously just be fixed.

[dkorpel]

The existing behavior is already broken, not in line with the spec and giving incorrect results. It should obviously just be fixed.

I'm not sure it's obvious, extern(System) is pretty rare. I know it's useful for providing one OpenGL header for both Windows and Linux, but I think headers are usually either consistent or system-specific, not differing in ABI only.

I also don't have a good idea what __traits(getLinkage) is actually used for. If you care about twiddling with parameters in assembly, then it would be useful to get a concrete ABI ('Windows', 'C') instead of a variable one ('System'). In Druntime I see it's used to distinguish extern(C++) classes with regular D ones. Also there's this:

private template hasPlainMangling(FT) if (is(FT == function))
{
    enum lnk = __traits(getLinkage, FT);
    // C || Windows
    enum hasPlainMangling = lnk == "C" || lnk == "Windows";
}

This will break.

I'm not familiar with your use case of 'using built-in reflection to generate a binding file ahead of time', so I'd appreciate an example of that.

[adamdruppe]

On Mon, Jan 24, 2022 at 06:27:23AM -0800, Dennis wrote: I'm not sure it's obvious, `extern(System)` is pretty rare. I know it's useful for providing one OpenGL header for both Windows and Linux, but I think headers are usually either consistent or system-specific, not differing in ABI only.

In my arsd repo alone, I have bindings to four fairly major things that use extern(System): OpenGL, MySQL, Chromium Embedded Framework, and Java Native Interface. The current behavior *crashes* if you generate any files with it. The correct behavior would: 1) if used on extern(System) in a case where it matters, it fixes a bug you might not have realized you had. 2) if used in a case where the specific thing is needed, you get a compile error and now know where the bug was and can fix it. Either way, fixing the bug leads to bugs getting fixed, and ignoring the bug leads to bugs staying in the wild.

I also don't have a good idea what `__traits(getLinkage)` is actually used for. If you care about twiddling with parameters in assembly, then it would be useful to get a concrete ABI ('Windows', 'C') instead of a variable one ('System').

You can easily do `version(Windows) enum systemLinkage = "Windows"; else enum systemLinkage = "C";` and just if(linkage == "System") return systemLinkage; It is easy to go from detailed information down to simpler information. But once you discard it, it cannot be recovered.

In Druntime I see it's used to distinguish `extern(C++)` classes with regular D ones. Also there's this: ```D private template hasPlainMangling(FT) if (is(FT == function)) { enum lnk = __traits(getLinkage, FT); // C || Windows enum hasPlainMangling = lnk == "C" || lnk == "Windows"; } ``` This will break.

That is ALREADY BROKEN per the language spec. The druntime authors should know better. But it is also trivially easy to add `|| lnk == "System" there as well. I'll do a PR to fix their bug.

I'm not familiar with your use case of 'using built-in reflection to generate a binding file ahead of time', so I'd appreciate an example of that.

Same idea as dmd -H and dmd -HC, but not built into the compiler. I made this ages ago and haven't touched it since but same idea: https://github.com/adamdruppe/dtoh the old program uses the json output instead of reflection, but again, same root source of data.

[adamdruppe]

dlang/druntime#3705

[adamdruppe]

BTW if you are concerned about silent breakage, the solution is to still fix the bug, but just output a note similar to a deprecation transition message, to tell people they need to fix their code. So getLinkage could return "System" and do a warning telling people the code was recently fixed and they should ensure their code is also correct.

[dkorpel]

The current behavior crashes if you generate any files with it.

I understand, but I didn't know that transforming D headers with __traits(getLinkage) passing along extern(System) was something people did. But thanks for clearing this up, I'll see if I can fix this in DMD.

[dkorpel]

if you are concerned about silent breakage, the solution is to still
fix the bug,

I'm not concerned for myself since I don't have any skin in this game, I'm just cautious that I'm not just flipping from one broken state to another.

[adamdruppe]

I doubt it would be worth it but if you did want to add another thing, it could be like getBinaryLinkage (or keep this called getLinkage) vs getSourceLinkage but still when the spec has said something and it is a simple little change it doesn't seem worth it to do two traits.

[dkorpel]

and it is a simple little change it doesn't seem worth it to do two traits.

The difficult part is that it's not clear when exactly System stops being its own thing and starts becoming C/Windows.

extern(System) void fsys();
extern(C) void fc();

static assert( typeof(fsys) == typeof(fc) ); // equal on Linux?
static assert( __traits(getLinkage, fsys) == __traits(getLinkage, fc) ); // but this is not equal on Linux

pragma(msg, fsys.mangleof); // special mangle for System, or do we choose C/Windows now?

[adamdruppe]

On Mon, Jan 24, 2022 at 07:18:34AM -0800, Dennis wrote: The difficult part is that it's not clear when exactly System stops being its own thing and starts becoming C/Windows.

The spec says "Returns a string representing the LinkageAttribute of the declaration." The LinkageAttribute is part of the source code. This influences the binary - it can change mangle scheme and calling convention - but it doesn't necessarily do so. (For example, extern(D) and extern(C) might actually be binary compatible, but they are still logically different in the source.)

static assert( typeof(fsys) == typeof(fc) ); // equal on Linux?

Types are aliased but the alias is still important. It is similar to how `size_t` means something else in 32 and 64 bit, but any code that blindly replaces it with `uint` instead of keeping `size_t` is incorrect.

pragma(msg, fsys.mangleof); // special mangle for System, or do we choose C/Windows now?

Mangle is part of the binary interface, and the spec defines that in terms of CallConvention, not LinkageAttribute. Of course, some extern(x) functions do not follow the D mangle scheme at all since they need to interface with external systems. It might be sometimes useful to get the binary compatibility out of the compiler. We have .mangleof to see what specifically it decided to make (which is, again, based on the attribute as well as the scope of the declaration), a binary linkage thing might be useful. What's interesting there is on platforms where System, or even D or C++ is equivalent to C's ABI, it might actually return C.