fix Issue 20906 - unnecessary divide-by-zero errors when constant fol…

[WalterBright]

…ding short circuits

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20906	normal	unnecessary divide-by-zero errors when constant folding short circuits

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

[WalterBright]

CyberShadow/DAutoTest — Build failed

No clue in the log file why it failed.

ci/circleci: build — Your tests failed on CircleCI

Appears to not give enough time for the tests to complete.

[thewilsonator]

No clue in the log file why it failed.

Look closer.

Failed to download package taggedalgebraic from https://code.dlang.org/packages/taggedalgebraic/0.10.11.zip

Network error.

[WalterBright]

There are 6 Failed to messages in the log file, none of which match what you posted.

[MoonlightSentinel]

Appears to not give enough time for the tests to complete.

No, your patch seems to trigger an infinite loop during self-compilation when building the lexer target.

[WalterBright]

Consider:

https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=4077946&isPull=true

It times out. I have no idea which test timed out.

[WalterBright]

How do I fix the tests to run serially instead of in parallel?

[MoonlightSentinel]

Pass -j1 to run.d

[WalterBright]

Where's the command to run.d?

[WalterBright]

test/run.d does not appear to have a -J switch.

[MoonlightSentinel]

You can execute rdmd run.d inside of the test directory. Passing --help will also print some usage information.

[WalterBright]

Thank you, @MoonlightSentinel
You da man!
I'll try this out later today.

[WalterBright]

@MoonlightSentinel test case works now. A new problem has appeared: the windows tests are timing out. Log files give no clue where.

[Geod24]

Do you have a Win64 to reproduce ? Also you can try to run with -j1 to make sure the last test showing up is the one timing out.
Your best bet is to add a debug commit to set this to 1:

dmd/test/run.d

Line 91 in ca636dc

int jobs = totalCPUs;

[MoonlightSentinel]

Running the test suite with enabled assertions:

 ... runnable\builtin.d              -L/OPT:NOICF (-inline -release -g -O)
Test runnable\builtin.d failed.  The logged output:

generated\windows\release\64\dmd.exe -conf= -m64 -Irunnable  -L/OPT:NOICF  -odtest\test_results\runnable -oftest\test_results\runnable\builtin_0.exe  runnable\builtin.d
test\test_results\runnable\builtin_0.exe
0x1.f9f8d9aea10fep-2
0x1.f9f8d9aea10fep-2
0x1.bd21aaf88dcfap-1
0x1.bd21aaf88dcfap-1
0x1.22fd752af75cdp-1
0x1.22fd752af75cdp-1
Success

generated\windows\release\64\dmd.exe -conf= -m64 -Irunnable  -L/OPT:NOICF -inline -odtest\test_results\runnable -oftest\test_results\runnable\builtin_1.exe  runnable\builtin.d
Assertion failed: (count += 1) < 20, file src\dmd\backend\cgcod.d, line 2058

==============================
Test runnable\builtin.d failed: expected rc == 0, exited with rc == 9

The tests timeouts without assertions.

[WalterBright]

The tests timeouts without assertions.

The compiler is designed to run with assertions on. There's a PR to fix that: #10679

[WalterBright]

@MoonlightSentinel thanks for finding the case, I'll check it out.

[WalterBright]

@Geod24 @MoonlightSentinel What I'm asking for is for the test runner to print:

dmd builtin.d
error: timed out after NNNN seconds

The purpose of a test suite is to not simply fail when it doesn't pass, but to aid in finding where it failed. I've posted many grumpy messages about this in recent weeks.

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

[WalterBright]

@MoonlightSentinel I am having problems reproing it, as it works on my machine. builtin.d is a pretty small file. Is it possible you can reduce builtin.d down? The problem has something to do with the special register calling convention Win64 uses. Thanks!

[WalterBright]

meanwhile, I should figure out why my build is different

[Geod24]

@WalterBright : The timeout is not handled by the test runner, but by the CI provider. The auto-tester has a 1h timeout, Azure has 2h, etc... In order to do what you ask for, we would need to move the timeout feature to the test runner.

[WalterBright]

Moving it to the test runner sounds good to me.

This is not the first nor will be the last time this problem crops up and wastes several peoples' time. I've asked Brad to add "test suite" as a component in Bugzilla so I can start filing issues for all the problems I've been having with near-useless log files, heisenbugs, undocumented tests, scroll bars that are too small to be grabbed with the mouse, etc.

Because, after all, if it isn't in bugzilla it'll never get fixed :-)

[WalterBright]

@MoonlightSentinel never mind, I was able to duplicate it.

[WalterBright]

Finally! Working now.

[v1ne]

Why don't you also use d?

Suggested change

	return a | b | c;
	return a | b | c | d;

[WalterBright]

It doesn't matter for the test.

[WalterBright]

@CyberShadow this was passing CyberShadow/DAutoTest yesterday, now it is failing. What can we do to make it reliable?

[CyberShadow]

What can we do to make it reliable?

Generally:

• Pin all dependencies
• Don't force-merge broken PRs
• Avoid introducing further dependencies between D components
• Ensure build scripts are deterministic and don't have any race conditions which would affect the outcome

It doesn't look like any of the above apply in this case:

-----------------------------------------------------------
�[1msrc/dmd/templateparamsem.d(37): �[1;33mWarning: �[mDdoc: function declaration has no parameter 'tp'
�[1msrc/dmd/templateparamsem.d(37): �[1;33mWarning: �[mDdoc: function declaration has no parameter 'sc'
�[1msrc/dmd/templateparamsem.d(37): �[1;33mWarning: �[mDdoc: function declaration has no parameter 'parameters'

Why does that error appear now but (presumably) not previously? A regression introduced by this PR? A non-deterministic bug in DMD? In any case it doesn't look like a problem with the CI itself.

[CyberShadow]

Avoid introducing further dependencies between D components

It looks like a problem with the interaction of the CI and the dlang.org build scripts. The CI observed that it had built this commit before, but when trying to test it, the website then stupidly tries to rebuild DMD from source, but the wrong DMD version is checked out at that point.

[CyberShadow]

I think it should be fixed in CyberShadow/ae@3dd32f1 . Thanks for this bug report. I manually queued a retest for this PR.

[WalterBright]

Why does that error appear now but (presumably) not previously?

I have no idea. The file also looks correct to me.

[WalterBright]

I think it should be fixed

Ah, wonderful! Thanks for the quick action!