rt.config: Compatibility with GDC '-fno-weak-templates' option

[ssvb]

The rt.config module provides a set of configuration variables,
with various ways to override them as documented here:
https://dlang.org/phobos/rt_config.html

The desirable assembly output for the 'rt_cmdline_enabled'
variable looks like this (that's what is now generated by
default):

        .weak   rt_cmdline_enabled
        .data
        .type   rt_cmdline_enabled, @object
        .size   rt_cmdline_enabled, 1
rt_cmdline_enabled:
        .byte   1

But unfortunately when GDC11 or GDC12 is used with the
'-fno-weak-templates' option, the assembly output for
the 'rt_cmdline_enabled' variable changes to:

        .weak   rt_cmdline_enabled
        .section .data.rt_cmdline_enabled,"awG",@progbits,rt_cmdline_enabled,comdat
        .type   rt_cmdline_enabled, @gnu_unique_object
        .size   rt_cmdline_enabled, 1
rt_cmdline_enabled:
        .byte   1

And this results in "multiple definition of `rt_cmdline_enabled';
/tmp/ccc5MZMh.o:(.bss+0x0): first defined here" linker error when
trying to compile the following small program:

import std.stdio;
extern(C) __gshared bool rt_cmdline_enabled = false;
void main(string[] args) { writeln(args); }

This patch solves the problem by setting GDC-specific
@Attribute("weak") for these variables instead of having
them enclosed in a "template { }" block. The assembly output
is now always desirable in both '-fweak-templates' and
'-fno-weak-templates' configurations.

There are very strong reasons to prefer '-fno-weak-templates',
because this allows inlining template functions. See:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102765

[dlang-bot]

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

[thewilsonator]

cc @ibuclaw

[ssvb]

This appears to be a little bit more complicated. At least GDC 9/10 versions need to use import gcc.attribute instead of import gcc.attributes (and GDC 9 is still useful for bootstrapping GDC 12, so we can't forget about it yet). I wonder why CI checks haven't caught this problem? Also what would be the best solution? Maybe just use import gcc.attribute and ignore the deprecation warnings spewed by the newer versions of GDC? I also can't test mingw and wonder if gcc.attribute would work there.

Another thing is that the bundled copies of druntime in GDC 9/10/11 are missing ae9581c and currently mutilate the command line arguments regardless of the value of the rt_cmdline_enabled variable. But it's just a minor tweak. And other than this, GDC 10 seems to work perfectly: it can inline template functions and it has a properly working rt.config

Also GDC 11 is still problematic. I tried to flip the configuration default from '-fweak-templates' to '-fno-weak-templates' in GDC (because fixing template functions inlining is the actual goal), but the compiled toolchain becomes defunct and can't compile applications. The linker complains about missing symbols from Phobos. I'm going to quickly check if reverting all of this '-fweak-templates' / '-fno-weak-templates' stuff in GDC 11/12 back to how it was in GDC 10 may help, but any feedback/advice from @ibuclaw would be very useful.

[Geod24]

You probably want to use core.attribute and can loose the version.

druntime/src/core/attribute.d

Lines 40 to 66 in 348a4ff

	/**
	* Use this attribute to specify that a global symbol should be emitted with
	* weak linkage. This is primarily useful in defining library functions that
	* can be overridden by user code, though it can also be used with shared and
	* static variables too.
	*
	* The overriding symbol must have the same type as the weak symbol. In
	* addition, if it designates a variable it must also have the same size and
	* alignment as the weak symbol.
	*
	* Quote from the LLVM manual: "Note that weak linkage does not actually allow
	* the optimizer to inline the body of this function into callers because it
	* doesn’t know if this definition of the function is the definitive definition
	* within the program or whether it will be overridden by a stronger
	* definition."
	*
	* This attribute is only meaningful to the GNU and LLVM D compilers. The
	* Digital Mars D compiler emits all symbols with weak linkage by default.
	*/
	version (DigitalMars)
	{
	enum weak;
	}
	else
	{
	// GDC and LDC declare this attribute in their own modules.
	}

I wonder why CI checks haven't caught this problem?

CI doesn't check the GDC build (only DMD built with GDC, not GDC building itself), and Iain does manual merges frequently. So whatever you put in version (GNU), as long as it's syntactically correct, will pass. Same with version (LDC).

[ssvb]

You probably want to use core.attribute and can loose the version.

That's a good point and thanks for the link. The patch can be changed to have no special path for GDC and use weak attributes instead of template { } for all compilers. My only concern is that now all compilers will be affected and it will be necessary to watch for regressions. Whereas the GDC-specific tweak should be perfectly safe at least in Linux, because I tested it and also confirmed the symbol section and attributes in the generated assembly source.

I can give it a try. Should I force-push the updated patch to see what the CI says about it?

I wonder why CI checks haven't caught this problem?

CI doesn't check the GDC build (only DMD built with GDC, not GDC building itself), and Iain does manual merges frequently. So whatever you put in version (GNU), as long as it's syntactically correct, will pass. Same with version (LDC).

I actually made a mistake and incorrectly assumed that the gcc.attributes module comes from the host compiler (hence GDC 9 wouldn't have it available when compiling druntime). But gcc.attributes is a part of druntime itself (edit: a part of the druntime copy that is bundled with GDC), so this wasn't an issue in the first place. Now I compiled GDC 12 by GDC 9 to confirm this and it was just fine.

As for my earlier comment about

I'm going to quickly check if reverting all of this '-fweak-templates' / '-fno-weak-templates' stuff in GDC 11/12 back to how it was in GDC 10 may help

Reverting these 3 patches in GDC 11 does the job and makes template functions inlineable again without any visible problems.

[ssvb]

Looks like the forkgc2(9225,0x70000020a000) malloc: *** error for object 0x7fa979503130: double free CI failure is just another case of https://issues.dlang.org/show_bug.cgi?id=22238

[kinke]

The patch can be changed to have no special path for GDC and use weak attributes instead of template { } for all compilers. My only concern is that now all compilers will be affected and it will be necessary to watch for regressions

LDC should be fine. DMD might still need the template hack for Windows targets. I suggest simply adding the @weak UDA (and to drop the wrapper functions - seem useless?) and see what happens in CI - I think there are some tests for these rt options, and I'd expect failures on Windows. If so, we know it's CI-tested and can restore the templates for version (DigitalMars) version (Windows) or so.

[RazvanN7]

ping @ssvb

[RazvanN7]

@ssvb what is the status of this PR?