Point users to Bugzilla on ICEs #8138

wilzbach · 2018-04-05T03:11:43Z

Revival of #6103.

While there's still a bit of hold-up on DIP1006 being implemente see e.g. #7980, we can already prepare the codebase today.

Also LDC already allows to selectively enable assert's in -release today.

ICEs to test this:

Frontend

///
template allSameType()
{
    static foreach (idx; T)
            enum allSameType ;
}

(from: https://issues.dlang.org/show_bug.cgi?id=18721)

> dmd -D
This is a compiler bug, please report it via https://issues.dlang.org/enter_bug.cgi
core.exception.AssertError@dmd/cond.d(378): Assertion failure
----------------
??:? _d_assertp [0x4f6c81b5]
??:? pure nothrow @nogc @safe void dmd.cond.StaticForeach.prepare(dmd.dscope.Scope*).__require() [0x4f4bb637]
??:? void dmd.cond.StaticForeach.prepare(dmd.dscope.Scope*) [0x4f4bb4f4]
??:? _ZN24StaticForeachDeclaration7includeEP5Scope [0x4f4b4b5b]
...

Backend

alias A = Complex!double;

struct Complex(T)
{
    double re, im;

    Complex!double baz()
    {
        return Complex!double(re).blah;
    }

    ref Complex!double blah()
    {
        im = re;
        return this;
    }
}

(from: https://issues.dlang.org/show_bug.cgi?id=15206)

> dmd -c -inline -O -m64
tym = x1d
Internal error: dmd/backend/cgxmm.c 684
This is a compiler bug, please report it via https://issues.dlang.org/enter_bug.cgi

Other ideas:

How about dumping all global data too? (or just a long variant of --version)?

dlang-bot · 2018-04-05T03:11:44Z

Thanks for your pull request, @wilzbach!

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 fetch digger
dub run digger -- build "master + dmd#8138"

JinShil · 2018-04-06T08:49:41Z

How about dumping all global data too? (or just a long variant of --version)?

Yeah, I think it would be helpful to output some more information. Compiler version would be good. I'm not sure what you mean by global.

JinShil · 2018-04-06T08:51:05Z

I think you should also consider that this code may be shared by LDC and GDC which use different bug trackers. It'd be nice to make is easy for them to adapt it to their workflows.

wilzbach · 2018-04-06T09:01:06Z

Yeah, I think it would be helpful to output some more information. Compiler version would be good. I'm not sure what you mean by global.

I was thinking about something along this:

> dmddev -Xf=- -Xi=compilerInfo
{
 "compilerInfo" : {
  "vendor" : "Digital Mars D",
  "version" : "v2.079.0-372-gc16061fe4",
  "__VERSION__" : 2079,
  "interface" : "dmd",
  "size_t" : 8,
  "platforms" : [
   "posix"
  ],
  "architectures" : [
   "x86_64"
  ],
  "predefinedVersions" : [],
  "supportedFeatures" : {
   "includeImports" : true
  }
 }
}

or:

> echo "foo" | dmd -v -
predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    dmd
version   v2.079.0
config    /etc/dmd.conf
DFLAGS    (none)
parse     __stdin
...

I think you should also consider that this code may be shared by LDC and GDC which use different bug trackers. It'd be nice to make is easy for them to adapt it to their workflows.

The luckily don't use the same backend and the frontend check is directly in dmd's own main method in mars.d.
I think GDC doesn't even use mars.d at all, and I think LDC re-uses some bits of it (?), but definitely not the general main method.
Anyhow, it's totally up to GDC and LDC how they want to handle assertion failures in the frontend.

It shouldn't be too hard to write scope(failure) themselves if they want to
DMD is currently still shipped with assertions turned off in the frontend (this hopefully might change soon though), but AFAICT there's nothing stopping LDC from shipping their binaries with their -enable-asserts flag

JinShil · 2018-04-06T09:04:56Z

> echo "foo" | dmd -v -
predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    dmd
version   v2.079.0
config    /etc/dmd.conf
DFLAGS    (none)
parse     __stdin
...

I think I'd prefer that format over the json.

JinShil · 2018-04-06T09:05:50Z

Is a stack trace going too far?

JinShil · 2018-04-06T09:10:36Z

Also, I suggest "This is a compiler bug, please report it via https://issues.dlang.org/enter_bug.cgi with, preferably, a reduced, reproducible example." or something like that to encourage the user to do a little more than just cut and paste the compiler output into the bug tracker.

wilzbach · 2018-04-06T10:02:17Z

Okay here's how it will look with the new changes:

---
ERROR: This is a compiler bug.
Please report it via https://issues.dlang.org/enter_bug.cgi
with, preferably, a reduced, reproducible example and the information below.
DustMite (https://github.com/CyberShadow/DustMite/wiki) can help with the reduction.
---
DMD v2.079.0-372-gc16061fe4
predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloatbinary    /home/seb/dlang/bin/dmddev
version   v2.079.0-372-gc16061fe4
config    /home/seb/dlang/dmd/generated/linux/release/64/dmd.conf
DFLAGS    -I/home/seb/dlang/dmd/generated/linux/release/64/../../../../../druntime/import -I/home/seb/dlang/dmd/generated/linux/release/64/../../../../../phobos -L-L/home/seb/dlang/dmd/generated/linux/release/64/../../../../../phobos/generated/linux/release/64 -L--export-dynamic -fPIC
---
core.exception.AssertError@dmd/mars.d(1091): Assertion failure

Is a stack trace going too far?

Assertion failures already have a stack trace right now (well, if assert's are enabled like in the release build) and they are very useful, so I don't think there's a reason not to show them.
If you were just asking about requesting to include it from the user I think "and the information below" catches this.

wilzbach · 2018-04-06T10:04:06Z

src/dmd/mars.d

+    stream.fprintf("---\n");
+    stream.fprintf("ERROR: This is a compiler bug.\n" ~
+            "Please report it via https://issues.dlang.org/enter_bug.cgi\n" ~
+            "with, preferably, a reduced, reproducible example and the information below.\n");


Maybe: "Include a a reduced, reproducible ..." ?

wilzbach · 2018-04-06T10:04:48Z

src/dmd/mars.d

+Params:
+    stream = output stream to print the information on
+*/
+extern(C) void internalFailureLogo(FILE* stream)


The backend was printing to stdout and I didn't want to change this behavior in this PR.

ibuclaw · 2018-04-06T10:47:41Z

Is a stack trace going too far?

No, stack trace is fine - and infact ideally all bug reports should have the file/line info of the ICE as it makes it easy to find possibly related issues.

ibuclaw · 2018-04-06T10:50:00Z

src/dmd/mars.d

+    stream.fprintf("ERROR: This is a compiler bug.\n" ~
+            "Please report it via https://issues.dlang.org/enter_bug.cgi\n" ~
+            "with, preferably, a reduced, reproducible example and the information below.\n");
+    stream.fprintf("DustMite (https://github.com/CyberShadow/DustMite/wiki) can help with the reduction.\n");


Time to make dustmite a dlang project? 😉

Yeah 👍
FYI: it gets checked out as a "full source" to the tools repo already (https://github.com/dlang/tools/tree/master/DustMite), which is a bit useless as no one is developing on this copy.

CC @andralex

JinShil · 2018-04-06T10:52:40Z

src/dmd/backend/util2.c

@@ -38,6 +38,8 @@ void *ph_calloc(size_t nbytes);
 void ph_free(void *p);
 void *ph_realloc(void *p , size_t nbytes);

+extern "C" void internalFailureLogo(FILE* stream); // from dmd/mars.d


"Logo" or "Log"?

DMD already had a function called "logo" that printed its version. Hence "Logo" as this is an extended version of it, but I really don't care about the name ;-)

Ok, I understand why it was called logo, but this isn't that. Also, I suggest making methods begin with a verb, so my preference would be something like "printInternalFailure".

andralex

This is a great idea, looking forward to it!

WalterBright · 2018-04-18T20:18:59Z

src/dmd/mars.d

    {
        OutBuffer buf;
        foreach (const str; *global.versionids)
        {
            buf.writeByte(' ');
            buf.writestring(str.toChars());
        }
-        message("predefs  %s", buf.peekString());
+        stream.fprintf("predefs  %.*s", buf.peekString());


Use %*s for 0 terminated strings.

WalterBright · 2018-04-18T20:22:00Z

src/dmd/mars.d

+*/
+extern(C) void internalFailureLogo(FILE* stream)
+{
+    stream.fprintf("---\n");


Some of these strings have separate fprintf's, some are joined with ~. The latter is better. Also, for mere strings, use fputs.

WalterBright · 2018-04-18T20:23:42Z

src/dmd/mars.d

+                }
+            }
+
+            if (flag.strchr(' '))


Shouldn't that be if (needsquoting) ?

I agree, but I just moved the code - check the red part on the left.

Apparently it's not that easy (breaks a test), so I will modify this in a different PR.

WalterBright · 2018-04-18T20:29:19Z

src/dmd/mars.d

+        foreach (flag; dflags.asDArray)
+        {
+            bool needsQuoting;
+            for (auto flagp = flag; flagp; flagp++)


Wat? Since when does incrementing a pointer until it is null work?

For null terminated strings?

FWIW I just moved the code - check the red part on the left.

null terminated string

That's incrementing the pointer until what it points to is null. Not until the pointer itself is null!

moved

I understand, and normally I would let things slide for that reason. But this code is so clearly wrong I don't think we should defer fixing it.

WalterBright · 2018-04-18T20:37:28Z

src/dmd/mars.d

+        getenv_setargv(readFromEnv(&environment, "DFLAGS"), &dflags);
+        environment.reset(1);
+        OutBuffer buf;
+        foreach (flag; dflags.asDArray)


This asDArray seems mighty peculiar, and is in fact not necessary.

Alternatively dereferencing can be used.

FWIW I just moved the code - check the red part on the left.

wilzbach

I understand, and normally I would let things slide for that reason. But this code is so clearly wrong I don't think we should defer fixing it.

Fair enough. Changed the code.

wilzbach · 2018-05-03T09:46:45Z

src/dmd/mars.d

+            bool needsQuoting;
+            foreach (c; flag[0 .. strlen(flag)])
+            {
+                if (!(isalnum(c) || c == '_' || c == '-' || c == '/' || c == '.'))


Example:

DFLAGS -I/home/seb/dlang/dmd/generated/linux/release/64/../../../../../druntime/import -I/home/seb/dlang/dmd/generated/linux/release/64/../../../../../phobos -L-L/home/seb/dlang/dmd/generated/linux/release/64/../../../../../phobos/generated/linux/release/64 -L--export-dynamic -fPIC

(AFAICT no quoting needed)

wilzbach · 2018-05-03T09:47:07Z

src/dmd/mars.d

+        foreach (flag; dflags[])
+        {
+            bool needsQuoting;
+            foreach (c; flag[0 .. strlen(flag)])


Sadly readFrom calls getenv which returns a boring, old C-string.

JinShil · 2018-05-07T01:06:11Z

@wilzbach Is this ready to go? Do you plan on adding anything else?

wilzbach · 2018-05-07T20:07:39Z

AFAICT this is ready (at least I don't plan on adding sth. else).
It would be great to combine this with DIP1006 (see #7980) and leave the asserts enabled, because at the moment this will only have effect for the backend code (the backend asserts stay enabled)

wilzbach requested review from MartinNowak, rainers and WalterBright as code owners April 5, 2018 03:11

wilzbach mentioned this pull request Apr 5, 2018

Custom assert handler to point to bugzilla #6103

Closed

wilzbach added the Review:Easy Review label Apr 6, 2018

wilzbach force-pushed the bugzilla branch 2 times, most recently from 37f5ccc to 7e74521 Compare April 6, 2018 10:02

wilzbach commented Apr 6, 2018

View reviewed changes

ibuclaw reviewed Apr 6, 2018

View reviewed changes

JinShil reviewed Apr 6, 2018

View reviewed changes

JinShil mentioned this pull request Apr 7, 2018

Automatically print the segtrace for segfaulting tests #8132

Merged

wilzbach added the Review:Needs Work label Apr 9, 2018

andralex approved these changes Apr 18, 2018

View reviewed changes

WalterBright reviewed Apr 18, 2018

View reviewed changes

wilzbach force-pushed the bugzilla branch from c29524c to fddb4e5 Compare May 3, 2018 09:01

wilzbach requested a review from Geod24 as a code owner May 3, 2018 09:01

dlang-bot removed the Review:Needs Work label May 3, 2018

wilzbach force-pushed the bugzilla branch 2 times, most recently from 7761923 to 7a85853 Compare May 3, 2018 09:45

wilzbach commented May 3, 2018

View reviewed changes

Point users to Bugzilla on ICEs

c402464

wilzbach force-pushed the bugzilla branch from 7a85853 to c402464 Compare May 4, 2018 15:51

andralex approved these changes May 7, 2018

View reviewed changes

andralex merged commit 247273e into dlang:master May 7, 2018

wilzbach deleted the bugzilla branch July 1, 2018 23:26

Point users to Bugzilla on ICEs #8138

Point users to Bugzilla on ICEs #8138

Conversation

wilzbach commented Apr 5, 2018 • edited Loading

Frontend

Backend

Other ideas:

dlang-bot commented Apr 5, 2018

Bugzilla references

Testing this PR locally

JinShil commented Apr 6, 2018

JinShil commented Apr 6, 2018

wilzbach commented Apr 6, 2018

JinShil commented Apr 6, 2018

JinShil commented Apr 6, 2018

JinShil commented Apr 6, 2018

wilzbach commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuclaw commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

andralex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil commented May 7, 2018

wilzbach commented May 7, 2018

wilzbach commented Apr 5, 2018 •

edited

Loading

JinShil Apr 6, 2018 •

edited

Loading