Qualify diagnostic functions as pragma(printf)

[nordlow]

Will improve type-checking of parameters used in C-printf-style diagnostics formatting.

[dlang-bot]

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

[MoonlightSentinel]

This requires building with -ignore for older host compilers ( < 2.091.1 (?) 2.092.0).

[MoonlightSentinel]

Maybe do something like this:

diff --git a/src/build.d b/src/build.d
index 184b29313..76c6ec9f5 100755
--- a/src/build.d
+++ b/src/build.d
@@ -1007,6 +1007,11 @@ void processEnvironment()
     else
         env["HOST_DMD_KIND"] = "gdc";

+    // Determine version:
+    const verStart = hostDMDVersion.countUntil('v') + 1;
+    const versionNum = hostDMDVersion[verStart .. verStart + "X.XXX.X".length];
+    env["HOST_DMD_VERSION"] = versionNum;
+
     env["DMD_PATH"] = env["G"].buildPath("dmd").exeName;
     env.getDefault("DETAB", "detab");
     env.getDefault("TOLF", "tolf");
@@ -1023,6 +1028,10 @@ void processEnvironment()
     if (env["HOST_DMD_KIND"] != "gdc")
         dflags ~= ["-dip25"]; // gdmd doesn't support -dip25

+    // Ignore
+    if (versionNum < "2.092.0")
+        dflags ~= "-ignore";
+
     // TODO: add support for dObjc
     auto dObjc = false;
     version(OSX) version(X86_64)

EDIT: For dmd as host compiler, others don't work as well. See #11639 for an alternative solution.

[nordlow]

This requires building with -ignore for older host compilers ( < 2.091.1 (?) 2.092.0).

Thanks. I'll try your patch.

[nordlow]

Does the error

src/dmd/errors.d(45): Error: pragma `printf` is missing a terminating `;`

mean that -ignore didn't have any effect in this case?

[MoonlightSentinel]

Found that -ignore did not work for unknown pragmas with bodys until 2.089.1 (fixed by this PR).

Guess we would have to implement another workaround for older host compilers or wait until they become recent enough...

[nordlow]

Doing version-branch each definition of course for now until minimum compiler version is bumped.

There's much error checking to gain here so lets see what people think about this brute-force version-branching.

Is this the dmd version where pragma(printf) was introduced?:
https://dlang.org/changelog/2.092.0.html

Update: We could create wrapper functions that have different versions depending on __VERSION__...

[MoonlightSentinel]

Is this the dmd version where pragma(printf) was introduced?:
https://dlang.org/changelog/2.092.0.html

Think so.

Update: We could create wrapper functions that have different versions depending on __VERSION__...

Seems reasonable.

[nordlow]

@MoonlightSentinel: What do you make of the error https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=4161908&isPull=true? I'm clueless.

[MoonlightSentinel]

Running locally using Ubuntu-WSL (built with -checkaction=context):

core.exception.AssertError@src/dmd/lexer.d(2730): "escape hex sequence has 1 hex digits instead of 2" != "escape hex sequence has 1 hex digits instead of -703763272"       
----------------
??:? _d_assert_msg [0x7fa00f693bf2]
src/dmd/lexer.d:2730 nothrow @nogc bool dmd.lexer.__unittest_L2716_C1().expectDiagnosticHandler(ref const(dmd.globals.Loc), dmd.console.Color, const(char)*, const(char)*, core.internal.vararg.sysv_x64.__va_list_tag*, const(char)*, const(char)*) [0x7fa00f5b1d2f]
src/dmd/errors.d:338 nothrow void dmd.errors.verrorPrint(ref const(dmd.globals.Loc), dmd.console.Color, const(char)*, const(char)*, core.internal.vararg.sysv_x64.__va_list_tag*, const(char)*, const(char)*) [0x7fa00f5a1161]
src/dmd/errors.d:421 _Z6verrorRK3LocPKcP13__va_list_tagS3_S3_S3_ [0x7fa00f5a15c6]
src/dmd/errors.d:59 _Z6_errorRK3LocPKcz [0x7fa00f5a0309]
src/dmd/errors.d:53 _Z5errorRK3LocPKcz [0x7fa00f5a023e]
src/dmd/lexer.d:1230 nothrow dchar dmd.lexer.Lexer.escapeSequence(ref const(dmd.globals.Loc), ref const(char)*) [0x7fa00f5ae7a8]
src/dmd/lexer.d:2742 nothrow void dmd.lexer.__unittest_L2716_C1().test(immutable(char)[], immutable(char)[], dchar, uint) [0x7fa00f5b1da6]
src/dmd/lexer.d:2754 nothrow void dmd.lexer.__unittest_L2716_C1() [0x7fa00f5b17d6]

Win64 passes the test, so it looks like a parameter corruption.

[nordlow]

So I have to duplicate the creation of the va_list aps, then.

[nordlow]

Ready to go, @MoonlightSentinel?

[MoonlightSentinel]

LGTM except that this overload is missing:

dmd/src/dmd/errors.d

Lines 62 to 75 in ea93b5c

	/**
	* Same as above, but allows Loc() literals to be passed.
	* Params:
	* loc = location of error
	* format = printf-style format specification
	* ... = printf-style variadic arguments
	*/
	extern (D) void error(Loc loc, const(char)* format, ...)
	{
	va_list ap;
	va_start(ap, format);
	verror(loc, format, ap);
	va_end(ap);
	}

