Add build option ENABLE_ASSERTS and use it for CI #11523

kinke · 2020-08-06T19:32:29Z

To finally have DMD's own assertions properly CI-tested.

dlang-bot · 2020-08-06T19:32:31Z

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

kinke · 2020-08-06T19:35:09Z

[I've grepped for ENABLE_RELEASE.]

wilzbach · 2020-08-16T01:02:04Z

src/posix.mak

@@ -99,7 +100,7 @@ else
  HOST_DMD_RUN=$(HOST_DMD) -conf=$(dir $(HOST_DMD))dmd.conf
 endif

-RUN_BUILD = $(GENERATED)/build HOST_DMD="$(HOST_DMD)" CXX="$(HOST_CXX)" OS=$(OS) BUILD=$(BUILD) MODEL=$(MODEL) AUTO_BOOTSTRAP="$(AUTO_BOOTSTRAP)" DOCDIR="$(DOCDIR)" STDDOC="$(STDDOC)" DOC_OUTPUT_DIR="$(DOC_OUTPUT_DIR)" MAKE="$(MAKE)" --called-from-make
+RUN_BUILD = $(GENERATED)/build --called-from-make OS="$(OS)" BUILD="$(BUILD)" MODEL="$(MODEL)" HOST_DMD="$(HOST_DMD)" CXX="$(HOST_CXX)" AUTO_BOOTSTRAP="$(AUTO_BOOTSTRAP)" DOCDIR="$(DOCDIR)" STDDOC="$(STDDOC)" DOC_OUTPUT_DIR="$(DOC_OUTPUT_DIR)" MAKE="$(MAKE)" VERBOSE="$(VERBOSE)" ENABLE_RELEASE="$(ENABLE_RELEASE)" ENABLE_DEBUG="$(ENABLE_DEBUG)" ENABLE_ASSERTS="$(ENABLE_ASSERTS)" ENABLE_UNITTEST="$(ENABLE_UNITTEST)" ENABLE_PROFILE="$(ENABLE_PROFILE)" ENABLE_COVERAGE="$(ENABLE_COVERAGE)" DFLAGS="$(DFLAGS)"


IIRC build.d parses ENABLE_ from both CLI arguments and environment arguments and as GNUMake forwards all environment variables, this shouldn't be necessary?
Or was this done for explicitness?

GNU make apparently automatically exports all variables set in the cmdline to the environment (incl. on Windows), but DM make doesn't (at least not on Windows). I wasn't sure whether DM make is supposed to be supported on Posix systems.

And yeah, explicitness is IMO a nice side-effect (looking at the build cmdline suffices, no need to check earlier make cmdlines).

Well, posix.mak can't even be called with DM make as it uses many features that DM make doesn't support. It will outright fail to even parse the Makefile.

As we are about to finally deprecate these mak files, I'm fine either way. I was just curious, because I thought it wasn't technically necessary.

CC @MoonlightSentinel: you were the one who added the make->build.d call initially. Do you have any objections for or against this?

This line was actually introduced by @marler8997 in e07dc95.

Don't think there is any harm in adding those variables because it doesn't change the behaviour and we're phasing out the makefiles anyway.

Hmm, I thought dm make did add variables to its environment, which is then available to any invoked command. I wrote that program 35 years ago, I guess I'd have to go look at the source code :-)

MoonlightSentinel · 2020-08-16T13:04:25Z

ENABLE_ASSERTS is kinda redundant to manually passing DFLAGS but we can add it for simplicity anyway.

The documentation needs some changes though:

the commit message is misleading because assertions are already checked on some platform: linux (CircleCI) and windows (Azure).
The name is misleading because ENABLE_ASSERTS also enables bound checks, not only asserts (but this might be negligible)

wilzbach · 2020-08-16T15:16:26Z

src/build.d

-        dflags ~= ["-O", "-release", "-inline"];
+        dflags ~= ["-O", "-inline"];
+        if (!env.getNumberedBool("ENABLE_ASSERTS"))
+            dflags ~= ["-release"];


One thing that we shouldn't forget is that -release might generate different code with DMD, so it might be worthwhile to do this only for certain CIs.

Additionally, to tackle this problem on newer CIs there's -check switch, so we conditionally on the host compiler version we could do -release -check=assert=on.

kinke · 2020-08-16T15:33:55Z

the commit message is misleading because assertions are already checked on some platform: linux (CircleCI) and windows (Azure).

The Circle build is a quick one and skips some tests AFAIK. And I don't think any assertions are enabled on Windows, as there was a pretty wild failing assertion not too long ago wrt. FileName which should have triggered.

The name is misleading because ENABLE_ASSERTS also enables bound checks

Well I definitely want bounds checks (in @safe functions) and checked contracts too, I remember bugs being detected this way. Omitting -release is what we've been doing for LDC CI for years.

MoonlightSentinel · 2020-08-16T16:18:19Z

The Circle build is a quick one and skips some tests AFAIK. And I don't think any assertions are enabled on Windows, as there was a pretty wild failing assertion not too long ago wrt. FileName which should have triggered.

Assertions were enabled and caused the --help test to fail but the Dlang bot merged the PR despite the Azure failure.
Anyway, I'm referring to the commit message stating that they are not checked at all.

Well I definitely want bounds checks (in @safe functions) and checked contracts too, I remember bugs being detected this way. Omitting -release is what we've been doing for LDC CI for years.

Me too, I was just concerned that the name could be confusing (although the probability should be quite low).

Anyway, those points were just bugging me personally, so don't let them stop this improvement.

rainers · 2020-08-16T18:28:23Z

And I don't think any assertions are enabled on Windows, as there was a pretty wild failing assertion not too long ago wrt. FileName which should have triggered.

Windows_LDC_Debug x64-debug-ldc builds dmd with ldmd2 -debug -g -boundscheck=on

kinke · 2020-08-16T20:26:23Z

I've pushed a little test to see where the assertions are still disabled - and those are the 2 non-debug MSBuild-based Azure jobs [the debug job hangs after hitting the assertion and doesn't cleanly terminate]. Not sure whether the auto-tester will process the remaining outstanding jobs.

