[Memory Corruption] add toTypeFunction() to catch bad casts

[WalterBright]

I finally got caught with a bad cast to TypeFunction resulting in memory corruption. Fixed the test, and replaced the cast with a function call to always check. The failing case was ice13311.d, so no need to add another test case.

Memory corruption is always serious, so this should get priority.

[UplinkCoder]

Since this should never trigger in a release-build
make it assert(ty == Tfunction, "toTypeFunction only works on functions");

[WalterBright]

The assert(0) will trigger in release builds, this is deliberate. I don't need the message (which will be useless to anyone who doesn't know the compiler internals).

[wilzbach]

FWIW we already had two similar discussions

• for Phobos there's consensus that all assert's should have an error message (e.g. 1, 2 ...)
• for DMD it has been agreed that having a custom assert handler that points to bugzilla, would be very user-friendly.
  Unfortunately this PR went nowhere as all the assert(0)'s trigger SEGFAULTs in release mode ...

[UplinkCoder]

@WalterBright It'll be useful to someone debugging a problem in the compiler.
If you insist on assert(0) using a proper error messages is even more important such that the user does atleast have something to put into bugzilla when reporting the crash.

[dnadlinger]

@UplinkCoder: assert(0) is lowered to a hlt/ud2/... in release mode, so it won't print any error message.

[WalterBright]

It'll be useful to someone debugging a problem in the compiler.

I've been debugging the compiler for 30 years, and such information adds nothing to what the source code and comments around the assert tells you.

atleast have something to put into bugzilla

There is - an assert message giving file and line.

[WalterBright]

assert(0) is lowered to a hlt/ud2/... in release mode, so it won't print any error message.

That's right. It's still there, though, doing its job, unlike assert(ty == Tfunction) which would just corrupt memory in release mode. (asserts should be left on in the compiler in release mode).

I put this assert in this way BECAUSE I was getting silent memory corruption, and who knows how long that has been going on. Fortunately, it would only happen if the compiler had already diagnosed errors, so at least it wasn't corrupting clean compiles.

[andralex]

I've been debugging the compiler for 30 years

There'd be a funny interpretation of this somewhere :)

[WalterBright]

All this discussion about assert messages, what to do with asserts, what to do with release mode, is all good, but is all off topic for this PR. This PR is about solving a memory corruption issue in the compiler. It needs to be reviewed as such, and pulled in a timely manner.

[mathias-lang-sociomantic]

This PR is about solving a memory corruption issue in the compiler. It needs to be reviewed as such, and pulled in a timely manner.

No, most of the changes are not about fixing a memory corruption.
Do I assume correctly that the following is the fix ?

+        if (type.ty != Tfunction)
+        {
+            if (type.ty != Terror)
+            {
+                error("%s must be a function instead of %s", toChars(), type.toChars());
+                type = Type.terror;
+            }
+            errors = true;
+            return;
+        }

So we have 9 additional lines which fixes the problem, yet this P.R. is +61 / -55.

[WalterBright]

No, most of the changes are not about fixing a memory corruption.

They are about detecting it, and they did detect it.

Do I assume correctly that the following is the fix ?

Yes.

[ibuclaw]

Left out headers.