This overload cannot be marked with pragma(printf) as it's extern(D)so you would have to mark it as extern(C++). But I think you can simply remove it because it's only used once:

dmd/src/dmd/dinifile.d

Line 346 in ace2aee

error(Loc(filename.xarraydup.ptr, lineNum, 0), "Use `NAME=value` syntax, not `%s`", pn);

[nordlow]

LGTM except that this overload is missing:

The call in dinifile.d requires an r-value Loc. Shall I convert the call to use an l-value or add r-value overloads for

extern (C++) void error(const Loc loc, const(char)* format, ...)

or, perhaps, even better, remove the extern(D)-overload and convert all arguments taking Loc to be passed as in value? Loc is 16 bytes wide and will be passed in registers, AFAICT. If we later add more members to Loc the new rules for in in future versions of the host dmd compiler will handle this correctly.

[MoonlightSentinel]

Rewrite it to store a local Loc for now, we can look into optimizations later.

-error(Loc(filename.xarraydup.ptr, lineNum, 0), "Use `NAME=value` syntax, not `%s`", pn); 
+Loc(filename.xarraydup.ptr, lineNum, 0)
+error(loc, "Use `NAME=value` syntax, not `%s`", pn); 

[nordlow]

Rewrite it to store a local Loc for now, we can look into optimizations later.

Using in Loc loc builds ok.

You mean do the transition from const ref to in at larger scale?

[MoonlightSentinel]

Yes, let's stick with the current default (const ref) and discuss optimizations in a dedicated PR.

[nordlow]

Yes, let's stick with the current default (const ref) and discuss optimizations in a dedicated PR.

Done.

[nordlow]

Why does druntime fail to build in buildkite/dmd? I can't see any error message.

Only a warning:

Warning: struct HasNonConstToHash has method toHash, however it cannot be called with const(HasNonConstToHash) this.

[MoonlightSentinel]

Seems like a spurious OOM, restarted the job manually.

Testing printf
--
  | ./generated/linux/debug/64/printf
  | /var/lib/buildkite-agent/builds/ci-agent-a5eb829d-1249-4ef6-a546-0eb660ed8d77-5/dlang/dmd/dmd/generated/linux/release/64/dmd -debug=LOGGING -m64 -fPIC -w -I../../src -I../../import -Isrc -defaultlib= -debuglib= -dip1000 -L-lpthread -L-lm -L/var/lib/buildkite-agent/builds/ci-agent-a5eb829d-1249-4ef6-a546-0eb660ed8d77-5/dlang/dmd/druntime/generated/linux/release/64/libdruntime.a -O -release -unittest -version=CoreUnittest -main -ofgenerated/linux/release/64/logging ../../src/gc/impl/conservative/gc.d ../../src/rt/lifetime.d
  | src/core/exception.d(647): [unittest] Memory allocation failed
  | core.exception.OutOfMemoryError@src/core/exception.d(647): Memory allocation failed
  | ----------------
  | 1/1 modules FAILED unittests
  | Makefile:15: recipe for target 'generated/linux/debug/64/printf.done' failed
  | make[2]: *** [generated/linux/debug/64/printf.done] Error 1

[nordlow]

Seems like a spurious OOM, restarted the job manually.

Is dmd taking too much memory?

[MoonlightSentinel]

Is dmd taking too much memory?

The OOM occurred in the printf executable (but I guess Buildkite just got too many jobs at once there)

[nordlow]

The OOM occurred in the printf executable (but I guess Buildkite just got too many jobs at once there)

Is there no guarantee for the minimum amount of memory available?

[wilzbach]

The OOM occurred in the printf executable (but I guess Buildkite just got too many jobs at once there)

Is there no guarantee for the minimum amount of memory available?

Nope, we run the agents ourselves and to save resources we run multiple agents on one machine (and they share the memory). It usually works out well though ;-)

[nordlow]

What's up with this failure?

[MoonlightSentinel]

What's up with this failure?

No idea but it looks environmental (std.parallelism wasn't changed in the last few days AFAICT)

[nordlow]

No idea but it looks environmental (std.parallelism wasn't changed in the last few days AFAICT)

Ok, thanks. Shall I do a forced push again?

[MoonlightSentinel]

No, I've restarted it manually

[nordlow]

No, I've restarted it manually

Thanks.

[nordlow]

No, I've restarted it manually

Can you restart parts of the autotester or only the whole?

I presume I don't have the permissions to restart individual jobs manually, right?

[MoonlightSentinel]

Can you restart parts of the autotester or only the whole?

Individual jobs (causing less workload than force-pushing)

I presume I don't have the permissions to restart individual jobs manually, right?

No, the auto-tester requires a linked account with mege rights IIRC. (But that situation should improve once we retire the auto-tester inf favour of other CI's, see e.g. #11653).

See below

[wilzbach]

No, the auto-tester requires a linked account with mege rights IIRC. (But that situation should improve once we retire the auto-tester inf favour of other CI's, see e.g. #11653).

Actually, anyone whose PRs can get tested on the auto-tester can also login there and deprecate the builds. Not that it would be important as the builds are constantly restarted. Once "auto-merge" is applied the PR branch will get higher priority and restart the build until it is green.

[wilzbach]

Should have been private and extern(D)