So assertions have been enabled for the trimmed-down testsuite for CircleCI on Linux x64, and the (full?) testsuite for 1 Azure job on a Win64 host with MSVC runtime, and now basically all others (incl. 32-bit hosts, MacOS, FreeBSD, OMF/DMC) feature these checks too, with one notable exception: Win32 with MSVC runtime, and I think that corresponds to the prebuilt 32-bit Windows binaries.

rainers · 2020-08-17T06:46:43Z

with one notable exception: Win32 with MSVC runtime, and I think that corresponds to the prebuilt 32-bit Windows binaries.

Not sure if that's what you meant by "prebuilt", but the passing builds are using the VS projects to build dmd. ENABLE_ASSERTS has no effect on these.

kinke · 2020-08-17T06:55:32Z

I meant that that platform is the only one which isn't CI-tested with enabled assertions, and if the official Win32 DMD binaries are built with LDC, it would IMO be more important to test than Win32 OMF (tested twice with enabled assertions - Azure + auto-tester).

rainers · 2020-08-17T07:15:55Z

Replacing

<DebugCode>Release</DebugCode>

with

<DebugCode>Default</DebugCode>

in the VS project removes "-release" from the command line.

kinke · 2020-08-18T08:53:48Z

I've added a RelWithAsserts VS configuration, which uses that <DebugCode>Default</DebugCode> (and, FWIW, doesn't predefine NDEBUG for C++), and use it for Azure; now all Azure jobs fail as intended.

kinke · 2020-08-18T09:04:01Z

Anyway, I'm referring to the commit message stating that they are not checked at all.

Msg revised.

