fix Issue 20457 - Asserts must not be turned off in release builds of dmd

[WalterBright]

No description provided.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✗	20457	regression	Asserts must not be turned off in release builds of dmd

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

[wilzbach]

-check=assert isn't available in all supported host compilers. That's why some CIs are failing. Though it looks like it's only gdc, so you might be able to just version out the flag for gdc.

[MoonlightSentinel]

Does not work until the minmal compiler used by the auto-tester becomes 2.084+.

As an intermediate solution -release could be replaced with -boundscheck=safeonly. That would keep asserts and other contracts but would change the default assertion failure handling from C to D

[WalterBright]

-check=assert isn't available in all supported host compilers.

Can build.d be changed to bootstrap with one set of flags, and then do the final build with a different set and compile itself with the latest version?

This disabling of assert's is a serious problem.

[wilzbach]

Can build.d be changed to bootstrap with one set of flags, and then do the final build with a different set and compile itself with the latest version?

I guess, but I thought it's the point of the build script to be fast.
As mentioned two easy solutions:

1. version (GDC)
2. use host compiler version via VERSION (--version)

[WalterBright]

@wilzbach (1) and (2) are set by the compiler that compiles build.d, not the compiler that build.d runs once it is compiled.

[WalterBright]

This causes the semaphoreci DMD x86 build to fail with:

++ diff -pu --strip-trailing-cr compilable/extra-files/header2i.di test_results/compilable/testheader2i.di.2
--- compilable/extra-files/header2i.di	2019-12-20 10:07:14.476150082 +0000
+++ test_results/compilable/testheader2i.di.2	2019-12-20 10:19:34.141602203 +0000
@@ -117,7 +117,7 @@ void test13275()
 	if (auto n = 1)
 	{
 	}
-	if (const n = 1)
+	if (const @trusted n = 1)
 	{
 	}
 	if (immutable n = 1)

which seems totally weird to me. I have no idea what makes the semaphoreci DMD x86 build different. @wilzbach do you have an idea?

[wilzbach]

I have no idea what makes the semaphoreci DMD x86 build different. @wilzbach do you have an idea?

Sorry, but I don't have an idea either :/
Edit: this was a response to why it fails with that diff.

[WalterBright]

If it isn't different, why is it being tested? Who made semaphoreci? Who maintains it?

[wilzbach]

It tests that you can actually compile and run D code with the latest D compiler (not 2.079) + LDC + GDC. It does this via a two-stage bootstrap process: compile DMD with latest DMD. Use this DMD to compile DMD and then run the testsuite.
So what you're done no longer works with master. Aren't you able to reproduce it locally? You can even ssh into the build machines.

SemaphoreCI is built and maintained by the people from Semaphore.
It runs the same tests that used to run on Travis.

[WalterBright]

So whatever you're done no longer works with master.

I removed the -release flag from build.d. That's it.

[WalterBright]

@wilzbach thanks for the explanation. I'm interested in which of us is building/maintaining the semaphoreci tests, not who the semaphoreci organization is.

[WalterBright]

To confirm, the failing test is using the 32 bit Linux DMD to build the 32 bit Linux DMD?

[WalterBright]

I am finally able to repro it.

[WalterBright]

Found the problem: #10696

[WalterBright]

I refactored the code a bit so it will not trigger https://issues.dlang.org/show_bug.cgi?id=20466 and compile successfully with older compilers.

[WalterBright]

@wilzbach this is ready to go.

[rainers]

Last time this was tried it was deemed to be too expensive: #7344 (comment)

Checking the autotester, the effect is visible in the logs (i.e. search for 10679 here https://auto-tester.puremagic.com/hosts/details.ghtml?hostid=53 or here https://auto-tester.puremagic.com/hosts/details.ghtml?hostid=46 ): builds are about 5-10% slower with this PR.

[WalterBright]

builds are about 5-10% slower with this PR.

I know, that is because of the ongoing trend to make assert()s ever more expensive. This is why the checkaction= switches were added, to go back to cheap asserts. Unfortunately, the release build of dmd is done with an old DMD that doesn't support that switch. It needs to be done with a newer DMD. Even the cheapest (execute a HALT instruction) is better than none.

There are also some poor placements of assert()s in hot code in DMD.

[rainers]

This is why the checkaction= switches were added, to go back to cheap asserts.

IMO the reporting code has very little impact on the execution of the condition. (I know there is some difference but I expect it to be hardly notable in build times). The major difference is code size.

There are also some poor placements of assert()s in hot code in DMD.

That is definitely more likely to be an issue.

BTW: the Azure x64-debug-dmd build also runs the tests with all debug code enabled.

[ibuclaw]

Yes, you can add -check= and -checkaction= as accepted but ignored arguments in the gdmd script (although -check=assert is equivalent to -fassert in gdc).

[UplinkCoder]

The change to hdrgen seems unrelated and should be explained.

[MoonlightSentinel]

This found an OSX failure:

Test runnable/test16096.sh failed. The logged output:
+ source runnable/test16096.sh
++ '[' osx '!=' osx ']'
++ '[' 64 '!=' 64 ']'
++ /Users/braddr/sandbox/at-client/pull-4130450-Darwin_64_64/dmd/generated/osx/release/64/dmd -Irunnable/extra-files -ofgenerated/runnable/test16096.a -lib runnable/extra-files/test16096a.d
runnable/extra-files/test16096a.d(6): Deprecation: interface `test16096a.Class` Objective-C interfaces have been deprecated
runnable/extra-files/test16096a.d(6):        Representing an Objective-C class as a D interface has been deprecated. Please use `extern (Objective-C) extern class` instead
runnable/extra-files/test16096a.d(12): Deprecation: interface `test16096a.NSObject` Objective-C interfaces have been deprecated
runnable/extra-files/test16096a.d(12):        Representing an Objective-C class as a D interface has been deprecated. Please use `extern (Objective-C) extern class` instead
Assertion failed: (i < SegData.length), function dmd.backend.machobj.Obj_data_start, file /Users/braddr/sandbox/at-client/pull-4130450-Darwin_64_64/dmd/src/dmd/backend/barray.d, line 172.
runnable/test16096.sh: line 8: 71655 Abort trap: 6           $DMD -I${EXTRA_FILES} -of${OUTPUT_BASE}${LIBEXT} -lib ${EXTRA_FILES}/test16096a.d
+ finish 134
+ set +x
==============================
Test runnable/test16096.sh failed. The xtrace output:

[Geod24]

https://issues.dlang.org/show_bug.cgi?id=21082

[ghost]

You could remove some of the poorly placed assert that you mentioned, with this PR, just as example.

[wilzbach]

This shouldn't be merged. See #11523 for a vastly better approach.

[wilzbach]

@NilsLankila why do you ask for our opinions via the review feature and simultaneously add "auto-merge"?
That doesn't leave much time to reply :/

[wilzbach]

Superseded by #11523