To finally have DMD's own assertions properly CI-tested. Assertions have so far only been enabled for CircleCI (reduced testsuite on Linux x64) and one Azure job (Win64 debug).

Required for DigitalMars make, which doesn't export the variables set in the make cmdline to the environment.

A copy of the Release configuration, except for omitting `-release` (`<DebugCode>Default</DebugCode>`) [and not predefining NDEBUG for C++].

kinke · 2020-08-24T08:42:43Z

Rebased, in case the previous unrelated CI hiccups have been the reason this isn't merged yet.

MoonlightSentinel · 2020-08-24T08:54:19Z

LGTM except https://github.com/dlang/dmd/pull/11523/files#r471124647

MoonlightSentinel · 2020-08-24T09:00:58Z

ci.sh

@@ -45,7 +45,7 @@ clone() {
 # build dmd, druntime, phobos
 build() {
    source ~/dlang/*/activate # activate host compiler
-    make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=$DMD ENABLE_RELEASE=1 ENABLE_WARNINGS=1 all
+    make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=$DMD ENABLE_RELEASE=1 ENABLE_ASSERTS=1 ENABLE_WARNINGS=1 all


This should probably be manually enabled per CI by passing ENABLE_ASSERTS=1 as an environment variable.

(To tackle https://github.com/dlang/dmd/pull/11523/files#r471124647)

I don't get your point, and I thought we've established that omitting -release is the way to go, better than to rely on host-compiler-version-specific -check switches. -release is IMO there for a reason and is the standard way of en/disabling default safety checks.

To quote @wilzbach:

One thing that we shouldn't forget is that -release might generate different code with DMD, so it might be worthwhile to do this only for certain CIs.

There are still releases compiled by dmd + -release IIRC, so we should test it at least on one CI. Or am I missing a CI that isn't affected by your changes?

EDIT: There's BuildKite but it doesn't run DMD's test suite

Oh sry I misunderstood; TBH, I couldn't imagine checked assertions etc. to have side effects wrt. generated code, but well, it most likely cannot be ruled out. I'm not too familiar with DMD's CI and what each one does exactly, so someone else pls take care of selecting the job(s) to be built without assertions.

Me neither but better safe than sorry,

I would suggest removing ~~this line~~ all changes in this file s.t. SemaphoreCI remains in it's current state and all other CI's run without -release.

To restore some CI coverage with disabled assertions as suggested by Seb & Florian.

MoonlightSentinel

Can't comment on the VS-related changes but the rest LGTM

kinke requested review from MartinNowak and wilzbach as code owners August 6, 2020 19:32

kinke force-pushed the asserts branch 2 times, most recently from fff6b34 to 5871c91 Compare August 6, 2020 20:28

kinke force-pushed the asserts branch from 1dfeea4 to 2936575 Compare August 15, 2020 15:42

ghost approved these changes Aug 15, 2020

View reviewed changes

wilzbach reviewed Aug 16, 2020

View reviewed changes

kinke requested review from Geod24, rainers, UplinkCoder and WalterBright as code owners August 16, 2020 19:26

kinke force-pushed the asserts branch from aa55354 to 8bc0873 Compare August 18, 2020 09:03

kinke force-pushed the asserts branch 3 times, most recently from f337c88 to 5008c19 Compare August 19, 2020 18:51

WalterBright approved these changes Aug 19, 2020

View reviewed changes

Add build option ENABLE_ASSERTS and use it for CI

07ec5cd

To finally have DMD's own assertions properly CI-tested. Assertions have so far only been enabled for CircleCI (reduced testsuite on Linux x64) and one Azure job (Win64 debug).

kinke added 2 commits August 24, 2020 10:41

Make sure to forward all Makefile options to build.d

217f809

Required for DigitalMars make, which doesn't export the variables set in the make cmdline to the environment.

Add RelWithAsserts VS configuration and use it for Azure CI

a468d13

A copy of the Release configuration, except for omitting `-release` (`<DebugCode>Default</DebugCode>`) [and not predefining NDEBUG for C++].

kinke force-pushed the asserts branch from 5008c19 to a468d13 Compare August 24, 2020 08:41

MoonlightSentinel reviewed Aug 24, 2020

View reviewed changes

Semaphore CI: Revert to building DMD with -release

8c77422

To restore some CI coverage with disabled assertions as suggested by Seb & Florian.

MoonlightSentinel approved these changes Aug 27, 2020

View reviewed changes

PetarKirov approved these changes Aug 28, 2020

View reviewed changes

PetarKirov added auto-merge Build System Continuous Integration labels Aug 28, 2020

dlang-bot merged commit eb6836f into dlang:master Aug 28, 2020

kinke deleted the asserts branch August 28, 2020 11:28

wilzbach mentioned this pull request Sep 7, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build option ENABLE_ASSERTS and use it for CI #11523

Add build option ENABLE_ASSERTS and use it for CI #11523

kinke commented Aug 6, 2020 •

edited

Loading

dlang-bot commented Aug 6, 2020

kinke commented Aug 6, 2020

wilzbach Aug 16, 2020

kinke Aug 16, 2020 •

edited

Loading

wilzbach Aug 16, 2020

MoonlightSentinel Aug 16, 2020 •

edited

Loading

WalterBright Aug 19, 2020

MoonlightSentinel commented Aug 16, 2020 •

edited

Loading

wilzbach Aug 16, 2020

kinke commented Aug 16, 2020 •

edited

Loading

MoonlightSentinel commented Aug 16, 2020

rainers commented Aug 16, 2020

kinke commented Aug 16, 2020 •

edited

Loading

rainers commented Aug 17, 2020

kinke commented Aug 17, 2020

rainers commented Aug 17, 2020

kinke commented Aug 18, 2020 •

edited

Loading

kinke commented Aug 18, 2020

kinke commented Aug 24, 2020

MoonlightSentinel commented Aug 24, 2020

MoonlightSentinel Aug 24, 2020

kinke Aug 24, 2020

MoonlightSentinel Aug 24, 2020 •

edited

Loading

kinke Aug 24, 2020

MoonlightSentinel Aug 24, 2020 •

edited

Loading

kinke Aug 27, 2020

MoonlightSentinel left a comment

Add build option ENABLE_ASSERTS and use it for CI #11523

Add build option ENABLE_ASSERTS and use it for CI #11523

Conversation

kinke commented Aug 6, 2020 • edited Loading

dlang-bot commented Aug 6, 2020

Bugzilla references

Testing this PR locally

kinke commented Aug 6, 2020

Choose a reason for hiding this comment

kinke Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonlightSentinel Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonlightSentinel commented Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

kinke commented Aug 16, 2020 • edited Loading

MoonlightSentinel commented Aug 16, 2020

rainers commented Aug 16, 2020

kinke commented Aug 16, 2020 • edited Loading

rainers commented Aug 17, 2020

kinke commented Aug 17, 2020

rainers commented Aug 17, 2020

kinke commented Aug 18, 2020 • edited Loading

kinke commented Aug 18, 2020

kinke commented Aug 24, 2020

MoonlightSentinel commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonlightSentinel Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonlightSentinel Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonlightSentinel left a comment

Choose a reason for hiding this comment

kinke commented Aug 6, 2020 •

edited

Loading

kinke Aug 16, 2020 •

edited

Loading

MoonlightSentinel Aug 16, 2020 •

edited

Loading

MoonlightSentinel commented Aug 16, 2020 •

edited

Loading

kinke commented Aug 16, 2020 •

edited

Loading

kinke commented Aug 16, 2020 •

edited

Loading

kinke commented Aug 18, 2020 •

edited

Loading

MoonlightSentinel Aug 24, 2020 •

edited

Loading

MoonlightSentinel Aug 24, 2020 •

edited

Loading